| Message ID | 20211004224224.4137992-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Assorted improvements to usercopy | expand |
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> -Kees > + return; > + } > + > page = virt_to_head_page(ptr); > > if (PageSlab(page)) { > -- > 2.32.0 >
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?
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.
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); + return; + } + page = virt_to_head_page(ptr); if (PageSlab(page)) {
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(+)