Re: [PATCH 1/5] btrfs: extent_io: Do extra check for extent buffer read write functions

[Nikolay Borisov <nborisov@suse.com>]

On 25.07.19 г. 1:54 ч., Qu Wenruo wrote:
> 
> 
> On 2019/7/25 上午12:00, David Sterba wrote:
>> On Wed, Jul 10, 2019 at 10:58:40AM +0000, WenRuo Qu wrote:
>>>
>>>
>>> On 2019/7/10 下午6:42, Nikolay Borisov wrote:
>>>>
>>>>
>>> [...]
>>>>>
>>>>> +/*
>>>>> + * Check if the [start, start + len) range is valid before reading/writing
>>>>> + * the eb.
>>>>> + *
>>>>> + * Caller should not touch the dst/src memory if this function returns error.
>>>>> + */
>>>>> +static int check_eb_range(const struct extent_buffer *eb, unsigned long start,
>>>>> +			  unsigned long len)
>>>>> +{
>>>>> +	unsigned long end;
>>>>> +
>>>>> +	/* start, start + len should not go beyond eb->len nor overflow */
>>>>> +	if (unlikely(start > eb->len || start + len > eb->len ||
>>>>
>>>> I think your check here is wrong, it should be start + len > start +
>>>> eb->len. start is the logical address hence it can be a lot bigger than
>>>> the size of the eb which is 16k by default.
>>>
>>> Definitely NO.
>>>
>>> [start, start + len) must be in the range of [0, nodesize).
>>> So think again.
>>
>> 'start' is the logical address, that's always larger than eb->len (16K),
>> Nikolay is IMO right, the check
> 
> No. This @start is not eb->start. It's the offset inside the eb.

Given David is the 2nd person who got confused I think it's clear that
the variable inside check_eb_range is poorly named ... For the next
version rename it to something like offset or eb_offset.

<snip>

>