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@xxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxx>
>
> 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
