btrfs: fix extent buffer read/write range checks
diff mbox series

Message ID 20190726052724.12338-1-naohiro.aota@wdc.com
State New
Headers show
Series
  • btrfs: fix extent buffer read/write range checks
Related show

Commit Message

Naohiro Aota July 26, 2019, 5:27 a.m. UTC
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>
---
 fs/btrfs/extent_io.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Nikolay Borisov July 26, 2019, 5:38 a.m. UTC | #1
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.


> ---
>  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);
>  
>
Naohiro Aota July 26, 2019, 6:13 a.m. UTC | #2
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"?

>
>
>> ---
>>  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);
>>
>>
Qu Wenruo July 26, 2019, 6:36 a.m. UTC | #3
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);
>>>
>>>
Naohiro Aota July 26, 2019, 8:15 a.m. UTC | #4
On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote:
>
>
>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.

Correct.

But then, we can even say "start > eb->len" is valid if len == 0?

> Or should we also warn such bad practice?

Maybe...

Or how about let the callers bailing out by e.g. "if (!len) return 1;"
in the check function?

Regards,
Naohiro
Qu Wenruo July 26, 2019, 8:26 a.m. UTC | #5
On 2019/7/26 下午4:15, Naohiro Aota wrote:
> On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote:
>>
>>
>> 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.
>
> Correct.
>
> But then, we can even say "start > eb->len" is valid if len == 0?
>
>> Or should we also warn such bad practice?
>
> Maybe...
>
> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
> in the check function?

Well, let's forgot len == 0 case and make start >= eb->len invalid.

That len == 0 is making a lot of invalid use case valid, and making the
check more complex.

Thanks,
Qu
>
> Regards,
> Naohiro
Qu Wenruo July 29, 2019, 5:07 a.m. UTC | #6
On 2019/7/26 下午4:15, Naohiro Aota wrote:
> On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote:
>>
>>
>> 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.
>
> Correct.
>
> But then, we can even say "start > eb->len" is valid if len == 0?

Tried the "start >= eb->len" check in the centralized check_eb_range(),
and unfortunately it triggers a lot of warnings.

Some callers in fact pass start == eb->len and len == 0:
memmove_extent_buffer() in btrfs_del_items()
copy_extent_buffer() in __push_leaf_*()

Since the check of "start > eb->len || len > eb->len || start + len >
eb->len)" has already ensured we won't access anything beyond the eb
data, I'd prefer to let the start == eb->len && len == 0 case to pass.

Doing the extra len == 0 check in those callers seems a little
over-reacted IMHO.

Thanks,
Qu
>
>> Or should we also warn such bad practice?
>
> Maybe...
>
> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
> in the check function?
>
> Regards,
> Naohiro
Nikolay Borisov July 29, 2019, 6:46 a.m. UTC | #7
<snip>

>>
>> But then, we can even say "start > eb->len" is valid if len == 0?
> 
> Tried the "start >= eb->len" check in the centralized check_eb_range(),
> and unfortunately it triggers a lot of warnings.
> 
> Some callers in fact pass start == eb->len and len == 0:

Isn't this a noop?

> memmove_extent_buffer() in btrfs_del_items()
> copy_extent_buffer() in __push_leaf_*()
> 
> Since the check of "start > eb->len || len > eb->len || start + len >
> eb->len)" has already ensured we won't access anything beyond the eb
> data, I'd prefer to let the start == eb->len && len == 0 case to pass.

In an ideal world shouldn't callers detect their parameters are going to
be a NOOP and never execute the code in the first place? E.g. is it
posible that the math in btrfs_del_item is broken for some edge
condition hence calling those functions with such parameters?

> 
> Doing the extra len == 0 check in those callers seems a little
> over-reacted IMHO.
> 
> Thanks,
> Qu
>>
>>> Or should we also warn such bad practice?
>>
>> Maybe...
>>
>> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
>> in the check function?
>>
>> Regards,
>> Naohiro
>
Qu Wenruo July 29, 2019, 6:54 a.m. UTC | #8
On 2019/7/29 下午2:46, Nikolay Borisov wrote:
> <snip>
>
>>>
>>> But then, we can even say "start > eb->len" is valid if len == 0?
>>
>> Tried the "start >= eb->len" check in the centralized check_eb_range(),
>> and unfortunately it triggers a lot of warnings.
>>
>> Some callers in fact pass start == eb->len and len == 0:
>
> Isn't this a noop?

Yep.

>
>> memmove_extent_buffer() in btrfs_del_items()
>> copy_extent_buffer() in __push_leaf_*()
>>
>> Since the check of "start > eb->len || len > eb->len || start + len >
>> eb->len)" has already ensured we won't access anything beyond the eb
>> data, I'd prefer to let the start == eb->len && len == 0 case to pass.
>
> In an ideal world shouldn't callers detect their parameters are going to
> be a NOOP and never execute the code in the first place? E.g. is it
> posible that the math in btrfs_del_item is broken for some edge
> condition hence calling those functions with such parameters?

This depends.
Sometimes we can save unnecessary (len == 0) check depending on how the
loop is written.

In btrfs, leaf item 0 always ends at eb->len, thus I believe it's the
reason why we have some loop generating (start = eb->len len = 0) request.

As long as we're not accessing any range beyond [0, eb->len), I tend not
to touch all these callers.

Thanks,
Qu

>
>>
>> Doing the extra len == 0 check in those callers seems a little
>> over-reacted IMHO.
>>
>> Thanks,
>> Qu
>>>
>>>> Or should we also warn such bad practice?
>>>
>>> Maybe...
>>>
>>> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
>>> in the check function?
>>>
>>> Regards,
>>> Naohiro
>>

Patch
diff mbox series

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);