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

[Matthew Wilcox <willy@infradead.org>]

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:

        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?