[PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Peter Zijlstra]

__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.

(I noticed this when I managed to corrupt the kernel text by accident)

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 mm/vmalloc.c |    7 +++++++
 1 file changed, 7 insertions(+)

--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n
 struct vm_struct *__get_vm_area(unsigned long size, unsigned long flags,
 				unsigned long start, unsigned long end)
 {
+	/*
+	 * Ensure callers stay in the vmalloc range.
+	 */
+	if (WARN_ON(start < VMALLOC_START || start > VMALLOC_END ||
+		    end < VMALLOC_START || end > VMALLOC_END))
+		return NULL;
+
 	return __get_vm_area_node(size, 1, flags, start, end, NUMA_NO_NODE,
 				  GFP_KERNEL, __builtin_return_address(0));
 }

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Uladzislau Rezki]

On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> 
> (I noticed this when I managed to corrupt the kernel text by accident)
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  mm/vmalloc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n
>  struct vm_struct *__get_vm_area(unsigned long size, unsigned long flags,
>  				unsigned long start, unsigned long end)
>  {
> +	/*
> +	 * Ensure callers stay in the vmalloc range.
> +	 */
> +	if (WARN_ON(start < VMALLOC_START || start > VMALLOC_END ||
> +		    end < VMALLOC_START || end > VMALLOC_END))
> +		return NULL;
> +
>  	return __get_vm_area_node(size, 1, flags, start, end, NUMA_NO_NODE,
>  				  GFP_KERNEL, __builtin_return_address(0));
>  }
Peter, could you please clarify what kind of issues you had and how you
tested?

__get_vm_area() is not limited by allocating only with vmalloc space,
it can use whole virtual address space/range, i.e. 1 - ULONG_MAX.

Though, i am not sure if there are users(who uses __get_vm_area())
which allocate outside of vmalloc address space.

--
Vlad Rezki

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Peter Zijlstra]

On Fri, Apr 03, 2020 at 08:18:18PM +0200, Uladzislau Rezki wrote:
> On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> > 
> > (I noticed this when I managed to corrupt the kernel text by accident)
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  mm/vmalloc.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n
> >  struct vm_struct *__get_vm_area(unsigned long size, unsigned long flags,
> >  				unsigned long start, unsigned long end)
> >  {
> > +	/*
> > +	 * Ensure callers stay in the vmalloc range.
> > +	 */
> > +	if (WARN_ON(start < VMALLOC_START || start > VMALLOC_END ||
> > +		    end < VMALLOC_START || end > VMALLOC_END))
> > +		return NULL;
> > +
> >  	return __get_vm_area_node(size, 1, flags, start, end, NUMA_NO_NODE,
> >  				  GFP_KERNEL, __builtin_return_address(0));
> >  }
> Peter, could you please clarify what kind of issues you had and how you
> tested?

Well, I had a bug and corrupted text; but then I tested:

	__get_vm_area(PAGE_SIZE, VM_ALLOC, __START_KERNEL_map,
		      __START_KERNEL_map + KERNEL_IMAGE_SIZE);

and that *works*.

> __get_vm_area() is not limited by allocating only with vmalloc space,
> it can use whole virtual address space/range, i.e. 1 - ULONG_MAX.

Yeah, I know, I'm saying it perhaps should be, because not limiting it
while exposing it to modules seems risky at best, downright dangerous if
you consider map_vm_area() is also exported.

And while I know the machinery works for the complete virtual address
space, architectures do set aside explicit VA ranges for specific
purposes, we had better respect that, esp. for modules.

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Uladzislau Rezki]

On Fri, Apr 03, 2020 at 08:53:00PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 08:18:18PM +0200, Uladzislau Rezki wrote:
> > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> > > 
> > > (I noticed this when I managed to corrupt the kernel text by accident)
> > > 
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >  mm/vmalloc.c |    7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n
> > >  struct vm_struct *__get_vm_area(unsigned long size, unsigned long flags,
> > >  				unsigned long start, unsigned long end)
> > >  {
> > > +	/*
> > > +	 * Ensure callers stay in the vmalloc range.
> > > +	 */
> > > +	if (WARN_ON(start < VMALLOC_START || start > VMALLOC_END ||
> > > +		    end < VMALLOC_START || end > VMALLOC_END))
> > > +		return NULL;
> > > +
> > >  	return __get_vm_area_node(size, 1, flags, start, end, NUMA_NO_NODE,
> > >  				  GFP_KERNEL, __builtin_return_address(0));
> > >  }
> > Peter, could you please clarify what kind of issues you had and how you
> > tested?
> 
> Well, I had a bug and corrupted text; but then I tested:
> 
> 	__get_vm_area(PAGE_SIZE, VM_ALLOC, __START_KERNEL_map,
> 		      __START_KERNEL_map + KERNEL_IMAGE_SIZE);
> 
> and that *works*.
> 
Do you mean that you corrupted "text" by calling __get_vm_area(...)
with special parameters? If so could you please show how you used it.

> > __get_vm_area() is not limited by allocating only with vmalloc space,
> > it can use whole virtual address space/range, i.e. 1 - ULONG_MAX.
> 
> Yeah, I know, I'm saying it perhaps should be, because not limiting it
> while exposing it to modules seems risky at best, downright dangerous if
> you consider map_vm_area() is also exported.
> 
Doing it to secure modules, probably is OK, but modules can also be reside
within vmalloc address space.

