usercopy: Skip HIGHMEM page checking

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.

---
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(-)

index ea5cdbd8c2c3..c881698b8023 100644
@@ -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);

index 107b10f9878e..e99eca4f63fa 100644
@@ -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]);
 	}

index 98e924864554..924e634cc95d 100644
@@ -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);

2.17.1


-- 
Kees Cook

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.

Kees Cook

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.

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

Kees Cook