Re: [PATCH] btrfs: fix extent buffer read/write range checks

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2019/7/26 下午2:13, Naohiro Aota wrote:
> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote:
>>> Several functions to read/write an extent buffer check if specified
>>> offset
>>> range resides in the size of the extent buffer. However, those checks
>>> have
>>> two problems:
>>>
>>> (1) they don't catch "start == eb->len" case.
>>> (2) it checks offset in extent buffer against logical address using
>>>     eb->start.
>>>
>>> Generally, eb->start is much larger than the offset, so the second
>>> WARN_ON
>>> was almost useless.
>>>
>>> Fix these problems in read_extent_buffer_to_user(),
>>> {memcmp,write,memzero}_extent_buffer().
>>>
>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>
>> Qu already sent similar patch:
>>
>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read
>> write functions
>>
>>
>> He centralised the checking code, your >= fixes though should be merged
>> there.
>
> Oops, I missed that series. Thank you for pointing out. Then, this
> should be merged into Qu's version.
>
> Qu, could you pick the change from "start > eb->len" to "start >= eb->len"?

start >= eb->len is not always invalid.

start == eb->len while len == 0 is still valid.

Or should we also warn such bad practice?

Thanks,
Qu

>
>>
>>
>>> ---
>>>  fs/btrfs/extent_io.c | 16 ++++++++--------
>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index 50cbaf1dad5b..c0174f530568 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct
>>> extent_buffer *eb,
>>>      unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>>      int ret = 0;
>>>
>>> -    WARN_ON(start > eb->len);
>>> -    WARN_ON(start + len > eb->start + eb->len);
>>> +    WARN_ON(start >= eb->len);
>>> +    WARN_ON(start + len > eb->len);
>>>
>>>      offset = offset_in_page(start_offset + start);
>>>
>>> @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct
>>> extent_buffer *eb, const void *ptrv,
>>>      unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>>      int ret = 0;
>>>
>>> -    WARN_ON(start > eb->len);
>>> -    WARN_ON(start + len > eb->start + eb->len);
>>> +    WARN_ON(start >= eb->len);
>>> +    WARN_ON(start + len > eb->len);
>>>
>>>      offset = offset_in_page(start_offset + start);
>>>
>>> @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer
>>> *eb, const void *srcv,
>>>      size_t start_offset = offset_in_page(eb->start);
>>>      unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>>
>>> -    WARN_ON(start > eb->len);
>>> -    WARN_ON(start + len > eb->start + eb->len);
>>> +    WARN_ON(start >= eb->len);
>>> +    WARN_ON(start + len > eb->len);
>>>
>>>      offset = offset_in_page(start_offset + start);
>>>
>>> @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer
>>> *eb, unsigned long start,
>>>      size_t start_offset = offset_in_page(eb->start);
>>>      unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>>
>>> -    WARN_ON(start > eb->len);
>>> -    WARN_ON(start + len > eb->start + eb->len);
>>> +    WARN_ON(start >= eb->len);
>>> +    WARN_ON(start + len > eb->len);
>>>
>>>      offset = offset_in_page(start_offset + start);
>>>
>>>