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

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

[wenyang.linux]

From: Yu Kuai <yukuai3@huawei.com>

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 <yukuai3@huawei.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Link: https://lore.kernel.org/r/20211102015237.2309763-2-yebin10@huawei.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
---
 drivers/block/nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index bf8148ebd858..bd05eaf04143 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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;
 	}
-- 
2.37.2

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

[Greg Kroah-Hartman]

On Tue, Feb 21, 2023 at 02:04:47AM +0800, wenyang.linux@foxmail.com wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> 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

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

[Greg Kroah-Hartman]

On Thu, Feb 23, 2023 at 10:39:17AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Feb 21, 2023 at 02:04:47AM +0800, wenyang.linux@foxmail.com wrote:
> > From: Yu Kuai <yukuai3@huawei.com>
> > 
> > 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

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

[wenyang.linux]

From: Yu Kuai <yukuai3@huawei.com>

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 <yukuai3@huawei.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Link: https://lore.kernel.org/r/20211102015237.2309763-3-yebin10@huawei.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
---
 drivers/block/nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index bd05eaf04143..1ddae8f768d5 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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;
 	}
-- 
2.37.2

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

[wenyang.linux]

From: Zhang Wensheng <zhangwensheng5@huawei.com>

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 <zhangwensheng5@huawei.com>
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Link: https://lore.kernel.org/r/20220521073749.3146892-6-yukuai3@huawei.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
---
 drivers/block/nbd.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 1ddae8f768d5..dbcd903ba128 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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;
-- 
2.37.2

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

[Greg Kroah-Hartman]

On Tue, Feb 21, 2023 at 02:04:49AM +0800, wenyang.linux@foxmail.com wrote:
> From: Zhang Wensheng <zhangwensheng5@huawei.com>
> 
> 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 <zhangwensheng5@huawei.com>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> Link: https://lore.kernel.org/r/20220521073749.3146892-6-yukuai3@huawei.com
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> Cc: stable@vger.kernel.org # v5.10+
> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
> ---
>  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