diff mbox series

[-next] block: Fix the partition start may overflow in add_partition()

Message ID 20230522070615.1485014-1-zhongjinghua@huaweicloud.com (mailing list archive)
State New, archived
Headers show
Series [-next] block: Fix the partition start may overflow in add_partition() | expand

Commit Message

zhongjinghua May 22, 2023, 7:06 a.m. UTC 
From: Zhong Jinghua <zhongjinghua@huawei.com>

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 <chengzhihao1@huawei.com>
Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com>
---
 block/ioctl.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Yu Kuai May 25, 2023, 2:14 a.m. UTC | #1 
Hi,

在 2023/05/22 15:06, Zhong Jinghua 写道:
> From: Zhong Jinghua <zhongjinghua@huawei.com>
> 
> 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 <chengzhihao1@huawei.com>
> Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com>
> ---
>   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 <yukuai3@huawei.com>
Yu Kuai May 25, 2023, 2:27 a.m. UTC | #2 
Hi,

在 2023/05/25 10:14, Yu Kuai 写道:
> Hi,
> 
> 在 2023/05/22 15:06, Zhong Jinghua 写道:
>> From: Zhong Jinghua <zhongjinghua@huawei.com>
>>
>> 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 <chengzhihao1@huawei.com>
>> Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com>
>> ---
>>   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 <yukuai3@huawei.com>
> 
> .
>
Christoph Hellwig May 25, 2023, 8:55 a.m. UTC | #3 
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?
zhongjinghua May 25, 2023, 1:20 p.m. UTC | #4 
在 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.
zhongjinghua May 25, 2023, 2:12 p.m. UTC | #5 
在 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?
diff mbox series

Patch

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 */