| Message ID | 20231212214738.work.169-kees@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/rnbd-srv: Check for unlikely string overflow | expand |
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 >
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>
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 --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 */
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(-)