Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1478536 > unrolled thread

[PATCH v2] usercopy: remove page-spanning test for now

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] usercopy: remove page-spanning test for now Kees Cook <keescook@chromium.org> - 2016-09-07 20:10 +0200
    Re: [PATCH v2] usercopy: remove page-spanning test for now Kees Cook <keescook@chromium.org> - 2016-09-07 20:40 +0200
    Re: [PATCH v2] usercopy: remove page-spanning test for now Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-07 20:40 +0200
    Re: [PATCH v2] usercopy: remove page-spanning test for now Rik van Riel <riel@redhat.com> - 2016-09-07 20:40 +0200

#1478536 — [PATCH v2] usercopy: remove page-spanning test for now

A custom allocator without __GFP_COMP that copies to userspace has been
found in vmw_execbuf_process[1], so this disables the page-span checker
by placing it behind a CONFIG for future work where such things can be
tracked down later.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1373326

Reported-by: Vinson Lee <vlee@freedesktop.org>
Fixes: f5509cc18daa ("mm: Hardened usercopy")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
- split logic into separate function entirely, torvalds
---
 mm/usercopy.c    | 61 ++++++++++++++++++++++++++++++++------------------------
 security/Kconfig | 11 ++++++++++
 2 files changed, 46 insertions(+), 26 deletions(-)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index a3cc3052f830..089328f2b920 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -134,31 +134,16 @@ static inline const char *check_bogus_address(const void *ptr, unsigned long n)
 	return NULL;
 }
 
-static inline const char *check_heap_object(const void *ptr, unsigned long n,
-					    bool to_user)
+/* Checks for allocs that are marked in some way as spanning multiple pages. */
+static inline const char *check_page_span(const void *ptr, unsigned long n,
+					  struct page *page, bool to_user)
 {
-	struct page *page, *endpage;
+#ifdef CONFIG_HARDENED_USERCOPY_PAGESPAN
 	const void *end = ptr + n - 1;
+	struct page *endpage;
 	bool is_reserved, is_cma;
 
 	/*
-	 * Some architectures (arm64) return true for virt_addr_valid() on
-	 * vmalloced addresses. Work around this by checking for vmalloc
-	 * first.
-	 */
-	if (is_vmalloc_addr(ptr))
-		return NULL;
-
-	if (!virt_addr_valid(ptr))
-		return NULL;
-
-	page = virt_to_head_page(ptr);
-
-	/* Check slab allocator for flags and size. */
-	if (PageSlab(page))
-		return __check_heap_object(ptr, n, page);
-
-	/*
 	 * Sometimes the kernel data regions are not marked Reserved (see
 	 * check below). And sometimes [_sdata,_edata) does not cover
 	 * rodata and/or bss, so check each range explicitly.
@@ -186,7 +171,7 @@ static inline const char *check_heap_object(const void *ptr, unsigned long n,
 		   ((unsigned long)end & (unsigned long)PAGE_MASK)))
 		return NULL;
 
-	/* Allow if start and end are inside the same compound page. */
+	/* Allow if fully inside the same compound (__GFP_COMP) page. */
 	endpage = virt_to_head_page(end);
 	if (likely(endpage == page))
 		return NULL;
@@ -199,20 +184,44 @@ static inline const char *check_heap_object(const void *ptr, unsigned long n,
 	is_reserved = PageReserved(page);
 	is_cma = is_migrate_cma_page(page);
 	if (!is_reserved && !is_cma)
-		goto reject;
+		return "<spans multiple pages>";
 
 	for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
 		page = virt_to_head_page(ptr);
 		if (is_reserved && !PageReserved(page))
-			goto reject;
+			return "<spans Reserved and non-Reserved pages>";
 		if (is_cma && !is_migrate_cma_page(page))
-			goto reject;
+			return "<spans CMA and non-CMA pages>";
 	}
+#endif
 
 	return NULL;
+}
+
+static inline const char *check_heap_object(const void *ptr, unsigned long n,
+					    bool to_user)
+{
+	struct page *page;
+
+	/*
+	 * Some architectures (arm64) return true for virt_addr_valid() on
+	 * vmalloced addresses. Work around this by checking for vmalloc
+	 * first.
+	 */
+	if (is_vmalloc_addr(ptr))
+		return NULL;
+
+	if (!virt_addr_valid(ptr))
+		return NULL;
+
+	page = virt_to_head_page(ptr);
+
+	/* Check slab allocator for flags and size. */
+	if (PageSlab(page))
+		return __check_heap_object(ptr, n, page);
 
-reject:
-	return "<spans multiple pages>";
+	/* Verify object does not incorrectly span multiple pages. */
+	return check_page_span(ptr, n, page, to_user);
 }
 
 /*
diff --git a/security/Kconfig b/security/Kconfig
index da10d9b573a4..2dfc0ce4083e 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -147,6 +147,17 @@ config HARDENED_USERCOPY
 	  or are part of the kernel text. This kills entire classes
 	  of heap overflow exploits and similar kernel memory exposures.
 
+config HARDENED_USERCOPY_PAGESPAN
+	bool "Refuse to copy allocations that span multiple pages"
+	depends on HARDENED_USERCOPY
+	depends on !COMPILE_TEST
+	help
+	  When a multi-page allocation is done without __GFP_COMP,
+	  hardened usercopy will reject attempts to copy it. There are,
+	  however, several cases of this in the kernel that have not all
+	  been removed. This config is intended to be used only while
+	  trying to find such users.
+
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
-- 
2.7.4


-- 
Kees Cook
Nexus Security

[toc] | [next] | [standalone]

#1478546

On Wed, Sep 7, 2016 at 11:31 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Sep 7, 2016 at 11:08 AM, Kees Cook <keescook@chromium.org> wrote:
>> v2:
>> - split logic into separate function entirely, torvalds
>
> The diff looks much messier, but the end result looks better. Thanks.
>
> Was I supposed to apply this directly, or will it come through a git tree later?

I can send it as a pull request now along with the __always_inline
fix. One moment...

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1478550

On Wed, Sep 7, 2016 at 11:08 AM, Kees Cook <keescook@chromium.org> wrote:
> v2:
> - split logic into separate function entirely, torvalds

The diff looks much messier, but the end result looks better. Thanks.

Was I supposed to apply this directly, or will it come through a git tree later?

                    Linus

[toc] | [prev] | [next] | [standalone]

#1478552

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2016-09-07 at 11:08 -0700, Kees Cook wrote:
> A custom allocator without __GFP_COMP that copies to userspace has
> been
> found in vmw_execbuf_process[1], so this disables the page-span
> checker
> by placing it behind a CONFIG for future work where such things can
> be
> tracked down later.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1373326
> 
> Reported-by: Vinson Lee <vlee@freedesktop.org>
> Fixes: f5509cc18daa ("mm: Hardened usercopy")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web