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

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

[Dan Carpenter]

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  }

regards,
dan carpenter

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

[Jens Axboe]

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

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

[Ajay Joshi]

On 26/10/19 1:55 am, Jens Axboe wrote:
> 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.

The "sector" is used to get the zone information from the array, so the
check "sector > zone->wp", would be possible only after getting the zone
information i.e. after this line struct blk_zone *zone =
&dev->zones[null_zone_no(dev, sector)]. We cannot add this check at the
beginning, but we can do the check just before the condition "sector +
nr_sectors <= zone->wp".The patch would be something like this:

if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
              return len;

/* Read must be below the write pointer position */
if (sector > zone->wp)
              return 0;


if (sector + nr_sectors <= zone->wp)
              return len;

What do you feel?

>>
>>     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.
>

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

[Dan Carpenter]

Argh...  Sorry.  No, I just read the code badly.  Forget about it.

regards,
dan carpenter