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

[David Sterba <dsterba@suse.cz>]

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