| Message ID | 20230605122838.2148878-1-zhongjinghua@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] loop: Add parm check in loop_control_ioctl | expand |
Hi, 在 2023/06/05 20:28, Zhong Jinghua 写道: > From: Zhong Jinghua <zhongjinghua@huawei.com> > > We found that in loop_control_ioctl, the kernel panic can be easily caused: > > 1. syscall(__NR_ioctl, r[1], 0x4c80, 0x80000200000ul); > Create a loop device 0x80000200000ul. > In fact, in the code, it is used as the first_minor number, and the > first_minor number is 0. > So the created loop device number is 7:0. > > 2. syscall(__NR_ioctl, r[2], 0x4c80, 0ul); > Create a loop device 0x0ul. > Since the 7:0 device has been created in 1, add_disk will fail because > the major and first_minor numbers are consistent. > > 3. syscall(__NR_ioctl, r[5], 0x4c81, 0ul); > Delete the device that failed to create, the kernel panics. Please notice that kernel panic won't be triggered because add_disk() has appropriate error handling. Thanks, Kuai > > Panic like below: > BUG: KASAN: null-ptr-deref in device_del+0xb3/0x840 drivers/base/core.c:3107 > Call Trace: > kill_device drivers/base/core.c:3079 [inline] > device_del+0xb3/0x840 drivers/base/core.c:3107 > del_gendisk+0x463/0x5f0 block/genhd.c:971 > loop_remove drivers/block/loop.c:2190 [inline] > loop_control_ioctl drivers/block/loop.c:2289 [inline] > > The stack like below: > Create loop device: > loop_control_ioctl > loop_add > add_disk > device_add_disk > bdi_register > bdi_register_va > device_create > device_create_groups_vargs > device_add > kfree(dev->p); > dev->p = NULL; > > Remove loop device: > loop_control_ioctl > loop_remove > del_gendisk > device_del > kill_device > if (dev->p->dead) // p is null > > Fix it by adding a check for parm. > > Fixes: 770fe30a46a1 ("loop: add management interface for on-demand device allocation") > Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com> > --- > drivers/block/loop.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 76b96c42f417..60f2a31c4a24 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -2084,6 +2084,17 @@ static int loop_add(struct loop_device **l, int i) > struct gendisk *disk; > int err; > > + /* > + * i << part_shift is actually used as the first_minor. > + * So here should avoid i << part_shift overflow. > + * And, MKDEV() expect that the max bits of > + * first_minor is 20. > + */ > + if (i > 0 && i > MINORMASK >> part_shift) { > + err = -EINVAL; > + goto out; > + } > + > err = -ENOMEM; > lo = kzalloc(sizeof(*lo), GFP_KERNEL); > if (!lo) > @@ -2097,7 +2108,8 @@ static int loop_add(struct loop_device **l, int i) > if (err == -ENOSPC) > err = -EEXIST; > } else { > - err = idr_alloc(&loop_index_idr, lo, 0, 0, GFP_KERNEL); > + err = idr_alloc(&loop_index_idr, lo, 0, > + (MINORMASK >> part_shift) + 1, GFP_KERNEL); > } > if (err < 0) > goto out_free_dev; >
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 76b96c42f417..60f2a31c4a24 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -2084,6 +2084,17 @@ static int loop_add(struct loop_device **l, int i) struct gendisk *disk; int err; + /* + * i << part_shift is actually used as the first_minor. + * So here should avoid i << part_shift overflow. + * And, MKDEV() expect that the max bits of + * first_minor is 20. + */ + if (i > 0 && i > MINORMASK >> part_shift) { + err = -EINVAL; + goto out; + } + err = -ENOMEM; lo = kzalloc(sizeof(*lo), GFP_KERNEL); if (!lo) @@ -2097,7 +2108,8 @@ static int loop_add(struct loop_device **l, int i) if (err == -ENOSPC) err = -EEXIST; } else { - err = idr_alloc(&loop_index_idr, lo, 0, 0, GFP_KERNEL); + err = idr_alloc(&loop_index_idr, lo, 0, + (MINORMASK >> part_shift) + 1, GFP_KERNEL); } if (err < 0) goto out_free_dev;