[PATCH 0/3] Assorted improvements to usercopy

[PATCH 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?

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                    | 33 +++++++++++++++++++++-----------
 3 files changed, 33 insertions(+), 11 deletions(-)

-- 
2.32.0

[PATCH 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>
---
 arch/x86/include/asm/highmem.h   |  1 +
 include/linux/highmem-internal.h | 10 ++++++++++
 mm/usercopy.c                    | 15 +++++++++------
 3 files changed, 20 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 4aa1031d3e4c..97d6dc836749 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -143,6 +143,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)
@@ -223,6 +228,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..ac95b22fbbce 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -228,12 +228,15 @@ 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, 0, n);
+		return;
+	}
+
+	page = virt_to_head_page(ptr);
 
 	if (PageSlab(page)) {
 		/* Check slab allocator for flags and size. */
-- 
2.32.0

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

[Kees Cook]

On Mon, Oct 04, 2021 at 11:42:21PM +0100, Matthew Wilcox (Oracle) wrote:
> 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>
> ---
>  arch/x86/include/asm/highmem.h   |  1 +
>  include/linux/highmem-internal.h | 10 ++++++++++
>  mm/usercopy.c                    | 15 +++++++++------
>  3 files changed, 20 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 4aa1031d3e4c..97d6dc836749 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -143,6 +143,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)
> @@ -223,6 +228,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..ac95b22fbbce 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -228,12 +228,15 @@ 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, 0, n);

It's likely not worth getting an offset here, but "0" above could be
something like "ptr - PKMAP_ADDR(0)".

Either way:

Acked-by: Kees Cook <keescook@chromium.org>

Thanks!

-Kees

> +		return;
> +	}
> +
> +	page = virt_to_head_page(ptr);
>  
>  	if (PageSlab(page)) {
>  		/* Check slab allocator for flags and size. */
> -- 
> 2.32.0
> 

-- 
Kees Cook

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

[Matthew Wilcox]

On Tue, Oct 05, 2021 at 02:23:09PM -0700, Kees Cook wrote:
> > +	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, 0, n);
> 
> It's likely not worth getting an offset here, but "0" above could be
> something like "ptr - PKMAP_ADDR(0)".

Mmm.  page_offset(ptr) should do the trick, no?

> Either way:
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> Thanks!
> 
> -Kees
> 
> > +		return;
> > +	}
> > +
> > +	page = virt_to_head_page(ptr);
> >  
> >  	if (PageSlab(page)) {
> >  		/* Check slab allocator for flags and size. */
> > -- 
> > 2.32.0
> > 
> 
> -- 
> Kees Cook

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

[Kees Cook]

On Tue, Oct 05, 2021 at 10:43:13PM +0100, Matthew Wilcox wrote:
> On Tue, Oct 05, 2021 at 02:23:09PM -0700, Kees Cook wrote:
> > > +	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, 0, n);
> > 
> > It's likely not worth getting an offset here, but "0" above could be
> > something like "ptr - PKMAP_ADDR(0)".
> 
> Mmm.  page_offset(ptr) should do the trick, no?

Ah yeah, that'd be good!

-Kees

> 
> > Either way:
> > 
> > Acked-by: Kees Cook <keescook@chromium.org>
> > 
> > Thanks!
> > 
> > -Kees
> > 
> > > +		return;
> > > +	}
> > > +
> > > +	page = virt_to_head_page(ptr);
> > >  
> > >  	if (PageSlab(page)) {
> > >  		/* Check slab allocator for flags and size. */
> > > -- 
> > > 2.32.0
> > > 
> > 
> > -- 
> > Kees Cook

-- 
Kees Cook

[PATCH 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>
---
 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);
+		return;
+	}
+
 	page = virt_to_head_page(ptr);
 
 	if (PageSlab(page)) {
-- 
2.32.0

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

[Kees Cook]

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
> 

-- 
Kees Cook

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

[Matthew Wilcox]

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?

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

[Kees Cook]

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.

-- 
Kees Cook

[PATCH 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>
---
 mm/usercopy.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index 7bfc4f9ed1e4..e395462961d5 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -194,11 +194,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
@@ -250,6 +245,10 @@ 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 */
+		if (ptr + n > page_address(page) + page_size(page))
+			usercopy_abort("page alloc", NULL, to_user, 0, n);
 	} else {
 		/* Verify object does not incorrectly span multiple pages. */
 		check_page_span(ptr, n, page, to_user);
-- 
2.32.0

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

[Kees Cook]

On Mon, Oct 04, 2021 at 11:42:23PM +0100, Matthew Wilcox (Oracle) wrote:
> 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>
> ---
>  mm/usercopy.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 7bfc4f9ed1e4..e395462961d5 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -194,11 +194,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
> @@ -250,6 +245,10 @@ 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 */
> +		if (ptr + n > page_address(page) + page_size(page))
> +			usercopy_abort("page alloc", NULL, to_user, 0, n);

"0" could be "ptr - page_address(page)", I think? With that:

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>  	} else {
>  		/* Verify object does not incorrectly span multiple pages. */
>  		check_page_span(ptr, n, page, to_user);
> -- 
> 2.32.0
> 

-- 
Kees Cook

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

[Matthew Wilcox]

On Tue, Oct 05, 2021 at 02:26:37PM -0700, Kees Cook wrote:
> On Mon, Oct 04, 2021 at 11:42:23PM +0100, Matthew Wilcox (Oracle) wrote:
> > +	} else if (PageHead(page)) {
> > +		/* A compound allocation */
> > +		if (ptr + n > page_address(page) + page_size(page))
> > +			usercopy_abort("page alloc", NULL, to_user, 0, n);
> 
> "0" could be "ptr - page_address(page)", I think? With that:
> 
> Acked-by: Kees Cook <keescook@chromium.org>

Right, so that can be:

        } 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);

which saves us calling page_address() twice.  Probably GCC is smart
enough to CSE it anyway, but it also avoids splitting at the 80 column
boundary ;-)

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

[Kees Cook]

On Tue, Oct 05, 2021 at 11:12:47PM +0100, Matthew Wilcox wrote:
> On Tue, Oct 05, 2021 at 02:26:37PM -0700, Kees Cook wrote:
> > On Mon, Oct 04, 2021 at 11:42:23PM +0100, Matthew Wilcox (Oracle) wrote:
> > > +	} else if (PageHead(page)) {
> > > +		/* A compound allocation */
> > > +		if (ptr + n > page_address(page) + page_size(page))
> > > +			usercopy_abort("page alloc", NULL, to_user, 0, n);
> > 
> > "0" could be "ptr - page_address(page)", I think? With that:
> > 
> > Acked-by: Kees Cook <keescook@chromium.org>
> 
> Right, so that can be:
> 
>         } 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);
> 
> which saves us calling page_address() twice.  Probably GCC is smart
> enough to CSE it anyway, but it also avoids splitting at the 80 column
> boundary ;-)

Perfect, yes!

-- 
Kees Cook

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

[Kees Cook]

On Mon, Oct 04, 2021 at 11:42:20PM +0100, Matthew Wilcox (Oracle) 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?
> 
> Matthew Wilcox (Oracle) (3):
>   mm/usercopy: Check kmap addresses properly
>   mm/usercopy: Detect vmalloc overruns
>   mm/usercopy: Detect compound page overruns

Thanks! This is a nice additional bit of checking. I wonder if the CMA
and Reserved pieces should be extracted from the PAGESPAN check too?
Probably that CONFIG should be renamed as well now. :P

-Kees

> 
>  arch/x86/include/asm/highmem.h   |  1 +
>  include/linux/highmem-internal.h | 10 ++++++++++
>  mm/usercopy.c                    | 33 +++++++++++++++++++++-----------
>  3 files changed, 33 insertions(+), 11 deletions(-)
> 
> -- 
> 2.32.0
> 

-- 
Kees Cook