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

[Yu Kuai]

Hi,

在 2023/05/25 10:14, Yu Kuai 写道:
> Hi,
>
> 在 2023/05/22 15:06, Zhong Jinghua 写道:
> > From: Zhong Jinghua <zhongjinghua@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Zhong Jinghua <zhongjinghua@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
>
> .