[Bug Report] block: integer overflow in blk_ioctl_discard

[Bug Report] block: integer overflow in blk_ioctl_discard

[Changming Liu]

This email was sent because the previous one was rejected due to it was in html form.

From: Changming Liu 
Sent: Friday, March 6, 2020 3:59 PM
To: axboe@kernel.dk
Cc: linux-block@vger.kernel.org; yaohway@gmail.com
Subject: [Bug Report] block: integer overflow in blk_ioctl_discard

Hi Jens,
Greetings, I'm a first-year PhD student who is interested in the usage of UBSan in linux kernel. With some experiments, I found that in 
/block/ioctl.c function blk_ioctl_discard. 

Two uint64 integers, namely, start and len, are directly from user space, so the sum of these two can overflow and wrap around. As a consequence, the check of the sum against function i_size_read at 
if (start + len > i_size_read(bdev->bd_inode))
can be skipped due to the unsigned wrap around, the overflown sum is passed to the 3rd parameter of function truncate_inode_pages_range, which might cause undesired issue. This still exists in the latest version, i.e. linux-5.5.8.

It's well worth noting that, a very similar pattern can be witnessed in function blk_ioctl_zeroout where there are also two uint64 variables with the same name from user space, and the sum of the two variables are passed to function truncate_inode_pages_range too. However in this case, the wrap around is check at line 262, thus the value passed to truncate_inode_pages_range cannot overflow.

So it looks like the issue in blk_ioctl_zeroout was discussed and fixed in 
http://lkml.iu.edu/hypermail/linux/kernel/1511.1/04403.html 
But since in blk_ioctl_discard has the same issue, I wonder if it's worth fixing the issue in blk_ioctl_discard as well. If not, I would appreciate it if I can know the reason, this can help me understand the system a lot.

I cc my colleague on the experiment here to keep him updated.

It's a great honor to reach out to you hardcore linux kernel developer, you guys have been the hero ever since I started learning CS. Looking forward to your valuable response!

Have a good day!

Best regards,
Changming Liu

Re: [Bug Report] block: integer overflow in blk_ioctl_discard

[Jens Axboe]

On 3/7/20 8:52 PM, Changming Liu wrote:
> Hi Jens,
> Greetings, I'm a first-year PhD student who is interested in the usage
> of UBSan in linux kernel. With some experiments, I found that in
> /block/ioctl.c function blk_ioctl_discard. 
> 
> Two uint64 integers, namely, start and len, are directly from user
> space, so the sum of these two can overflow and wrap around. As a
> consequence, the check of the sum against function i_size_read at if
> (start + len > i_size_read(bdev->bd_inode)) can be skipped due to the
> unsigned wrap around, the overflown sum is passed to the 3rd parameter
> of function truncate_inode_pages_range, which might cause undesired
> issue. This still exists in the latest version, i.e. linux-5.5.8.
> 
> It's well worth noting that, a very similar pattern can be witnessed
> in function blk_ioctl_zeroout where there are also two uint64
> variables with the same name from user space, and the sum of the two
> variables are passed to function truncate_inode_pages_range too.
> However in this case, the wrap around is check at line 262, thus the
> value passed to truncate_inode_pages_range cannot overflow.
> 
> So it looks like the issue in blk_ioctl_zeroout was discussed and
> fixed in http://lkml.iu.edu/hypermail/linux/kernel/1511.1/04403.html
> But since in blk_ioctl_discard has the same issue, I wonder if it's
> worth fixing the issue in blk_ioctl_discard as well. If not, I would
> appreciate it if I can know the reason, this can help me understand
> the system a lot.
> 
> I cc my colleague on the experiment here to keep him updated.
> 
> It's a great honor to reach out to you hardcore linux kernel
> developer, you guys have been the hero ever since I started learning
> CS. Looking forward to your valuable response!

Looks like your analysis is correct, care to send a patch to fix it up?

-- 
Jens Axboe

[Bug Report] block: integer overflow in blk_ioctl_discard

[Changming Liu]

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Thursday, March 12, 2020 9:47 AM
> To: Changming Liu <liu.changm@northeastern.edu>; linux-
> block@vger.kernel.org
> Subject: Re: [Bug Report] block: integer overflow in blk_ioctl_discard
> 
> On 3/7/20 8:52 PM, Changming Liu wrote:
> > Hi Jens,
> > Greetings, I'm a first-year PhD student who is interested in the usage
> > of UBSan in linux kernel. With some experiments, I found that in
> > /block/ioctl.c function blk_ioctl_discard.
> >
> > Two uint64 integers, namely, start and len, are directly from user
> > space, so the sum of these two can overflow and wrap around. As a
> > consequence, the check of the sum against function i_size_read at if
> > (start + len > i_size_read(bdev->bd_inode)) can be skipped due to the
> > unsigned wrap around, the overflown sum is passed to the 3rd parameter
> > of function truncate_inode_pages_range, which might cause undesired
> > issue. This still exists in the latest version, i.e. linux-5.5.8.
> >
> > It's well worth noting that, a very similar pattern can be witnessed
> > in function blk_ioctl_zeroout where there are also two uint64
> > variables with the same name from user space, and the sum of the two
> > variables are passed to function truncate_inode_pages_range too.
> > However in this case, the wrap around is check at line 262, thus the
> > value passed to truncate_inode_pages_range cannot overflow.
> >
> > So it looks like the issue in blk_ioctl_zeroout was discussed and
> > fixed in
> > https://nam05.safelinks.protection.outlook.com/?url=http%3A%2F%2Flkml.
> >
> iu.edu%2Fhypermail%2Flinux%2Fkernel%2F1511.1%2F04403.html&amp;data
> =02%
> >
> 7C01%7Cliu.changm%40northeastern.edu%7C79772df8af8e468f43c508d7c68
> bcfd
> >
> b%7Ca8eec281aaa34daeac9b9a398b9215e7%7C0%7C0%7C63719617610736
> 6577&amp;
> >
> sdata=qk7mtvt5LJCmHEbz94YorxDpJ78zLiBkwPXra2qbURE%3D&amp;reserved
> =0
> > But since in blk_ioctl_discard has the same issue, I wonder if it's
> > worth fixing the issue in blk_ioctl_discard as well. If not, I would
> > appreciate it if I can know the reason, this can help me understand
> > the system a lot.
> >
> > I cc my colleague on the experiment here to keep him updated.
> >
> > It's a great honor to reach out to you hardcore linux kernel
> > developer, you guys have been the hero ever since I started learning
> > CS. Looking forward to your valuable response!
> 
> Looks like your analysis is correct, care to send a patch to fix it up?

Thanks for your recognition! This means a lot to me!
I've sent you a patch that can fix it, mostly in accord with patch 
22dd6d356628bccb1a83e12212ec2934f4444e2c. 
Feel free to modify as you want.

Best,
-- Changming Liu
> 
> --
> Jens Axboe