usercopy: Skip HIGHMEM page checking
diff mbox series

Message ID 201909161431.E69B29A0@keescook
State New
Headers show
Series
  • usercopy: Skip HIGHMEM page checking
Related show

Commit Message

Kees Cook Sept. 16, 2019, 9:32 p.m. UTC
When running on a system with >512MB RAM with a 32-bit kernel built with:

	CONFIG_DEBUG_VIRTUAL=y
	CONFIG_HIGHMEM=y
	CONFIG_HARDENED_USERCOPY=y

all execve()s will fail due to argv copying into kmap()ed pages, and on
usercopy checking the calls ultimately of virt_to_page() will be looking
for "bad" kmap (highmem) pointers due to CONFIG_DEBUG_VIRTUAL=y:

 ------------[ cut here ]------------
 kernel BUG at ../arch/x86/mm/physaddr.c:83!
 invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
 CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.3.0-rc8 #6
 Hardware name: Dell Inc. Inspiron 1318/0C236D, BIOS A04 01/15/2009
 EIP: __phys_addr+0xaf/0x100
 ...
 Call Trace:
  __check_object_size+0xaf/0x3c0
  ? __might_sleep+0x80/0xa0
  copy_strings+0x1c2/0x370
  copy_strings_kernel+0x2b/0x40
  __do_execve_file+0x4ca/0x810
  ? kmem_cache_alloc+0x1c7/0x370
  do_execve+0x1b/0x20
  ...

fs/exec.c:
		kaddr = kmap(kmapped_page);
	...
	if (copy_from_user(kaddr+offset, str, bytes_to_copy)) ...

Without CONFIG_DEBUG_VIRTUAL=y, these pages are effectively ignored,
so now we do the same explicitly: detect and ignore kmap pages, instead
of tripping over the check later.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Fixes: f5509cc18daa ("mm: Hardened usercopy")
Cc: Matthew Wilcox <willy@infradead.org>
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Randy, I dropped your other Tested-by, since this is a different
approach. I would expect the results to be identical (i.e. my testing
shows it works), but I didn't want to assume. :)
---
 include/linux/highmem.h | 7 +++++++
 mm/highmem.c            | 2 +-
 mm/usercopy.c           | 3 ++-
 3 files changed, 10 insertions(+), 2 deletions(-)

Comments

Matthew Wilcox Sept. 17, 2019, 12:32 a.m. UTC | #1
On Mon, Sep 16, 2019 at 02:32:56PM -0700, Kees Cook wrote:
> When running on a system with >512MB RAM with a 32-bit kernel built with:
> 
> 	CONFIG_DEBUG_VIRTUAL=y
> 	CONFIG_HIGHMEM=y
> 	CONFIG_HARDENED_USERCOPY=y
> 
> all execve()s will fail due to argv copying into kmap()ed pages, and on
> usercopy checking the calls ultimately of virt_to_page() will be looking
> for "bad" kmap (highmem) pointers due to CONFIG_DEBUG_VIRTUAL=y:

I don't understand why you want to skip the check.  We must not cross a
page boundary of a kmapped page.
Kees Cook Sept. 17, 2019, 3:05 a.m. UTC | #2
On Mon, Sep 16, 2019 at 05:32:09PM -0700, Matthew Wilcox wrote:
> On Mon, Sep 16, 2019 at 02:32:56PM -0700, Kees Cook wrote:
> > When running on a system with >512MB RAM with a 32-bit kernel built with:
> > 
> > 	CONFIG_DEBUG_VIRTUAL=y
> > 	CONFIG_HIGHMEM=y
> > 	CONFIG_HARDENED_USERCOPY=y
> > 
> > all execve()s will fail due to argv copying into kmap()ed pages, and on
> > usercopy checking the calls ultimately of virt_to_page() will be looking
> > for "bad" kmap (highmem) pointers due to CONFIG_DEBUG_VIRTUAL=y:
> 
> I don't understand why you want to skip the check.  We must not cross a
> page boundary of a kmapped page.

