block/rnbd-srv: Check for unlikely string overflow

PATCH Applied Kees Cook 4 participants 4 messages Dec 12, 2023 – Dec 13, 2023

Subsystems (2)

Block layer RNBD block drivers

lore

Share & tools

lore

Kees Cook

author

2023-12-12 21:47 UTC #0 lore raw

PATCH 1/1 block/rnbd-srv: Check for unlikely string overflow

Since "dev_search_path" can technically be as large as PATH_MAX, there was a risk of truncation when copying it and a second string into "full_path" since it was also PATH_MAX sized. The W=1 builds were reporting this warning:

drivers/block/rnbd/rnbd-srv.c: In function 'process_msg_open.isra':
drivers/block/rnbd/rnbd-srv.c:616:51: warning: '%s' directive output may be truncated writing up to 254 bytes into a region of size between 0 and 4095 [-Wformat-truncation=]
616 | snprintf(full_path, PATH_MAX, "%s/%s",
| ^~
In function 'rnbd_srv_get_full_path',
inlined from 'process_msg_open.isra' at drivers/block/rnbd/rnbd-srv.c:721:14: drivers/block/rnbd/rnbd-srv.c:616:17: note: 'snprintf' output between 2 and 4351 bytes into a destination of size 4096

  616 |                 snprintf(full_path, PATH_MAX, "%s/%s",
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  617 |                          dev_search_path, dev_name);
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~

To fix this, unconditionally check for truncation (as was already done for the case where "%SESSNAME%" was present).

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here

Cc "Md. Haris Iqbal" <[email protected]>

Cc Jack Wang <[email protected]>

Cc Jens Axboe <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 drivers/block/rnbd/rnbd-srv.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/block/rnbd/rnbd-srv.c +10 -9

index 65de51f3dfd9..ab78eab97d98 100644
@@ -585,6 +585,7 @@ static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
 {
 	char *full_path;
 	char *a, *b;
+	int len;
 
 	full_path = kmalloc(PATH_MAX, GFP_KERNEL);
 	if (!full_path)

@@ -596,19 +597,19 @@ static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
 	 */
 	a = strnstr(dev_search_path, "%SESSNAME%", sizeof(dev_search_path));
 	if (a) {
-		int len = a - dev_search_path;
+		len = a - dev_search_path;
 
 		len = snprintf(full_path, PATH_MAX, "%.*s/%s/%s", len,
 			       dev_search_path, srv_sess->sessname, dev_name);
-		if (len >= PATH_MAX) {
-			pr_err("Too long path: %s, %s, %s\n",
-			       dev_search_path, srv_sess->sessname, dev_name);
-			kfree(full_path);
-			return ERR_PTR(-EINVAL);
-		}
 	} else {
-		snprintf(full_path, PATH_MAX, "%s/%s",
-			 dev_search_path, dev_name);
+		len = snprintf(full_path, PATH_MAX, "%s/%s",
+			       dev_search_path, dev_name);
+	}
+	if (len >= PATH_MAX) {
+		pr_err("Too long path: %s, %s, %s\n",
+		       dev_search_path, srv_sess->sessname, dev_name);
+		kfree(full_path);
+		return ERR_PTR(-EINVAL);
 	}
 
 	/* eliminitate duplicated slashes */

Signature · 1 line

2.34.1

Jinpu Wang

maintainer Acked-by on v1 1/1

2023-12-13 06:12 UTC #1 lore raw

Quoted 20 lines · Kees Cook

Since "dev_search_path" can technically be as large as PATH_MAX, there was a risk of truncation when copying it and a second string into "full_path" since it was also PATH_MAX sized. The W=1 builds were reporting this warning:
drivers/block/rnbd/rnbd-srv.c: In function 'process_msg_open.isra':
drivers/block/rnbd/rnbd-srv.c:616:51: warning: '%s' directive output may be truncated writing up to 254 bytes into a region of size between 0 and 4095 [-Wformat-truncation=]
616 | snprintf(full_path, PATH_MAX, "%s/%s",
| ^~
In function 'rnbd_srv_get_full_path',
inlined from 'process_msg_open.isra' at drivers/block/rnbd/rnbd-srv.c:721:14: drivers/block/rnbd/rnbd-srv.c:616:17: note: 'snprintf' output between 2 and 4351 bytes into a destination of size 4096
  616 |                 snprintf(full_path, PATH_MAX, "%s/%s",
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  617 |                          dev_search_path, dev_name);
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~
To fix this, unconditionally check for truncation (as was already done for the case where "%SESSNAME%" was present).

Quoted 7 lines

Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
Cc "Md. Haris Iqbal" <[email protected]>
Cc Jack Wang <[email protected]>
Cc Jens Axboe <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

lgtm.

Acked-by Jack Wang <[email protected]>

Quoted 48 lines

---
 drivers/block/rnbd/rnbd-srv.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/block/rnbd/rnbd-srv.c +10 ~~-10~~

index 65de51f3dfd9..ab78eab97d98 100644
@@ -585,6 +585,7 @@ static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
 {
        char *full_path;
        char *a, *b;
+       int len;
 
        full_path = kmalloc(PATH_MAX, GFP_KERNEL);
        if (!full_path)

@@ -596,19 +597,19 @@ static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
         */
        a = strnstr(dev_search_path, "%SESSNAME%", sizeof(dev_search_path));
        if (a) {
-               int len = a - dev_search_path;
+               len = a - dev_search_path;
 
                len = snprintf(full_path, PATH_MAX, "%.*s/%s/%s", len,
                               dev_search_path, srv_sess->sessname, dev_name);
-               if (len >= PATH_MAX) {
-                       pr_err("Too long path: %s, %s, %s\n",
-                              dev_search_path, srv_sess->sessname, dev_name);
-                       kfree(full_path);
-                       return ERR_PTR(-EINVAL);
-               }
        } else {
-               snprintf(full_path, PATH_MAX, "%s/%s",
-                        dev_search_path, dev_name);
+               len = snprintf(full_path, PATH_MAX, "%s/%s",
+                              dev_search_path, dev_name);
+       }
+       if (len >= PATH_MAX) {
+               pr_err("Too long path: %s, %s, %s\n",
+                      dev_search_path, srv_sess->sessname, dev_name);
+               kfree(full_path);
+               return ERR_PTR(-EINVAL);
        }
 
        /* eliminitate duplicated slashes */
--

2.34.1

Guoqing Jiang

Acked-by on v1 1/1

2023-12-13 12:15 UTC #2 lore raw

In reply to Kees Cook

Quoted 61 lines · Kees Cook

Since "dev_search_path" can technically be as large as PATH_MAX, there was a risk of truncation when copying it and a second string into "full_path" since it was also PATH_MAX sized. The W=1 builds were reporting this warning:
drivers/block/rnbd/rnbd-srv.c: In function 'process_msg_open.isra':
drivers/block/rnbd/rnbd-srv.c:616:51: warning: '%s' directive output may be truncated writing up to 254 bytes into a region of size between 0 and 4095 [-Wformat-truncation=]
616 | snprintf(full_path, PATH_MAX, "%s/%s",
| ^~
In function 'rnbd_srv_get_full_path',
inlined from 'process_msg_open.isra' at drivers/block/rnbd/rnbd-srv.c:721:14: drivers/block/rnbd/rnbd-srv.c:616:17: note: 'snprintf' output between 2 and 4351 bytes into a destination of size 4096
   616 |                 snprintf(full_path, PATH_MAX, "%s/%s",
       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   617 |                          dev_search_path, dev_name);
       |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~
To fix this, unconditionally check for truncation (as was already done for the case where "%SESSNAME%" was present).
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
Cc "Md. Haris Iqbal" <[email protected]>
Cc Jack Wang <[email protected]>
Cc Jens Axboe <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
  drivers/block/rnbd/rnbd-srv.c | 19 ++++++++++---------
  1 file changed, 10 insertions(+), 9 deletions(-)
drivers/block/rnbd/rnbd-srv.c +3 -9
index 65de51f3dfd9..ab78eab97d98 100644
@@ -585,6 +585,7 @@ static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
  {
  	char *full_path;
  	char *a, *b;
+	int len;
 
  	full_path = kmalloc(PATH_MAX, GFP_KERNEL);
  	if (!full_path)
@@ -596,19 +597,19 @@ static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess,
  	 */
  	a = strnstr(dev_search_path, "%SESSNAME%", sizeof(dev_search_path));
  	if (a) {
-		int len = a - dev_search_path;
+		len = a - dev_search_path;
 
  		len = snprintf(full_path, PATH_MAX, "%.*s/%s/%s", len,
  			       dev_search_path, srv_sess->sessname, dev_name);
-		if (len >= PATH_MAX) {
-			pr_err("Too long path: %s, %s, %s\n",
-			       dev_search_path, srv_sess->sessname, dev_name);
-			kfree(full_path);
-			return ERR_PTR(-EINVAL);
-		}
  	} else {
-		snprintf(full_path, PATH_MAX, "%s/%s",
-			 dev_search_path, dev_name);
+		len = snprintf(full_path, PATH_MAX, "%s/%s",

drivers/block/rnbd/rnbd-srv.c static char *rnbd_srv_get_full_path(struct rnbd_srv_session *srv_sess, · line 615

Quoted 10 lines

drivers/block/rnbd/rnbd-srv.c +7 -0

+			       dev_search_path, dev_name);
+	}
+	if (len >= PATH_MAX) {
+		pr_err("Too long path: %s, %s, %s\n",
+		       dev_search_path, srv_sess->sessname, dev_name);
+		kfree(full_path);
+		return ERR_PTR(-EINVAL);
  	}
 
  	/* eliminitate duplicated slashes */

Looks good.

Acked-by Guoqing Jiang <[email protected]>

Jens Axboe

maintainer Applied

2023-12-13 15:16 UTC #3 lore raw

In reply to Kees Cook

Quoted 11 lines · Kees Cook

Since "dev_search_path" can technically be as large as PATH_MAX, there was a risk of truncation when copying it and a second string into "full_path" since it was also PATH_MAX sized. The W=1 builds were reporting this warning:
drivers/block/rnbd/rnbd-srv.c: In function 'process_msg_open.isra':
drivers/block/rnbd/rnbd-srv.c:616:51: warning: '%s' directive output may be truncated writing up to 254 bytes into a region of size between 0 and 4095 [-Wformat-truncation=]
616 | snprintf(full_path, PATH_MAX, "%s/%s",
| ^~
In function 'rnbd_srv_get_full_path',
inlined from 'process_msg_open.isra' at drivers/block/rnbd/rnbd-srv.c:721:14: drivers/block/rnbd/rnbd-srv.c:616:17: note: 'snprintf' output between 2 and 4351 bytes into a destination of size 4096

Quoted 6 lines

  616 |                 snprintf(full_path, PATH_MAX, "%s/%s",
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  617 |                          dev_search_path, dev_name);
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~

[...]

Applied, thanks!

[1/1] block/rnbd-srv: Check for unlikely string overflow

      commit: 9e4bf6a08d1e127bcc4bd72557f2dfafc6bc7f41

Best regards,

Signature · 1 line

Jens Axboe