From: Guoqing Jiang <guoqing.jiang@linux.dev>
To: Kees Cook <keescook@chromium.org>,
"Md. Haris Iqbal" <haris.iqbal@ionos.com>
Cc: 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: Re: [PATCH] block/rnbd-srv: Check for unlikely string overflow
Date: Wed, 13 Dec 2023 20:07:51 +0800 [thread overview]
Message-ID: <78c25a62-44d5-d40d-a3e8-1f40a0c95c11@linux.dev> (raw)
In-Reply-To: <20231212214738.work.169-kees@kernel.org>
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>
next prev parent reply other threads:[~2023-12-13 12:15 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-12-13 15:16 ` Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=78c25a62-44d5-d40d-a3e8-1f40a0c95c11@linux.dev \
--to=guoqing.jiang@linux.dev \
--cc=axboe@kernel.dk \
--cc=haris.iqbal@ionos.com \
--cc=jinpu.wang@ionos.com \
--cc=keescook@chromium.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox