[PATCH v3 0/3] Assorted improvements to usercopy

[PATCH v3 0/3] Assorted improvements to usercopy

[Matthew Wilcox (Oracle)]

We must prohibit page boundary crossing for kmap() addresses.
vmap() addresses are limited by the length of the mapping, and
compound pages are limited by the size of the page.

These should probably all have test cases?

v3:
 - Remove a now-unused variable
v2:
 - Prevent a NULL pointer dereference when a vmalloc-range pointer
   doesn't have an associated allocation (me)
 - Report better offsets than "0" (Kees)


Matthew Wilcox (Oracle) (3):
  mm/usercopy: Check kmap addresses properly
  mm/usercopy: Detect vmalloc overruns
  mm/usercopy: Detect compound page overruns

 arch/x86/include/asm/highmem.h   |  1 +
 include/linux/highmem-internal.h | 10 ++++++++
 mm/usercopy.c                    | 43 +++++++++++++++++++++++---------
 3 files changed, 42 insertions(+), 12 deletions(-)

-- 
2.33.0

[PATCH v3 1/3] mm/usercopy: Check kmap addresses properly

[Matthew Wilcox (Oracle)]

If you are copying to an address in the kmap region, you may not copy
across a page boundary, no matter what the size of the underlying
allocation.  You can't kmap() a slab page because slab pages always
come from low memory.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/highmem.h   |  1 +
 include/linux/highmem-internal.h | 10 ++++++++++
 mm/usercopy.c                    | 16 ++++++++++------
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/highmem.h b/arch/x86/include/asm/highmem.h
index 032e020853aa..731ee7cc40a5 100644
--- a/arch/x86/include/asm/highmem.h
+++ b/arch/x86/include/asm/highmem.h
@@ -26,6 +26,7 @@
 #include <asm/tlbflush.h>
 #include <asm/paravirt.h>
 #include <asm/fixmap.h>
+#include <asm/pgtable_areas.h>
 
 /* declarations for highmem.c */
 extern unsigned long highstart_pfn, highend_pfn;
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index 0a0b2b09b1b8..01fb76d101b0 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -149,6 +149,11 @@ static inline void totalhigh_pages_add(long count)
 	atomic_long_add(count, &_totalhigh_pages);
 }
 
+static inline bool is_kmap_addr(const void *x)
+{
+	unsigned long addr = (unsigned long)x;
+	return addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP);
+}
 #else /* CONFIG_HIGHMEM */
 
 static inline struct page *kmap_to_page(void *addr)
@@ -234,6 +239,11 @@ static inline void __kunmap_atomic(void *addr)
 static inline unsigned int nr_free_highpages(void) { return 0; }
 static inline unsigned long totalhigh_pages(void) { return 0UL; }
 
