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