On 14/10/20 6:34 pm, Andrew Price wrote:
> On 13/10/2020 16:26, Anant Thazhemadam wrote:
>> In gfs2_read_sb(), if the condition
>> (d != sdp->sd_heightsize[x - 1] || m)
>> isn't satisfied (in the first 11 iterations), the loop continues,
>> and begins to perform out-of-bounds access.
>> Fix this out-of-bounds access by introducing a condition in the for loop
>> that ensures that no more than GFS2_MAX_META_HEIGHT + 1 elements are
>> accessed.
>>
>> In addition to this, if the above condition is satisfied when
>> x = GFS2_MAX_META_HEIGHT (which = 10), and the flow of control breaks
>> out of the loop, then an out-of-bounds access is performed again while
>> assigning sdp->sd_heightsize[x] = ~0 (since x would be 11 now.), and
>> also the assertion that spd->sd_max_height <= GFS2_MAX_META_HEIGHT would
>> fail.
>> Fix this out-of-bounds access and ensure that the assertion doesn't fail
>> by introducing another variable "index", which keeps track of the last
>> valid value of x (pre-increment) that can be used.
>
> That's not quite the right approach. Your analysis below is correct: the problem stems from the block size in the superblock being zeroed by the fuzzer. So the correct fix would be to add a validation check for sb_bsize (gfs2_check_sb() is lacking somewhat). Valid values are powers of 2 between 512 and the page size.
>
I see. Thanks for the review! I'll send in a v2 that implements this check soon enough.
Thanks,
Anant