[PATCH v2] nvmet: Don't overflow subsysnqn

[PATCH v2] nvmet: Don't overflow subsysnqn

[Leo Stone]

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ff4aab278fa7e27e0f9e
Fixes: 95409e277d83 ("nvmet: implement unique discovery NQN")
Signed-off-by: Leo Stone <leocstone@gmail.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);
+	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);
 
 	return len;
-- 
2.43.0

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

[Caleb Sander]

On Wed, Dec 18, 2024 at 8:07 AM Leo Stone <leocstone@gmail.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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ff4aab278fa7e27e0f9e
> Fixes: 95409e277d83 ("nvmet: implement unique discovery NQN")
> Signed-off-by: Leo Stone <leocstone@gmail.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
>
>