From: Zhong Jinghua <[email protected]>
In the block_ioctl, we can pass in the unsigned number 0x8000000000000000
as an input parameter, like below:
block_ioctl
blkdev_ioctl
blkpg_ioctl
blkpg_do_ioctl
copy_from_user
bdev_add_partition
add_partition
p->start_sect = start; // start = 0x8000000000000000
Then, there was an warning when submit bio:
WARNING: CPU: 0 PID: 382 at fs/iomap/apply.c:54
Call trace:
iomap_apply+0x644/0x6e0
__iomap_dio_rw+0x5cc/0xa24
iomap_dio_rw+0x4c/0xcc
ext4_dio_read_iter
ext4_file_read_iter
ext4_file_read_iter+0x318/0x39c
call_read_iter
lo_rw_aio.isra.0+0x748/0x75c
do_req_filebacked+0x2d4/0x370
loop_handle_cmd
loop_queue_work+0x94/0x23c
kthread_worker_fn+0x160/0x6bc
loop_kthread_worker_fn+0x3c/0x50
kthread+0x20c/0x25c
ret_from_fork+0x10/0x18
Stack:
submit_bio_noacct
submit_bio_checks
blk_partition_remap
bio->bi_iter.bi_sector += p->start_sect
// bio->bi_iter.bi_sector = 0xffc0000000000000 + 65408
..
loop_queue_work
loop_handle_cmd
do_req_filebacked
pos = ((loff_t) blk_rq_pos(rq) << 9) + lo->lo_offset // pos < 0
lo_rw_aio
call_read_iter
ext4_dio_read_iter
__iomap_dio_rw
iomap_apply
ext4_iomap_begin
map.m_lblk = offset >> blkbits
ext4_set_iomap
iomap->offset = (u64) map->m_lblk << blkbits
// iomap->offset = 64512
WARN_ON(iomap.offset > pos) // iomap.offset = 64512 and pos < 0
This is unreasonable for start + length > disk->part0.nr_sects. There is
already a similar check in blk_add_partition().
Fix it by adding a check in bdev_add_partition().
Reported-by: Zhihao Cheng <[email protected]>
Signed-off-by: Zhong Jinghua <[email protected]>
---
block/ioctl.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/block/ioctl.c b/block/ioctl.c
index 9c5f637ff153..3223ea862523 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -33,9 +33,16 @@ static int blkpg_do_ioctl(struct block_device *bdev,
if (op == BLKPG_DEL_PARTITION)
return bdev_del_partition(disk, p.pno);
+ if (p.start < 0 || p.length <= 0 || p.start + p.length < 0)
+ return -EINVAL;
+
start = p.start >> SECTOR_SHIFT;
length = p.length >> SECTOR_SHIFT;
+ /* length may be equal to 0 after right shift */
+ if (!length || start + length > get_capacity(bdev->bd_disk))
+ return -EINVAL;
+
switch (op) {
case BLKPG_ADD_PARTITION:
/* check if partition is aligned to blocksize */
--
2.31.1
Hi,
?? 2023/05/22 15:06, Zhong Jinghua д??:
> From: Zhong Jinghua <[email protected]>
>
> In the block_ioctl, we can pass in the unsigned number 0x8000000000000000
> as an input parameter, like below:
>
> block_ioctl
> blkdev_ioctl
> blkpg_ioctl
> blkpg_do_ioctl
> copy_from_user
> bdev_add_partition
> add_partition
> p->start_sect = start; // start = 0x8000000000000000
start_sect is 0x8..... >> SECTOR_SHIFT.
>
> Then, there was an warning when submit bio:
>
> WARNING: CPU: 0 PID: 382 at fs/iomap/apply.c:54
> Call trace:
> iomap_apply+0x644/0x6e0
> __iomap_dio_rw+0x5cc/0xa24
> iomap_dio_rw+0x4c/0xcc
> ext4_dio_read_iter
> ext4_file_read_iter
> ext4_file_read_iter+0x318/0x39c
> call_read_iter
> lo_rw_aio.isra.0+0x748/0x75c
> do_req_filebacked+0x2d4/0x370
> loop_handle_cmd
> loop_queue_work+0x94/0x23c
> kthread_worker_fn+0x160/0x6bc
> loop_kthread_worker_fn+0x3c/0x50
> kthread+0x20c/0x25c
> ret_from_fork+0x10/0x18
>
> Stack:
>
> submit_bio_noacct
> submit_bio_checks
> blk_partition_remap
> bio->bi_iter.bi_sector += p->start_sect
> // bio->bi_iter.bi_sector = 0xffc0000000000000 + 65408
> ..
> loop_queue_work
> loop_handle_cmd
> do_req_filebacked
> pos = ((loff_t) blk_rq_pos(rq) << 9) + lo->lo_offset // pos < 0
> lo_rw_aio
> call_read_iter
> ext4_dio_read_iter
> __iomap_dio_rw
> iomap_apply
> ext4_iomap_begin
> map.m_lblk = offset >> blkbits
> ext4_set_iomap
> iomap->offset = (u64) map->m_lblk << blkbits
> // iomap->offset = 64512
> WARN_ON(iomap.offset > pos) // iomap.offset = 64512 and pos < 0
>
> This is unreasonable for start + length > disk->part0.nr_sects. There is
> already a similar check in blk_add_partition().
> Fix it by adding a check in bdev_add_partition().
The checking that you add is blkpg_do_ioctl().
>
> Reported-by: Zhihao Cheng <[email protected]>
> Signed-off-by: Zhong Jinghua <[email protected]>
> ---
> block/ioctl.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 9c5f637ff153..3223ea862523 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -33,9 +33,16 @@ static int blkpg_do_ioctl(struct block_device *bdev,
> if (op == BLKPG_DEL_PARTITION)
> return bdev_del_partition(disk, p.pno);
>
> + if (p.start < 0 || p.length <= 0 || p.start + p.length < 0)
> + return -EINVAL;
> +
> start = p.start >> SECTOR_SHIFT;
> length = p.length >> SECTOR_SHIFT;
>
> + /* length may be equal to 0 after right shift */
> + if (!length || start + length > get_capacity(bdev->bd_disk))
> + return -EINVAL;
> +
> switch (op) {
> case BLKPG_ADD_PARTITION:
> /* check if partition is aligned to blocksize */
>
The change itself looks good to me, feel free to add:
Reviewed-by: Yu Kuai <[email protected]>
Hi,
在 2023/05/25 10:14, Yu Kuai 写道:
> Hi,
>
> 在 2023/05/22 15:06, Zhong Jinghua 写道:
>> From: Zhong Jinghua <[email protected]>
>>
>> In the block_ioctl, we can pass in the unsigned number 0x8000000000000000
>> as an input parameter, like below:
>>
>> block_ioctl
>> blkdev_ioctl
>> blkpg_ioctl
>> blkpg_do_ioctl
>> copy_from_user
>> bdev_add_partition
>> add_partition
>> p->start_sect = start; // start = 0x8000000000000000
>
> start_sect is 0x8..... >> SECTOR_SHIFT.
>
>>
>> Then, there was an warning when submit bio:
>>
>> WARNING: CPU: 0 PID: 382 at fs/iomap/apply.c:54
>> Call trace:
>> iomap_apply+0x644/0x6e0
>> __iomap_dio_rw+0x5cc/0xa24
>> iomap_dio_rw+0x4c/0xcc
>> ext4_dio_read_iter
>> ext4_file_read_iter
>> ext4_file_read_iter+0x318/0x39c
>> call_read_iter
>> lo_rw_aio.isra.0+0x748/0x75c
>> do_req_filebacked+0x2d4/0x370
>> loop_handle_cmd
>> loop_queue_work+0x94/0x23c
>> kthread_worker_fn+0x160/0x6bc
>> loop_kthread_worker_fn+0x3c/0x50
>> kthread+0x20c/0x25c
>> ret_from_fork+0x10/0x18
>>
>> Stack:
>>
>> submit_bio_noacct
>> submit_bio_checks
>> blk_partition_remap
>> bio->bi_iter.bi_sector += p->start_sect
>> // bio->bi_iter.bi_sector = 0xffc0000000000000 + 65408
>> ..
>> loop_queue_work
>> loop_handle_cmd
>> do_req_filebacked
>> pos = ((loff_t) blk_rq_pos(rq) << 9) + lo->lo_offset // pos < 0
>> lo_rw_aio
>> call_read_iter
>> ext4_dio_read_iter
>> __iomap_dio_rw
>> iomap_apply
>> ext4_iomap_begin
>> map.m_lblk = offset >> blkbits
>> ext4_set_iomap
>> iomap->offset = (u64) map->m_lblk << blkbits
>> // iomap->offset = 64512
>> WARN_ON(iomap.offset > pos) // iomap.offset = 64512 and pos < 0
This is wrong, and please update above stack, it seems they're not based
on latest kernel.
Thanks,
Kuai
>>
>> This is unreasonable for start + length > disk->part0.nr_sects. There is
>> already a similar check in blk_add_partition().
>> Fix it by adding a check in bdev_add_partition().
>
> The checking that you add is blkpg_do_ioctl().
>
>>
>> Reported-by: Zhihao Cheng <[email protected]>
>> Signed-off-by: Zhong Jinghua <[email protected]>
>> ---
>> block/ioctl.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index 9c5f637ff153..3223ea862523 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -33,9 +33,16 @@ static int blkpg_do_ioctl(struct block_device *bdev,
>> if (op == BLKPG_DEL_PARTITION)
>> return bdev_del_partition(disk, p.pno);
>> + if (p.start < 0 || p.length <= 0 || p.start + p.length < 0)
>> + return -EINVAL;
>> +
>> start = p.start >> SECTOR_SHIFT;
>> length = p.length >> SECTOR_SHIFT;
>> + /* length may be equal to 0 after right shift */
>> + if (!length || start + length > get_capacity(bdev->bd_disk))
>> + return -EINVAL;
>> +
>> switch (op) {
>> case BLKPG_ADD_PARTITION:
>> /* check if partition is aligned to blocksize */
>>
>
> The change itself looks good to me, feel free to add:
>
> Reviewed-by: Yu Kuai <[email protected]>
>
> .
>
On Mon, May 22, 2023 at 03:06:15PM +0800, Zhong Jinghua wrote:
> + if (p.start < 0 || p.length <= 0 || p.start + p.length < 0)
> + return -EINVAL;
> +
> start = p.start >> SECTOR_SHIFT;
> length = p.length >> SECTOR_SHIFT;
>
> + /* length may be equal to 0 after right shift */
> + if (!length || start + length > get_capacity(bdev->bd_disk))
> + return -EINVAL;
While we're at it, shouldn't these be switched to use
check_add_overflow?
在 2023/5/25 16:55, Christoph Hellwig 写道:
> On Mon, May 22, 2023 at 03:06:15PM +0800, Zhong Jinghua wrote:
>> + if (p.start < 0 || p.length <= 0 || p.start + p.length < 0)
>> + return -EINVAL;
>> +
>> start = p.start >> SECTOR_SHIFT;
>> length = p.length >> SECTOR_SHIFT;
>>
>> + /* length may be equal to 0 after right shift */
>> + if (!length || start + length > get_capacity(bdev->bd_disk))
>> + return -EINVAL;
> While we're at it, shouldn't these be switched to use
> check_add_overflow?
p.start + p.length < 0 can use check_add_overflow instead.
在 2023/5/25 16:55, Christoph Hellwig 写道:
> On Mon, May 22, 2023 at 03:06:15PM +0800, Zhong Jinghua wrote:
>> + if (p.start < 0 || p.length <= 0 || p.start + p.length < 0)
>> + return -EINVAL;
>> +
>> start = p.start >> SECTOR_SHIFT;
>> length = p.length >> SECTOR_SHIFT;
>>
>> + /* length may be equal to 0 after right shift */
>> + if (!length || start + length > get_capacity(bdev->bd_disk))
>> + return -EINVAL;
> While we're at it, shouldn't these be switched to use
> check_add_overflow?
However, using check_add_overflow requires the introduction of an
additional local variable for the third parameter, which does not make
much difference to the current check. Is it worth it?
e.g:
diff --git a/block/ioctl.c b/block/ioctl.c
index 3223ea862523..9a40e8f864cb 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -18,7 +18,7 @@ static int blkpg_do_ioctl(struct block_device *bdev,
{
struct gendisk *disk = bdev->bd_disk;
struct blkpg_partition p;
- long long start, length;
+ long long start, length, tmp_check;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
@@ -33,7 +33,7 @@ static int blkpg_do_ioctl(struct block_device *bdev,
if (op == BLKPG_DEL_PARTITION)
return bdev_del_partition(disk, p.pno);
- if (p.start < 0 || p.length <= 0 || p.start + p.length < 0)
+ if (p.start < 0 || p.length <= 0 || check_add_overflow(p.start,
p.length, &tmp_check))
return -EINVAL;
start = p.start >> SECTOR_SHIFT;
Or do you have a better idea?