* [bug report] null_blk: return fixed zoned reads > write pointer
@ 2019-10-23 13:06 Dan Carpenter
2019-10-25 20:25 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2019-10-23 13:06 UTC (permalink / raw)
To: ajay.joshi; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] null_blk: return fixed zoned reads > write pointer
2019-10-23 13:06 [bug report] null_blk: return fixed zoned reads > write pointer Dan Carpenter
@ 2019-10-25 20:25 ` Jens Axboe
2019-10-28 4:40 ` Ajay Joshi
0 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2019-10-25 20:25 UTC (permalink / raw)
To: Dan Carpenter, ajay.joshi; +Cc: linux-block
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] null_blk: return fixed zoned reads > write pointer
2019-10-25 20:25 ` Jens Axboe
@ 2019-10-28 4:40 ` Ajay Joshi
2019-10-28 6:13 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Ajay Joshi @ 2019-10-28 4:40 UTC (permalink / raw)
To: Jens Axboe, Dan Carpenter; +Cc: linux-block
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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-28 6:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23 13:06 [bug report] null_blk: return fixed zoned reads > write pointer Dan Carpenter
2019-10-25 20:25 ` Jens Axboe
2019-10-28 4:40 ` Ajay Joshi
2019-10-28 6:13 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).