Skip to contentSkip to conversation

nbd: fix possible overflow for 'first_minor' in nbd_dev_add()

PATCH In review [email protected] 2 participants 6 messages Feb 20, 2023 – Feb 23, 2023 1 message not in the archive Stable branch

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

[email protected]

author
#1 lore raw

PATCH 2/4 nbd: fix max value for 'first_minor'

From: Yu Kuai <[email protected]>

commit e4c4871a73944353ea23e319de27ef73ce546623 upstream.

commit b1a811633f73 ("block: nbd: add sanity check for first_minor") checks that 'first_minor' should not be greater than 0xff, which is wrong. Whitout the commit, the details that when user pass 0x100000, it ends up create sysfs dir "/sys/block/43:0" are as follows:

nbd_dev_add
disk->first_minor = index << part_shift
-> default part_shift is 5, first_minor is 0x2000000 device_add_disk
ddev->devt = MKDEV(disk->major, disk->first_minor)

    -> (0x2b << 20) | (0x2000000) = 0x2b00000
   device_add
    device_create_sys_dev_entry
	 format_dev_t
	  sprintf(buffer, "%u:%u", MAJOR(dev), MINOR(dev));
	   -> got 43:0
	  sysfs_create_link -> /sys/block/43:0

By the way, with the wrong fix, when part_shift is the default value, only 8 ndb devices can be created since 8 << 5 is greater than 0xff.

Since the max bits for 'first_minor' should be the same as what MKDEV() does, which is 20. Change the upper bound of 'first_minor' from 0xff to 0xfffff.

Fixes b1a811633f73 ("block: nbd: add sanity check for first_minor")
Signed-off-by Yu Kuai <[email protected]>
Reviewed-by Josef Bacik <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Jens Axboe <[email protected]>
Cc [email protected] # v5.10+
Signed-off-by Wen Yang <[email protected]>
---
 drivers/block/nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/nbd.c +3 -3
index bf8148ebd858..bd05eaf04143 100644
@@ -1773,11 +1773,11 @@ static int nbd_dev_add(int index)
 	disk->major = NBD_MAJOR;
 
 	/* Too big first_minor can cause duplicate creation of
-	 * sysfs files/links, since first_minor will be truncated to
-	 * byte in __device_add_disk().
+	 * sysfs files/links, since MKDEV() expect that the max bits of
+	 * first_minor is 20.
 	 */
 	disk->first_minor = index << part_shift;
