Re: fix an integer overflow in __blkdev_sectors_to_bio_pages

[Mike Snitzer <snitzer@redhat.com>]

On Mon, Aug 14 2017 at  8:01pm -0400,
Mikulas Patocka <mpatocka@redhat.com> 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, hch@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 <damien.lemoal@wdc.com>
> > 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 <damien.lemoal@wdc.com>
> > ---
> >  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 <mpatocka@redhat.com>
> 
> Fix possible integer overflow in __blkdev_sectors_to_bio_pages if sector_t 
> is 32-bit.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 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);
>  }
>  
>  /**
> 

Jens can you pick this up?

Also here:
https://patchwork.kernel.org/patch/9900407/