* [PATCH] block/rnbd-srv: Check for unlikely string overflow
@ 2023-12-12 21:47 Kees Cook
2023-12-13 6:12 ` Jinpu Wang
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Kees Cook @ 2023-12-12 21:47 UTC (permalink / raw)
To: Md. Haris Iqbal
Cc: Kees Cook, kernel test robot, Jack Wang, Jens Axboe, linux-block,
linux-kernel, linux-hardening
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 */
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] block/rnbd-srv: Check for unlikely string overflow
2023-12-12 21:47 [PATCH] block/rnbd-srv: Check for unlikely string overflow Kees Cook
@ 2023-12-13 6:12 ` Jinpu Wang
2023-12-13 12:07 ` Guoqing Jiang
2023-12-13 15:16 ` Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Jinpu Wang @ 2023-12-13 6:12 UTC (permalink / raw)
To: Kees Cook
Cc: Md. Haris Iqbal, kernel test robot, Jens Axboe, linux-block,
linux-kernel, linux-hardening
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] block/rnbd-srv: Check for unlikely string overflow
2023-12-12 21:47 [PATCH] block/rnbd-srv: Check for unlikely string overflow Kees Cook
2023-12-13 6:12 ` Jinpu Wang
@ 2023-12-13 12:07 ` Guoqing Jiang
2023-12-13 15:16 ` Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Guoqing Jiang @ 2023-12-13 12:07 UTC (permalink / raw)
To: Kees Cook, Md. Haris Iqbal
Cc: kernel test robot, Jack Wang, Jens Axboe, linux-block,
linux-kernel, linux-hardening
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] block/rnbd-srv: Check for unlikely string overflow
2023-12-12 21:47 [PATCH] block/rnbd-srv: Check for unlikely string overflow Kees Cook
2023-12-13 6:12 ` Jinpu Wang
2023-12-13 12:07 ` Guoqing Jiang
@ 2023-12-13 15:16 ` Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2023-12-13 15:16 UTC (permalink / raw)
To: Md. Haris Iqbal, Kees Cook
Cc: kernel test robot, Jack Wang, linux-block, linux-kernel,
linux-hardening
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,
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-13 15:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-12 21:47 [PATCH] block/rnbd-srv: Check for unlikely string overflow Kees Cook
2023-12-13 6:12 ` Jinpu Wang
2023-12-13 12:07 ` Guoqing Jiang
2023-12-13 15:16 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox