Re: [PATCH 2/3] mm/usercopy: Detect vmalloc overruns

[Kees Cook <keescook@chromium.org>]

On Wed, Oct 06, 2021 at 02:26:41AM +0100, Matthew Wilcox wrote:
> On Tue, Oct 05, 2021 at 02:25:23PM -0700, Kees Cook wrote:
> > On Mon, Oct 04, 2021 at 11:42:22PM +0100, Matthew Wilcox (Oracle) wrote:
> > > If you have a vmalloc() allocation, or an address from calling vmap(),
> > > you cannot overrun the vm_area which describes it, regardless of the
> > > size of the underlying allocation.  This probably doesn't do much for
> > > security because vmalloc comes with guard pages these days, but it
> > > prevents usercopy aborts when copying to a vmap() of smaller pages.
> > > 
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > ---
> > >  mm/usercopy.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > > index ac95b22fbbce..7bfc4f9ed1e4 100644
> > > --- a/mm/usercopy.c
> > > +++ b/mm/usercopy.c
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/sched/task.h>
> > >  #include <linux/sched/task_stack.h>
> > >  #include <linux/thread_info.h>
> > > +#include <linux/vmalloc.h>
> > >  #include <linux/atomic.h>
> > >  #include <linux/jump_label.h>
> > >  #include <asm/sections.h>
> > > @@ -236,6 +237,14 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
> > >  		return;
> > >  	}
> > >  
> > > +	if (is_vmalloc_addr(ptr)) {
> > > +		struct vm_struct *vm = find_vm_area(ptr);
> > > +
> > > +		if (ptr + n > vm->addr + vm->size)
> > > +			usercopy_abort("vmalloc", NULL, to_user, 0, n);
> > 
> > This "0" is easy to make "ptr - vm->addr". With that fixed:
> > 
> > Acked-by: Kees Cook <keescook@chromium.org>
> 
> Looking at this again, if we do ...
> 
> 	char *p = vmalloc(2 * PAGE_SIZE);
> 	copy_from_user(p + 2 * PAGE_SIZE, ...);
> 
> then 'vm' can be NULL.  I think.  While we can't catch everything, a
> NULL pointer dereference here seems a little unfriendly?  So how about
> this:

Oh right, because ptr will be in a guard page (or otherwise unallocated)
but within the vmalloc range?

> 
>         if (is_vmalloc_addr(ptr)) {
>                 struct vm_struct *vm = find_vm_area(ptr);
>                 unsigned long offset;
> 
>                 if (!vm) {
>                         usercopy_abort("vmalloc", NULL, to_user, 0, n);
>                         return;
>                 }
> 
>                 offset = ptr - vm->addr;
>                 if (offset + n > vm->size)
>                         usercopy_abort("vmalloc", NULL, to_user, offset, n);
>                 return;
>         }
> 
> Do we want to distinguish the two cases somehow?

I'd report the first's "details" as "unmapped" or something:

	usercopy_abort("vmalloc", "unmapped", to_user, 0, n);

and the latter is fine as-is.

-- 
Kees Cook