Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH -next] loop: Add parm check in loop_control_ioctl
To:     Zhong Jinghua <zhongjinghua@huaweicloud.com>, axboe@kernel.dk,
        kay.sievers@vrfy.org
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhongjinghua@huawei.com, yi.zhang@huawei.com, yangerkun@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20230605122838.2148878-1-zhongjinghua@huaweicloud.com>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <aa6963b1-6add-161f-aac2-6e30634f9f19@huaweicloud.com>
Date:   Mon, 5 Jun 2023 20:36:26 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20230605122838.2148878-1-zhongjinghua@huaweicloud.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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;
>