That requires a new test which hasn't existed before. First I need to
fix the bug, and then we can add a new test and get that into -next,
etc.
Matthew Wilcox Sept. 17, 2019, 4:36 p.m. UTC | #3
On Mon, Sep 16, 2019 at 08:05:00PM -0700, Kees Cook wrote:
> On Mon, Sep 16, 2019 at 05:32:09PM -0700, Matthew Wilcox wrote:
> > On Mon, Sep 16, 2019 at 02:32:56PM -0700, Kees Cook wrote:
> > > When running on a system with >512MB RAM with a 32-bit kernel built with:
> > > 
> > > 	CONFIG_DEBUG_VIRTUAL=y
> > > 	CONFIG_HIGHMEM=y
> > > 	CONFIG_HARDENED_USERCOPY=y
> > > 
> > > all execve()s will fail due to argv copying into kmap()ed pages, and on
> > > usercopy checking the calls ultimately of virt_to_page() will be looking
> > > for "bad" kmap (highmem) pointers due to CONFIG_DEBUG_VIRTUAL=y:
> > 
> > I don't understand why you want to skip the check.  We must not cross a
> > page boundary of a kmapped page.
> 
> That requires a new test which hasn't existed before. First I need to
> fix the bug, and then we can add a new test and get that into -next,
> etc.

I suppose that depends where your baseline is.  From the perspective
of "before Kees added this feature", your point of view makes sense.
From the perspective of "what's been shipping for the last six months",
this is a case which has simply not happened before now (or we'd've seen
a bug report).

I don't think you need to change anything for check_page_span() to do
the right thing.  The rodata/data/bss checks will all fall through.
If the copy has the correct bounds, the 'wholly within one base page'
check will pass and it'll return.  If the copy does span a page,
the virt_to_head_page(end) call will return something bogus, then the
PageReserved and CMA test will cause the usercopy_abort() test to fail.

So I think your first patch is the right patch.
Kees Cook Sept. 17, 2019, 5:39 p.m. UTC | #4
On Tue, Sep 17, 2019 at 09:36:06AM -0700, Matthew Wilcox wrote:
> If the copy has the correct bounds, the 'wholly within one base page'
> check will pass and it'll return.  If the copy does span a page,
> the virt_to_head_page(end) call will return something bogus, then the
> PageReserved and CMA test will cause the usercopy_abort() test to fail.
> 
> So I think your first patch is the right patch.

Okay, good points. I'll respin...

Patch
diff mbox series

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index ea5cdbd8c2c3..c881698b8023 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -64,12 +64,19 @@  static inline void totalhigh_pages_set(long val)
 
 void kmap_flush_unused(void);
 
+static inline bool is_kmap(unsigned long addr)
+{
+	return (addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP));
+}
+
 struct page *kmap_to_page(void *addr);
 
 #else /* CONFIG_HIGHMEM */
 
 static inline unsigned int nr_free_highpages(void) { return 0; }
 
+static inline bool is_kmap(unsigned long addr) { return false; }
+
 static inline struct page *kmap_to_page(void *addr)
 {
 	return virt_to_page(addr);
diff --git a/mm/highmem.c b/mm/highmem.c
index 107b10f9878e..e99eca4f63fa 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -151,7 +151,7 @@  struct page *kmap_to_page(void *vaddr)
 {
 	unsigned long addr = (unsigned long)vaddr;
 
-	if (addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP)) {
+	if (is_kmap(addr)) {
 		int i = PKMAP_NR(addr);
 		return pte_page(pkmap_page_table[i]);
 	}
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 98e924864554..924e634cc95d 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -11,6 +11,7 @@ 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/mm.h>
+#include <linux/highmem.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
 #include <linux/sched/task.h>
@@ -224,7 +225,7 @@  static inline void check_heap_object(const void *ptr, unsigned long n,
 {
 	struct page *page;
 
-	if (!virt_addr_valid(ptr))
+	if (!virt_addr_valid(ptr) || is_kmap((unsigned long)ptr))
 		return;
 
 	page = virt_to_head_page(ptr);