linux-kernel - Re: [PATCH -next] loop: Add parm check in loop_control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 5 Jun 2023 20:36:26 +0800
From:   Yu Kuai <yukuai1@...weicloud.com>
To:     Zhong Jinghua <zhongjinghua@...weicloud.com>, axboe@...nel.dk,
        kay.sievers@...y.org
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        zhongjinghua@...wei.com, yi.zhang@...wei.com, yangerkun@...wei.com,
        "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next] loop: Add parm check in loop_control_ioctl

Hi,

在 2023/06/05 20:28, Zhong Jinghua 写道:
> From: Zhong Jinghua <zhongjinghua@...wei.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@...wei.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;
>