-	if (disk->first_minor > 0xff) {
+	if (disk->first_minor > MINORMASK) {
 		err = -EINVAL;
 		goto out_free_idr;
 	}
Signature · 1 line
2.37.2
maintainer
#2 lore raw

Re: [PATCH 5.10 2/4] nbd: fix max value for 'first_minor'

Quoted 3 lines · On Tue, Feb 21, 2023 at 02:04:47AM +0800, [email protected] wrote:

From: Yu Kuai <[email protected]>

commit e4c4871a73944353ea23e319de27ef73ce546623 upstream.

<snip>

I never recieved patch 0/4 or 1/4 of this series saying what it is for (and neither did lore.kernel.org.)

Please fix up and resend a v2 series.

thanks,

greg k-h

maintainer
#3 lore raw

Re: [PATCH 5.10 2/4] nbd: fix max value for 'first_minor'

Quoted 9 lines · Greg Kroah-Hartman
Quoted 3 lines · On Tue, Feb 21, 2023 at 02:04:47AM +0800, [email protected] wrote:

From: Yu Kuai <[email protected]>

commit e4c4871a73944353ea23e319de27ef73ce546623 upstream.

<snip>

I never recieved patch 0/4 or 1/4 of this series saying what it is for (and neither did lore.kernel.org.)

Ah, now I found patch 1/4, your email threading got broken somehow, odd...

thanks

greg k-h

author
#4 lore raw

PATCH 3/4 nbd: fix possible overflow for 'first_minor' in nbd_dev_add()

In reply to a message not in the archive

From: Yu Kuai <[email protected]>

commit 940c264984fd1457918393c49674f6b39ee16506 upstream.

If 'part_shift' is not zero, then 'index << part_shift' might overflow to a value that is not greater than '0xfffff', then sysfs might complains about duplicate creation.

Fixes b0d9111a2d53 ("nbd: use an idr to keep track of nbd devices")
Signed-off-by Yu Kuai <[email protected]>
Reviewed-by Josef Bacik <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Jens Axboe <[email protected]>
Cc [email protected] # v5.10+
Signed-off-by Wen Yang <[email protected]>
---
 drivers/block/nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/block/nbd.c +3 -3
index bd05eaf04143..1ddae8f768d5 100644
@@ -1773,11 +1773,11 @@ static int nbd_dev_add(int index)
 	disk->major = NBD_MAJOR;
 
 	/* Too big first_minor can cause duplicate creation of
-	 * sysfs files/links, since MKDEV() expect that the max bits of
-	 * first_minor is 20.
+	 * sysfs files/links, since index << part_shift might overflow, or
+	 * MKDEV() expect that the max bits of first_minor is 20.
 	 */
 	disk->first_minor = index << part_shift;
-	if (disk->first_minor > MINORMASK) {
+	if (disk->first_minor < index || disk->first_minor > MINORMASK) {
 		err = -EINVAL;
 		goto out_free_idr;
 	}
Signature · 1 line
2.37.2
author
#5 lore raw

PATCH 4/4 nbd: fix possible overflow on 'first_minor' in nbd_dev_add()

In reply to a message not in the archive

From: Zhang Wensheng <[email protected]>

commit 858f1bf65d3d9c00b5e2d8ca87dc79ed88267c98 upstream.

When 'index' is a big numbers, it may become negative which forced to 'int'. then 'index << part_shift' might overflow to a positive value that is not greater than '0xfffff', then sysfs might complains about duplicate creation. Because of this, move the 'index' judgment to the front will fix it and be better.

Fixes b0d9111a2d53 ("nbd: use an idr to keep track of nbd devices")
Fixes 940c264984fd ("nbd: fix possible overflow for 'first_minor' in nbd_dev_add()")
Signed-off-by Zhang Wensheng <[email protected]>
Signed-off-by Yu Kuai <[email protected]>
Reviewed-by Josef Bacik <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Jens Axboe <[email protected]>
Cc [email protected] # v5.10+
Signed-off-by Wen Yang <[email protected]>
---
 drivers/block/nbd.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
drivers/block/nbd.c +12 -11
index 1ddae8f768d5..dbcd903ba128 100644
@@ -1771,17 +1771,7 @@ static int nbd_dev_add(int index)
 	refcount_set(&nbd->refs, 1);
 	INIT_LIST_HEAD(&nbd->list);
 	disk->major = NBD_MAJOR;
-
-	/* Too big first_minor can cause duplicate creation of
-	 * sysfs files/links, since index << part_shift might overflow, or
-	 * MKDEV() expect that the max bits of first_minor is 20.
-	 */
 	disk->first_minor = index << part_shift;
-	if (disk->first_minor < index || disk->first_minor > MINORMASK) {
-		err = -EINVAL;
-		goto out_free_idr;
-	}
-
 	disk->fops = &nbd_fops;
 	disk->private_data = nbd;
 	sprintf(disk->disk_name, "nbd%d", index);

@@ -1875,8 +1865,19 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 	if (!netlink_capable(skb, CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (info->attrs[NBD_ATTR_INDEX])
+	if (info->attrs[NBD_ATTR_INDEX]) {
 		index = nla_get_u32(info->attrs[NBD_ATTR_INDEX]);
+
+		/*
+		 * Too big first_minor can cause duplicate creation of
+		 * sysfs files/links, since index << part_shift might overflow, or
+		 * MKDEV() expect that the max bits of first_minor is 20.
+		 */
+		if (index < 0 || index > MINORMASK >> part_shift) {
+			printk(KERN_ERR "nbd: illegal input index %d\n", index);
+			return -EINVAL;
+		}
+	}
 	if (!info->attrs[NBD_ATTR_SOCKETS]) {
 		printk(KERN_ERR "nbd: must specify at least one socket\n");
 		return -EINVAL;
Signature · 1 line
2.37.2
maintainer
#6 lore raw

Re: [PATCH 5.10 4/4] nbd: fix possible overflow on 'first_minor' in nbd_dev_add()

Quoted 19 lines · On Tue, Feb 21, 2023 at 02:04:49AM +0800, [email protected] wrote:

From: Zhang Wensheng <[email protected]>

commit 858f1bf65d3d9c00b5e2d8ca87dc79ed88267c98 upstream.

When 'index' is a big numbers, it may become negative which forced to 'int'. then 'index << part_shift' might overflow to a positive value that is not greater than '0xfffff', then sysfs might complains about duplicate creation. Because of this, move the 'index' judgment to the front will fix it and be better.

Fixes b0d9111a2d53 ("nbd: use an idr to keep track of nbd devices")
Fixes 940c264984fd ("nbd: fix possible overflow for 'first_minor' in nbd_dev_add()")
Signed-off-by Zhang Wensheng <[email protected]>
Signed-off-by Yu Kuai <[email protected]>
Reviewed-by Josef Bacik <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Jens Axboe <[email protected]>
Cc [email protected] # v5.10+
Signed-off-by Wen Yang <[email protected]>
Quoted 3 lines
---
 drivers/block/nbd.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

This is also needed in 5.15.y, please be more careful, you do not want to have a regression when moving to a newer kernel version.

I've queued it up there as well.

thanks,

greg k-h