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