Is there any need to similarly sanitize “size” to assure start + size doesn’t go past “end?”
> On Apr 3, 2020, at 10:33, Peter Zijlstra <[email protected]> wrote:
>
>
> __get_vm_area() is an exported symbol, make sure the callers stay in
> the expected memory range. When calling this function with memory
> ranges outside of the VMALLOC range *bad* things can happen.
>
> Is there any need to similarly sanitize “size” to assure start + size doesn’t go past “end?”
>
Why is that double check needed if all such tests are done deeper on stack?
--
Vlad Rezki
> On Apr 4, 2020, at 12:52 PM, Uladzislau Rezki <[email protected]> wrote:
>
>>
>> Is there any need to similarly sanitize “size” to assure start + size doesn’t go past “end?”
>>
> Why is that double check needed if all such tests are done deeper on stack?
If such tests ARE performed, then it doesn't matter to me whether it is checked before or after,
it just seems that nothing checks whether start + size makes some sort of sense with respect
to end.
I admit I didn't walk through all the routines to see if such a check would be superfluous.
On Sat, Apr 04, 2020 at 11:25:45PM -0600, William Kucharski wrote:
>
>
> > On Apr 4, 2020, at 12:52 PM, Uladzislau Rezki <[email protected]> wrote:
> >
> >>
> >> Is there any need to similarly sanitize “size” to assure start + size doesn’t go past “end?”
> >>
> > Why is that double check needed if all such tests are done deeper on stack?
>
> If such tests ARE performed, then it doesn't matter to me whether it is checked before or after,
> it just seems that nothing checks whether start + size makes some sort of sense with respect
> to end.
>
> I admit I didn't walk through all the routines to see if such a check would be superfluous.
>
Yes, we check it:
<snip>
static __always_inline bool
is_within_this_va(struct vmap_area *va, unsigned long size,
unsigned long align, unsigned long vstart)
{
...
return (nva_start_addr + size <= va->va_end);
}
<snip>
--
Vlad Rezki
On Sun, Apr 05, 2020 at 07:23:15PM +0200, Uladzislau Rezki wrote:
> On Sat, Apr 04, 2020 at 11:25:45PM -0600, William Kucharski wrote:
> >
> >
> > > On Apr 4, 2020, at 12:52 PM, Uladzislau Rezki <[email protected]> wrote:
> > >
> > >>
> > >> Is there any need to similarly sanitize “size” to assure start + size doesn’t go past “end?”
> > >>
> > > Why is that double check needed if all such tests are done deeper on stack?
> >
> > If such tests ARE performed, then it doesn't matter to me whether it is checked before or after,
> > it just seems that nothing checks whether start + size makes some sort of sense with respect
> > to end.
> >
> > I admit I didn't walk through all the routines to see if such a check would be superfluous.
> >
> Yes, we check it:
>
> <snip>
> static __always_inline bool
> is_within_this_va(struct vmap_area *va, unsigned long size,
> unsigned long align, unsigned long vstart)
> {
> ...
> return (nva_start_addr + size <= va->va_end);
> }
> <snip>
>
Sorry, was thinking about one place showed different one. Here we go:
<snip>
/* Check the "vend" restriction. */
if (nva_start_addr + size > vend)
return vend;
<snip>
--
Vlad Rezki
That's fine.
One could still argue size should be sanitized earlier when start and end
are, but it's a nit either way (though size is used before it's checked,
it's not in any way that could fail with bad results.)
-- Bill
> On Apr 5, 2020, at 1:21 PM, Uladzislau Rezki <[email protected]> wrote:
>
> On Sun, Apr 05, 2020 at 07:23:15PM +0200, Uladzislau Rezki wrote:
> Sorry, was thinking about one place showed different one. Here we go:
>
> <snip>
> /* Check the "vend" restriction. */
> if (nva_start_addr + size > vend)
> return vend;
> <snip>
>
> --
> Vlad Rezki
On Sun, Apr 05, 2020 at 02:49:13PM -0600, William Kucharski wrote:
> That's fine.
>
> One could still argue size should be sanitized earlier when start and end
> are, but it's a nit either way (though size is used before it's checked,
> it's not in any way that could fail with bad results.)
>
Agree. Therefore i do not have a strong opinion here, so it
basically depends on how to look at it :)
--
Vlad Rezki