| Message ID | 201909161431.E69B29A0@keescook (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usercopy: Skip HIGHMEM page checking | expand |
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.
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.
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.
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...
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);
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(-)