Re: [PATCH v5 1/4] btrfs: extent_io: do extra check for extent buffer read write functions

Qu Wenruo
2020-08-20T23:40:07Z

[-- Attachment #1.1: Type: text/plain, Size: 2221 bytes --]

On 2020/8/20 下午10:46, David Sterba wrote:
> On Thu, Aug 20, 2020 at 05:58:53PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/8/20 下午5:50, David Sterba wrote:
>>> On Thu, Aug 20, 2020 at 07:14:13AM +0800, Qu Wenruo wrote:
>>>>>> +static inline int check_eb_range(const struct extent_buffer *eb,
>>>>>> + unsigned long start, unsigned long len)
>>>>>> +{
>>>>>> + /* start, start + len should not go beyond eb->len nor overflow */
>>>>>> + if (unlikely(start > eb->len || start + len > eb->len ||
>>>>>> + len > eb->len)) {
>>>>>
>>>>> Can the number of condition be reduced? If 'start + len' overflows, then
>>>>> we don't need to check 'start > eb->len', and for the case where
>>>>> start = 1024 and len = -1024 the 'len > eb-len' would be enough.
>>>>
>>>> I'm afraid not.
>>>> Although 'start > eb->len || len > eb->len' is enough to detect overflow
>>>> case, it no longer detects cases like 'start = 2k, len = 3k' while
>>>> eb->len == 4K case.
>>>>
>>>> So we still need all 3 checks.
>>>
>>> I was suggesting 'start + len > eb->len', not 'start > eb-len'.
>>>
>>> "start > eb->len" is implied by "start + len > eb->len".
>>
>> start > eb->len is not implied if (start + len) over flows.
>>
>> E.g. start = -2K, len = 2k, eb->len = 4K. We can still pass !(start +
>> len > eb->len || len > eb->len).
>>
>> In short, if we want overflow check along with each one checked, we
>> really need 3 checks.
>
> So what if we add overflow check, that would catch negative start or
> negative length, and then do start + len > eb->len?
>
> The check_setget_bounds is different becasue the len is known at compile
> time so the overflows can't happen in the same way as for the eb range,
> so this this confused me first.
>
> check_add_overflow(start, len) || start + len > eb->len
>
Then it should be more or less the same as the existing 3 checks.

In fact, the 3 checks are just the overflow safe check for (start + len
> eb->len).

The difference between check_setget_bounds() and check_eb_range() is,
the size for check_setget_bounds() are fixed size, thus it will never be
negative, thus it can skip the (size > eb->len) check.

Thanks,
Qu

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]