linux-kernel - Re: [PATCH v2] nvmet: Don't overflow subsysnqn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADUfDZrV-_um_NzMiun==8JcDT0SEJ7qm4xt_gOmumuzRZUv9A@mail.gmail.com>
Date: Wed, 18 Dec 2024 09:16:43 -0800
From: Caleb Sander <csander@...estorage.com>
To: Leo Stone <leocstone@...il.com>
Cc: hch@....de, sagi@...mberg.me, kch@...dia.com, 
	linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	syzbot+ff4aab278fa7e27e0f9e@...kaller.appspotmail.com
Subject: Re: [PATCH v2] nvmet: Don't overflow subsysnqn

On Wed, Dec 18, 2024 at 8:07 AM Leo Stone <leocstone@...il.com> wrote:
>
> nvmet_root_discovery_nqn_store treats the subsysnqn string like a fixed
> size buffer, even though it is dynamically allocated to the size of the
> string.
>
> Create a new string with kstrdup instead of using the old buffer.
>
> Reported-by: syzbot+ff4aab278fa7e27e0f9e@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ff4aab278fa7e27e0f9e
> Fixes: 95409e277d83 ("nvmet: implement unique discovery NQN")
> Signed-off-by: Leo Stone <leocstone@...il.com>
> ---
> v2: Allocate memory outside the lock and handle errors.
> v1: https://lore.kernel.org/all/20241218005909.89092-2-leocstone@gmail.com/
> ---
>  drivers/nvme/target/configfs.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index eeee9e9b854c..3c9c98250cce 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -2254,12 +2254,17 @@ static ssize_t nvmet_root_discovery_nqn_store(struct config_item *item,
>                 const char *page, size_t count)
>  {
>         struct list_head *entry;
> +       char *new_nqn;
>         size_t len;
>
>         len = strcspn(page, "\n");
>         if (!len || len > NVMF_NQN_FIELD_LEN - 1)
>                 return -EINVAL;
>
> +       new_nqn = kstrdup(page, GFP_KERNEL);

Looks like this is now keeping the trailing \n in the string, when
previously it was excluded. Use kstrndup() instead?

> +       if (!new_nqn)
> +               return -ENOMEM;
> +
>         down_write(&nvmet_config_sem);
>         list_for_each(entry, &nvmet_subsystems_group.cg_children) {
>                 struct config_item *item =
> @@ -2268,11 +2273,12 @@ static ssize_t nvmet_root_discovery_nqn_store(struct config_item *item,
>                 if (!strncmp(config_item_name(item), page, len)) {
>                         pr_err("duplicate NQN %s\n", config_item_name(item));
>                         up_write(&nvmet_config_sem);
> +                       kfree(new_nqn);
>                         return -EINVAL;
>                 }
>         }
> -       memset(nvmet_disc_subsys->subsysnqn, 0, NVMF_NQN_FIELD_LEN);
> -       memcpy(nvmet_disc_subsys->subsysnqn, page, len);
> +       kfree(nvmet_disc_subsys->subsysnqn);
> +       nvmet_disc_subsys->subsysnqn = new_nqn;
>         up_write(&nvmet_config_sem);

The kfree() could also be moved after the up_write(), but it's
probably less of a concern than the potentially blocking kstrdup().

>
>         return len;
> --
> 2.43.0
>
>