diff mbox series

block/rnbd-srv: Check for unlikely string overflow

Message ID	20231212214738.work.169-kees@kernel.org
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; From: Kees Cook <keescook@chromium.org> To: "Md. Haris Iqbal" <haris.iqbal@ionos.com> Cc: Kees Cook <keescook@chromium.org>, kernel test robot <lkp@intel.com>, Jack Wang <jinpu.wang@ionos.com>, Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org Subject: [PATCH] block/rnbd-srv: Check for unlikely string overflow Date: Tue, 12 Dec 2023 13:47:42 -0800 Message-Id: <20231212214738.work.169-kees@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	block/rnbd-srv: Check for unlikely string overflow \| block/rnbd-srv: Check for unlikely string overflow

Commit Message

Kees Cook Dec. 12, 2023, 9:47 p.m. UTC

  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 <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312100355.lHoJPgKy-lkp@intel.com/
Cc: "Md. Haris Iqbal" <haris.iqbal@ionos.com>
Cc: Jack Wang <jinpu.wang@ionos.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/block/rnbd/rnbd-srv.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Comments

Jinpu Wang Dec. 13, 2023, 6:12 a.m. UTC | #1

On Tue, Dec 12, 2023 at 10:47 PM Kees Cook <keescook@chromium.org> wrote:
>
> 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 <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312100355.lHoJPgKy-lkp@intel.com/
> Cc: "Md. Haris Iqbal" <haris.iqbal@ionos.com>
> Cc: Jack Wang <jinpu.wang@ionos.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: linux-block@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
lgtm.
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/block/rnbd/rnbd-srv.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 65de51f3dfd9..ab78eab97d98 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -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 Dec. 13, 2023, 12:07 p.m. UTC | #2

On 12/13/23 05:47, Kees Cook wrote:
> 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 <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202312100355.lHoJPgKy-lkp@intel.com/
> Cc: "Md. Haris Iqbal" <haris.iqbal@ionos.com>
> Cc: Jack Wang <jinpu.wang@ionos.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: linux-block@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   drivers/block/rnbd/rnbd-srv.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 65de51f3dfd9..ab78eab97d98 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -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 */

Looks good.

Acked-by: Guoqing Jiang <guoqing.jiang@linux.dev>

Jens Axboe Dec. 13, 2023, 3:16 p.m. UTC | #3

On Tue, 12 Dec 2023 13:47:42 -0800, Kees Cook wrote:
> 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);
>       |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> [...]

Applied, thanks!

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

Best regards,

diff mbox series

Patch

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 65de51f3dfd9..ab78eab97d98 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -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 */