Re: [bug report] null_blk: return fixed zoned reads > write pointer

[Jens Axboe <axboe@kernel.dk>]

On 10/23/19 7:06 AM, Dan Carpenter wrote:
> Hello Ajay Joshi,
> 
> The patch dd85b4922de1: "null_blk: return fixed zoned reads > write
> pointer" from Oct 17, 2019, leads to the following static checker
> warning:
> 
> 	drivers/block/null_blk_zoned.c:91 null_zone_valid_read_len()
> 	warn: uncapped user index 'dev->zones[null_zone_no(dev, sector)]'
> 
> drivers/block/null_blk_zoned.c
>      87  size_t null_zone_valid_read_len(struct nullb *nullb,
>      88                                  sector_t sector, unsigned int len)
>      89  {
>      90          struct nullb_device *dev = nullb->dev;
>      91          struct blk_zone *zone = &dev->zones[null_zone_no(dev, sector)];
>                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>      92          unsigned int nr_sectors = len >> SECTOR_SHIFT;
>      93
>      94          /* Read must be below the write pointer position */
>      95          if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL ||
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>      96              sector + nr_sectors <= zone->wp)
>      97                  return len;
>      98
>      99          if (sector > zone->wp)
>                      ^^^^^^^^^^^^^^^^^
> 
> Smatch complains about "sector" being from the untrusted all the time
> and I kind of just ignore it these days.  But here it looks like we're
> checking "sector" after we already used it so that seems very suspicious.
> It feels like "sector > zone->wp" should come at the very start of the
> function.
> 
>     100                  return 0;
>     101
>     102          return (zone->wp - sector) << SECTOR_SHIFT;
>     103  }
> 

Ajay, please take a look at this and send in a patch if appropriate.

-- 
Jens Axboe