Thank you in advance!

--
Vlad Rezki

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Christoph Hellwig]

On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> 
> (I noticed this when I managed to corrupt the kernel text by accident)

Maybe it is time to unexport it?  There are only two users:

 - staging/media/ipu3 really should be using vmap.  And given that it
   is a staging driver it really doesn't matter anyway if we break it.
 - pcmcia/electra_cf.c is actually using it for something that is not
   a vmalloc address.  But it is so special that I think prohibiting
   to build it as module seems fine.

> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  mm/vmalloc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n
>  struct vm_struct *__get_vm_area(unsigned long size, unsigned long flags,
>  				unsigned long start, unsigned long end)
>  {
> +	/*
> +	 * Ensure callers stay in the vmalloc range.
> +	 */
> +	if (WARN_ON(start < VMALLOC_START || start > VMALLOC_END ||
> +		    end < VMALLOC_START || end > VMALLOC_END))
> +		return NULL;
> +
>  	return __get_vm_area_node(size, 1, flags, start, end, NUMA_NO_NODE,
>  				  GFP_KERNEL, __builtin_return_address(0));
>  }
---end quoted text---

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Peter Zijlstra]

On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote:
> On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> > 
> > (I noticed this when I managed to corrupt the kernel text by accident)
> 
> Maybe it is time to unexport it?  There are only two users:
> 
>  - staging/media/ipu3 really should be using vmap.  And given that it
>    is a staging driver it really doesn't matter anyway if we break it.
>  - pcmcia/electra_cf.c is actually using it for something that is not
>    a vmalloc address.  But it is so special that I think prohibiting
>    to build it as module seems fine.

I think I just sent you a patch along those lines ;-)

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Sakari Ailus]

Hi Christoph,

On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote:
> On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> > 
> > (I noticed this when I managed to corrupt the kernel text by accident)
> 
> Maybe it is time to unexport it?  There are only two users:
> 
>  - staging/media/ipu3 really should be using vmap.  And given that it
>    is a staging driver it really doesn't matter anyway if we break it.

It's not very polite to suggest breaking other people's drivers for such a
small matter, staging or not. That'd be bound to break kernel compilation
for a lot of people, if for nothing else.

Anyway, thanks for cc'ing me. I agree with suggestion and I'll submit a
patch to address it.

-- 
Regards,

Sakari Ailus

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Peter Zijlstra]

On Fri, Apr 17, 2020 at 03:57:35PM +0300, Sakari Ailus wrote:
> Hi Christoph,
> 
> On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote:
> > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> > > 
> > > (I noticed this when I managed to corrupt the kernel text by accident)
> > 
> > Maybe it is time to unexport it?  There are only two users:
> > 
> >  - staging/media/ipu3 really should be using vmap.  And given that it
> >    is a staging driver it really doesn't matter anyway if we break it.
> 
> It's not very polite to suggest breaking other people's drivers for such a
> small matter, staging or not. That'd be bound to break kernel compilation
> for a lot of people, if for nothing else.
> 
> Anyway, thanks for cc'ing me. I agree with suggestion and I'll submit a
> patch to address it.

Already done, see:

  https://lkml.kernel.org/r/20200414131348.444715-5-hch@lst.de

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Sakari Ailus]

On Fri, Apr 17, 2020 at 03:14:24PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 17, 2020 at 03:57:35PM +0300, Sakari Ailus wrote:
> > Hi Christoph,
> > 
> > On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote:
> > > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra 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.
> > > > 
> > > > (I noticed this when I managed to corrupt the kernel text by accident)
> > > 
> > > Maybe it is time to unexport it?  There are only two users:
> > > 
> > >  - staging/media/ipu3 really should be using vmap.  And given that it
> > >    is a staging driver it really doesn't matter anyway if we break it.
> > 
> > It's not very polite to suggest breaking other people's drivers for such a
> > small matter, staging or not. That'd be bound to break kernel compilation
> > for a lot of people, if for nothing else.
> > 
> > Anyway, thanks for cc'ing me. I agree with suggestion and I'll submit a
> > patch to address it.
> 
> Already done, see:
> 
>   https://lkml.kernel.org/r/20200414131348.444715-5-hch@lst.de

Ah, thanks for pointing this out. Please ignore this then.

-- 
Regards,

Sakari Ailus

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[William Kucharski]

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 <peterz@infradead.org> 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.

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Uladzislau Rezki]

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

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[William Kucharski]

> On Apr 4, 2020, at 12:52 PM, Uladzislau Rezki <urezki@gmail.com> 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.

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Uladzislau Rezki]

On Sat, Apr 04, 2020 at 11:25:45PM -0600, William Kucharski wrote:
> 
> 
> > On Apr 4, 2020, at 12:52 PM, Uladzislau Rezki <urezki@gmail.com> 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

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Uladzislau 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 <urezki@gmail.com> 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

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[William Kucharski]

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 <urezki@gmail.com> 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

Re: [PATCH] mm/vmalloc: Sanitize __get_vm_area() arguments

[Uladzislau 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