From: Zhong Jinghua <[email protected]>
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.
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 <[email protected]>
---
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;
--
2.31.1
Hi,
?? 2023/06/05 20:28, Zhong Jinghua ะด??:
> From: Zhong Jinghua <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
>