Re: [dm-devel] [PATCH] fix an integer overflow in __blkdev_sectors_to_bio_pages
On 8/15/17 09:01, Mikulas Patocka wrote:
>
>
> On Mon, 14 Aug 2017, Damien Le Moal wrote:
>
>> On Sun, 2017-08-13 at 22:47 -0400, Mikulas Patocka wrote:
>>>
>>> On Wed, 9 Aug 2017, h...@lst.de wrote:
>>>
Does commit 615d22a51c04856efe62af6e1d5b450aaf5cc2c0
"block: Fix __blkdev_issue_zeroout loop" fix the issue for you?
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
>>>
>>> I think that patch is incorrect. sector_t may be a 32-bit type and
>>> nr_sects << 9 may overflow.
>>>
>>> static unsigned int __blkdev_sectors_to_bio_pages(sector_t nr_sects)
>>> {
>>>sector_t bytes = (nr_sects << 9) + PAGE_SIZE - 1;
>>>
>>>return min(bytes >> PAGE_SHIFT, (sector_t)BIO_MAX_PAGES);
>>> }
>>>
>>> Mikulas
>>
>> Mikulas,
>>
>> Does the follwing patch fix the problem ?
>>
>> From 947b3cf41e759b2b23f684e215e651d0c8037f88 Mon Sep 17 00:00:00 2001
>> From: Damien Le Moal
>> Date: Mon, 14 Aug 2017 13:01:16 +0900
>> Subject: [PATCH] block: Fix __blkdev_sectors_to_bio_pages()
>>
>> On 32bit systems where sector_t is a 32bits type, the calculation of
>> bytes may overflow. Use the u64 type for the local calculation to avoid
>> overflows.
>>
>> Signed-off-by: Damien Le Moal
>> ---
>> block/blk-lib.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-lib.c b/block/blk-lib.c
>> index 3fe0aec90597..ccf22dba21f0 100644
>> --- a/block/blk-lib.c
>> +++ b/block/blk-lib.c
>> @@ -269,9 +269,9 @@ static int __blkdev_issue_write_zeroes(struct
>> block_device
>> *bdev,
>> */
>> static unsigned int __blkdev_sectors_to_bio_pages(sector_t nr_sects)
>> {
>> -sector_t bytes = (nr_sects << 9) + PAGE_SIZE - 1;
>> +u64 bytes = ((u64)nr_sects << 9) + PAGE_SIZE - 1;
>>
>> -return min(bytes >> PAGE_SHIFT, (sector_t)BIO_MAX_PAGES);
>> +return min(bytes >> PAGE_SHIFT, (u64)BIO_MAX_PAGES);
>> }
>>
>
> It's OK, but it is not needed to use 64-bit arithmetic here if all we need
> is to shift the value right. Here I submit a simplified patch, using the
> macro DIV_ROUND_UP_SECTOR_T (the macro gets optimized to just an addition
> and right shift).
>
>
>
> From: Mikulas Patocka
>
> Fix possible integer overflow in __blkdev_sectors_to_bio_pages if sector_t
> is 32-bit.
>
> Signed-off-by: Mikulas Patocka
> Fixes: 615d22a51c04 ("block: Fix __blkdev_issue_zeroout loop")
>
> ---
> block/blk-lib.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/block/blk-lib.c
> ===
> --- linux-2.6.orig/block/blk-lib.c
> +++ linux-2.6/block/blk-lib.c
> @@ -269,9 +269,9 @@ static int __blkdev_issue_write_zeroes(s
> */
> static unsigned int __blkdev_sectors_to_bio_pages(sector_t nr_sects)
> {
> - sector_t bytes = (nr_sects << 9) + PAGE_SIZE - 1;
> + sector_t pages = DIV_ROUND_UP_SECTOR_T(nr_sects, PAGE_SIZE / 512);
>
> - return min(bytes >> PAGE_SHIFT, (sector_t)BIO_MAX_PAGES);
> + return min(pages, (sector_t)BIO_MAX_PAGES);
> }
>
> /**
>
Nice ! Thank you.
Reviewed-by: Damien Le Moal
--
Damien Le Moal,
Western Digital
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] fix an integer overflow in __blkdev_sectors_to_bio_pages
On Mon, 14 Aug 2017, Damien Le Moal wrote:
> On Sun, 2017-08-13 at 22:47 -0400, Mikulas Patocka wrote:
> >
> > On Wed, 9 Aug 2017, h...@lst.de wrote:
> >
> > > Does commit 615d22a51c04856efe62af6e1d5b450aaf5cc2c0
> > > "block: Fix __blkdev_issue_zeroout loop" fix the issue for you?
> > >
> > > --
> > > dm-devel mailing list
> > > dm-devel@redhat.com
> > > https://www.redhat.com/mailman/listinfo/dm-devel
> >
> > I think that patch is incorrect. sector_t may be a 32-bit type and
> > nr_sects << 9 may overflow.
> >
> > static unsigned int __blkdev_sectors_to_bio_pages(sector_t nr_sects)
> > {
> >sector_t bytes = (nr_sects << 9) + PAGE_SIZE - 1;
> >
> >return min(bytes >> PAGE_SHIFT, (sector_t)BIO_MAX_PAGES);
> > }
> >
> > Mikulas
>
> Mikulas,
>
> Does the follwing patch fix the problem ?
>
> From 947b3cf41e759b2b23f684e215e651d0c8037f88 Mon Sep 17 00:00:00 2001
> From: Damien Le Moal
> Date: Mon, 14 Aug 2017 13:01:16 +0900
> Subject: [PATCH] block: Fix __blkdev_sectors_to_bio_pages()
>
> On 32bit systems where sector_t is a 32bits type, the calculation of
> bytes may overflow. Use the u64 type for the local calculation to avoid
> overflows.
>
> Signed-off-by: Damien Le Moal
> ---
> block/blk-lib.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 3fe0aec90597..ccf22dba21f0 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -269,9 +269,9 @@ static int __blkdev_issue_write_zeroes(struct block_device
> *bdev,
> */
> static unsigned int __blkdev_sectors_to_bio_pages(sector_t nr_sects)
> {
> - sector_t bytes = (nr_sects << 9) + PAGE_SIZE - 1;
> + u64 bytes = ((u64)nr_sects << 9) + PAGE_SIZE - 1;
>
> - return min(bytes >> PAGE_SHIFT, (sector_t)BIO_MAX_PAGES);
> + return min(bytes >> PAGE_SHIFT, (u64)BIO_MAX_PAGES);
> }
>
It's OK, but it is not needed to use 64-bit arithmetic here if all we need
is to shift the value right. Here I submit a simplified patch, using the
macro DIV_ROUND_UP_SECTOR_T (the macro gets optimized to just an addition
and right shift).
From: Mikulas Patocka
Fix possible integer overflow in __blkdev_sectors_to_bio_pages if sector_t
is 32-bit.
Signed-off-by: Mikulas Patocka
Fixes: 615d22a51c04 ("block: Fix __blkdev_issue_zeroout loop")
---
block/blk-lib.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux-2.6/block/blk-lib.c
===
--- linux-2.6.orig/block/blk-lib.c
+++ linux-2.6/block/blk-lib.c
@@ -269,9 +269,9 @@ static int __blkdev_issue_write_zeroes(s
*/
static unsigned int __blkdev_sectors_to_bio_pages(sector_t nr_sects)
{
- sector_t bytes = (nr_sects << 9) + PAGE_SIZE - 1;
+ sector_t pages = DIV_ROUND_UP_SECTOR_T(nr_sects, PAGE_SIZE / 512);
- return min(bytes >> PAGE_SHIFT, (sector_t)BIO_MAX_PAGES);
+ return min(pages, (sector_t)BIO_MAX_PAGES);
}
/**
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