+static inline bool is_kmap_addr(const void *x)
+{
+	return false;
+}
+
 #endif /* CONFIG_HIGHMEM */
 
 /*
diff --git a/mm/usercopy.c b/mm/usercopy.c
index b3de3c4eefba..8c039302465f 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -228,12 +228,16 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 	if (!virt_addr_valid(ptr))
 		return;
 
-	/*
-	 * When CONFIG_HIGHMEM=y, kmap_to_page() will give either the
-	 * highmem page or fallback to virt_to_page(). The following
-	 * is effectively a highmem-aware virt_to_head_page().
-	 */
-	page = compound_head(kmap_to_page((void *)ptr));
+	if (is_kmap_addr(ptr)) {
+		unsigned long page_end = (unsigned long)ptr | (PAGE_SIZE - 1);
+
+		if ((unsigned long)ptr + n - 1 > page_end)
+			usercopy_abort("kmap", NULL, to_user,
+					offset_in_page(ptr), n);
+		return;
+	}
+
+	page = virt_to_head_page(ptr);
 
 	if (PageSlab(page)) {
 		/* Check slab allocator for flags and size. */
-- 
2.33.0

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

[Matthew Wilcox (Oracle)]

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>
Acked-by: Kees Cook <keescook@chromium.org>
---
 mm/usercopy.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index 8c039302465f..63476e1506e0 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>
@@ -237,6 +238,21 @@ 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);
+		unsigned long offset;
+
+		if (!vm) {
+			usercopy_abort("vmalloc", "no area", to_user, 0, n);
+			return;
+		}
+
+		offset = ptr - vm->addr;
+		if (offset + n > vm->size)
+			usercopy_abort("vmalloc", NULL, to_user, offset, n);
+		return;
+	}
+
 	page = virt_to_head_page(ptr);
 
 	if (PageSlab(page)) {
-- 
2.33.0

[PATCH v3 3/3] mm/usercopy: Detect compound page overruns

[Matthew Wilcox (Oracle)]

Move the compound page overrun detection out of
CONFIG_HARDENED_USERCOPY_PAGESPAN so it's enabled for more people.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Kees Cook <keescook@chromium.org>
---
 mm/usercopy.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index 63476e1506e0..db2e8c4f79fd 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -163,7 +163,6 @@ static inline void check_page_span(const void *ptr, unsigned long n,
 {
 #ifdef CONFIG_HARDENED_USERCOPY_PAGESPAN
 	const void *end = ptr + n - 1;
-	struct page *endpage;
 	bool is_reserved, is_cma;
 
 	/*
@@ -194,11 +193,6 @@ static inline void check_page_span(const void *ptr, unsigned long n,
 		   ((unsigned long)end & (unsigned long)PAGE_MASK)))
 		return;
 
-	/* Allow if fully inside the same compound (__GFP_COMP) page. */
-	endpage = virt_to_head_page(end);
-	if (likely(endpage == page))
-		return;
-
 	/*
 	 * Reject if range is entirely either Reserved (i.e. special or
 	 * device memory), or CMA. Otherwise, reject since the object spans
@@ -258,6 +252,11 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 	if (PageSlab(page)) {
 		/* Check slab allocator for flags and size. */
 		__check_heap_object(ptr, n, page, to_user);
+	} else if (PageHead(page)) {
+		/* A compound allocation */
+		unsigned long offset = ptr - page_address(page);
+		if (offset + n > page_size(page))
+			usercopy_abort("page alloc", NULL, to_user, offset, n);
 	} else {
 		/* Verify object does not incorrectly span multiple pages. */
 		check_page_span(ptr, n, page, to_user);
-- 
2.33.0

Re: [PATCH v3 0/3] Assorted improvements to usercopy

[William Kucharski]

I like these, but a quick question:

Since the usercopy_abort() calls are all because the offset exceeds the page
size, is there a reason why you don't specifically state that via the detail
parameter rather than just supply a NULL pointer?

Otherwise for the patch series:

Reviewed-by: William Kucharski <william.kucharski@oracle.com>

> On Dec 13, 2021, at 7:27 AM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> 
> We must prohibit page boundary crossing for kmap() addresses.
> vmap() addresses are limited by the length of the mapping, and
> compound pages are limited by the size of the page.
> 
> These should probably all have test cases?
> 
> v3:
> - Remove a now-unused variable
> v2:
> - Prevent a NULL pointer dereference when a vmalloc-range pointer
>   doesn't have an associated allocation (me)
> - Report better offsets than "0" (Kees)
> 
> 
> Matthew Wilcox (Oracle) (3):
>  mm/usercopy: Check kmap addresses properly
>  mm/usercopy: Detect vmalloc overruns
>  mm/usercopy: Detect compound page overruns
> 
> arch/x86/include/asm/highmem.h   |  1 +
> include/linux/highmem-internal.h | 10 ++++++++
> mm/usercopy.c                    | 43 +++++++++++++++++++++++---------
> 3 files changed, 42 insertions(+), 12 deletions(-)
> 
> -- 
> 2.33.0

Re: [PATCH v3 0/3] Assorted improvements to usercopy

[Matthew Wilcox]

On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote:
> I like these, but a quick question:
> 
> Since the usercopy_abort() calls are all because the offset exceeds the page
> size, is there a reason why you don't specifically state that via the detail
> parameter rather than just supply a NULL pointer?

Hmm ... I'd defer to Kees on this, because I'm not familiar with
usercopy_abort() usage, but the only places which use the detail
parameter today are slab/slub, which use it to pass the name of
the slab.  I think the user is supposed to infer that we overran the
end of the page based on the offset & length values.

> Otherwise for the patch series:
> 
> Reviewed-by: William Kucharski <william.kucharski@oracle.com>

Thanks!

Re: [PATCH v3 0/3] Assorted improvements to usercopy

[Kees Cook]

On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote:
> On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote:
> > I like these, but a quick question:
> > 
> > Since the usercopy_abort() calls are all because the offset exceeds the page
> > size, is there a reason why you don't specifically state that via the detail
> > parameter rather than just supply a NULL pointer?
> 
> Hmm ... I'd defer to Kees on this, because I'm not familiar with
> usercopy_abort() usage, but the only places which use the detail
> parameter today are slab/slub, which use it to pass the name of
> the slab.  I think the user is supposed to infer that we overran the
> end of the page based on the offset & length values.

I agree that leaving it NULL is best here. The "detail" is really about
adding more information about which thing it was, which for slab makes
sense, but most other stuff there isn't really anything to quickly
distinguish one from another (i.e. vmap is all vmap).

-- 
Kees Cook

Re: [PATCH v3 3/3] mm/usercopy: Detect compound page overruns

[Kees Cook]

On Mon, Dec 13, 2021 at 02:27:03PM +0000, Matthew Wilcox (Oracle) wrote:
> Move the compound page overrun detection out of
> CONFIG_HARDENED_USERCOPY_PAGESPAN so it's enabled for more people.

I'd argue that everything else enabled by USERCOPY_PAGESPAN could be
removed now too. Do you want to add a 4th patch to rip that out?

https://github.com/KSPP/linux/issues/163

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v3 0/3] Assorted improvements to usercopy

[William Kucharski]

Thanks, good explanation.

> On Dec 13, 2021, at 1:47 PM, Kees Cook <keescook@chromium.org> wrote:
> 
> On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote:
>> On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote:
>>> I like these, but a quick question:
>>> 
>>> Since the usercopy_abort() calls are all because the offset exceeds the page
>>> size, is there a reason why you don't specifically state that via the detail
>>> parameter rather than just supply a NULL pointer?
>> 
>> Hmm ... I'd defer to Kees on this, because I'm not familiar with
>> usercopy_abort() usage, but the only places which use the detail
>> parameter today are slab/slub, which use it to pass the name of
>> the slab.  I think the user is supposed to infer that we overran the
>> end of the page based on the offset & length values.
> 
> I agree that leaving it NULL is best here. The "detail" is really about
> adding more information about which thing it was, which for slab makes
> sense, but most other stuff there isn't really anything to quickly
> distinguish one from another (i.e. vmap is all vmap).
> 
> -- 
> Kees Cook

Re: [PATCH v3 0/3] Assorted improvements to usercopy

[Matthew Wilcox]

On Mon, Dec 13, 2021 at 12:47:58PM -0800, Kees Cook wrote:
> On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote:
> > On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote:
> > > I like these, but a quick question:
> > > 
> > > Since the usercopy_abort() calls are all because the offset exceeds the page
> > > size, is there a reason why you don't specifically state that via the detail
> > > parameter rather than just supply a NULL pointer?
> > 
> > Hmm ... I'd defer to Kees on this, because I'm not familiar with
> > usercopy_abort() usage, but the only places which use the detail
> > parameter today are slab/slub, which use it to pass the name of
> > the slab.  I think the user is supposed to infer that we overran the
> > end of the page based on the offset & length values.
> 
> I agree that leaving it NULL is best here. The "detail" is really about
> adding more information about which thing it was, which for slab makes
> sense, but most other stuff there isn't really anything to quickly
> distinguish one from another (i.e. vmap is all vmap).

There _is_ a bit more information in the vmap case (not in the kmap
or compound page case).  You can see it in /proc/vmallocinfo.  We
could pass it in like this?

        if (is_vmalloc_addr(ptr)) {
                struct vm_struct *vm = find_vm_area(ptr);
+               char sym[100];
                unsigned long offset;

                if (!vm) {
..
+               if (vm->caller)
+                       snprintf(sym, sizeof(sym), "%pS", vm->caller);
                offset = ptr - vm->addr;
                if (offset + n > vm->size)
-                       usercopy_abort("vmalloc", NULL, to_user, offset, n);
+                       usercopy_abort("vmalloc", vm->caller ? sym : NULL,
+                                       to_user, offset, n);
                return;

Re: [PATCH v3 3/3] mm/usercopy: Detect compound page overruns

[Matthew Wilcox]

On Mon, Dec 13, 2021 at 12:52:22PM -0800, Kees Cook wrote:
> On Mon, Dec 13, 2021 at 02:27:03PM +0000, Matthew Wilcox (Oracle) wrote:
> > Move the compound page overrun detection out of
> > CONFIG_HARDENED_USERCOPY_PAGESPAN so it's enabled for more people.
> 
> I'd argue that everything else enabled by USERCOPY_PAGESPAN could be
> removed now too. Do you want to add a 4th patch to rip that out?
> 
> https://github.com/KSPP/linux/issues/163

I don't mind ... is it your assessment that it's not worth checking for
a copy_to/from_user that spans a boundary between a reserved and
!reserved page, or overlaps the boundary of rodata/bss/data/CMA?

I have no basis on which to judge that, so it's really up to you.

Re: [PATCH v3 0/3] Assorted improvements to usercopy

[Kees Cook]

On Mon, Dec 13, 2021 at 09:16:46PM +0000, Matthew Wilcox wrote:
> On Mon, Dec 13, 2021 at 12:47:58PM -0800, Kees Cook wrote:
> > On Mon, Dec 13, 2021 at 08:27:42PM +0000, Matthew Wilcox wrote:
> > > On Mon, Dec 13, 2021 at 07:18:57PM +0000, William Kucharski wrote:
> > > > I like these, but a quick question:
> > > > 
> > > > Since the usercopy_abort() calls are all because the offset exceeds the page
> > > > size, is there a reason why you don't specifically state that via the detail
> > > > parameter rather than just supply a NULL pointer?
> > > 
> > > Hmm ... I'd defer to Kees on this, because I'm not familiar with
> > > usercopy_abort() usage, but the only places which use the detail
> > > parameter today are slab/slub, which use it to pass the name of
> > > the slab.  I think the user is supposed to infer that we overran the
> > > end of the page based on the offset & length values.
> > 
> > I agree that leaving it NULL is best here. The "detail" is really about
> > adding more information about which thing it was, which for slab makes
> > sense, but most other stuff there isn't really anything to quickly
> > distinguish one from another (i.e. vmap is all vmap).
> 
> There _is_ a bit more information in the vmap case (not in the kmap
> or compound page case).  You can see it in /proc/vmallocinfo.  We
> could pass it in like this?
> 
>         if (is_vmalloc_addr(ptr)) {
>                 struct vm_struct *vm = find_vm_area(ptr);
> +               char sym[100];
>                 unsigned long offset;
> 
>                 if (!vm) {
> ..
> +               if (vm->caller)
> +                       snprintf(sym, sizeof(sym), "%pS", vm->caller);
>                 offset = ptr - vm->addr;
>                 if (offset + n > vm->size)
> -                       usercopy_abort("vmalloc", NULL, to_user, offset, n);
> +                       usercopy_abort("vmalloc", vm->caller ? sym : NULL,
> +                                       to_user, offset, n);

That is interesting, but I think we don't want to do it here; adding
to stack or making an allocation for this (even though it's slow-path)
doesn't seem like a good idea as far as keeping code size down.

-Kees

>                 return;
> 

-- 
Kees Cook

Re: [PATCH v3 3/3] mm/usercopy: Detect compound page overruns

[Kees Cook]

On Mon, Dec 13, 2021 at 11:44:33PM +0000, Matthew Wilcox wrote:
> On Mon, Dec 13, 2021 at 12:52:22PM -0800, Kees Cook wrote:
> > On Mon, Dec 13, 2021 at 02:27:03PM +0000, Matthew Wilcox (Oracle) wrote:
> > > Move the compound page overrun detection out of
> > > CONFIG_HARDENED_USERCOPY_PAGESPAN so it's enabled for more people.
> > 
> > I'd argue that everything else enabled by USERCOPY_PAGESPAN could be
> > removed now too. Do you want to add a 4th patch to rip that out?
> > 
> > https://github.com/KSPP/linux/issues/163
> 
> I don't mind ... is it your assessment that it's not worth checking for
> a copy_to/from_user that spans a boundary between a reserved and
> !reserved page, or overlaps the boundary of rodata/bss/data/CMA?
> 
> I have no basis on which to judge that, so it's really up to you.

It's always been a problem because some arch mark the kernel as reserved,
so we have to do all the allow-listing first, which is tedious. I'd
certainly like to add all the checks possible, but rationally, we need
to keep only the stuff that is fast, useful, or both. PAGESPAN has been
disabled almost everywhere, too, so I don't think it's a loss.

-- 
Kees Cook