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

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

[Kees Cook]

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>
---
 mm/usercopy.c    |  8 ++++++++
 security/Kconfig | 11 +++++++++++
 2 files changed, 19 insertions(+)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index a3cc3052f830..b3f0c3355dc8 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -172,6 +172,14 @@ static inline const char *check_heap_object(const void *ptr, unsigned long n,
 		return NULL;
 	}
 
+#ifndef CONFIG_HARDENED_USERCOPY_PAGESPAN
+	/*
+	 * The page-spanning checks are hitting false positives, so
+	 * do not check them for now.
+	 */
+	return NULL;
+#endif
+
 	/* Allow kernel data region (if not marked as Reserved). */
 	if (ptr >= (const void *)_sdata && end <= (const void *)_edata)
 		return NULL;
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

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

[Linus Torvalds]

On Wed, Sep 7, 2016 at 10:06 AM, Kees Cook <keescook@chromium.org> wrote:
>
> +#ifndef CONFIG_HARDENED_USERCOPY_PAGESPAN
> +       /*
> +        * The page-spanning checks are hitting false positives, so
> +        * do not check them for now.
> +        */
> +       return NULL;
> +#endif
> +
>         /* Allow kernel data region (if not marked as Reserved). */
>         if (ptr >= (const void *)_sdata && end <= (const void *)_edata)
>                 return NULL;

No. Don't do patches like this.

It's wrong for two reasons:

 (a) if you want to use an #ifdef to disable code, do so. Enclose the
code you want to disable with the #ifdef, not some *other* code that
then indirectly disables the code you want to disable.

 (b) don't do "surprising" things with control flow. It can cause
compiler warnings in reasonable compilers ("unreachable code"), but
it's also a strange pattern that throws people.

So really, make the patch bigger but more legible. In fact, I think
the best option would be to simply turn the code you want to disable
into a helper function of its own, and then make the #ifdef enable or
disable the whole function.

(That also solves the problems like having the declaration for
"endpage" and the other variables that is only used by the disabled
code be *with* the disabled code, so that you don't have to have
multiple ifdef'ed regions. I suspect avoiding that is a large reason
why you did the hacky thing in the first place).

                Linus

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

[Vegard Nossum]

On 7 September 2016 at 19:17, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Sep 7, 2016 at 10:06 AM, Kees Cook <keescook@chromium.org> wrote:
>>
>> +#ifndef CONFIG_HARDENED_USERCOPY_PAGESPAN
>> +       /*
>> +        * The page-spanning checks are hitting false positives, so
>> +        * do not check them for now.
>> +        */
>> +       return NULL;
>> +#endif
>> +
>>         /* Allow kernel data region (if not marked as Reserved). */
>>         if (ptr >= (const void *)_sdata && end <= (const void *)_edata)
>>                 return NULL;
>
> No. Don't do patches like this.
>
> It's wrong for two reasons:
>
>  (a) if you want to use an #ifdef to disable code, do so. Enclose the
> code you want to disable with the #ifdef, not some *other* code that
> then indirectly disables the code you want to disable.
>
>  (b) don't do "surprising" things with control flow. It can cause
> compiler warnings in reasonable compilers ("unreachable code"), but
> it's also a strange pattern that throws people.
>
> So really, make the patch bigger but more legible. In fact, I think
> the best option would be to simply turn the code you want to disable
> into a helper function of its own, and then make the #ifdef enable or
> disable the whole function.
>
> (That also solves the problems like having the declaration for
> "endpage" and the other variables that is only used by the disabled
> code be *with* the disabled code, so that you don't have to have
> multiple ifdef'ed regions. I suspect avoiding that is a large reason
> why you did the hacky thing in the first place).

For this particular case, one might also use something like

if (!IS_ENABLED(CONFIG_HARDENED_USERCOPY_PAGESPAN))
    return;

no ifdefs, the "early return" is a common pattern (and readable IMHO),
and no unused variable warnings or separate ifdef blocks for variables
and I *think* no unreachable code warnings.

Or?
</bikeshed>


Vegard