[RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Mark Rutland]

Hi,

In Prague, Kees mentioned that it would be nice to have a mechanism to
catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
issue with unsafe_put_user() in waitid().

These patches allow an optional access_ok() check to be dropped in
arm64's __{get,put}_user() primitives. These will then BUG() if a bad
user pointer is passed (which should only happen in the absence of an
earlier access_ok() check).

The first patch rewrites the arm64 access_ok() check in C. This gives
the compiler the visibility it needs to elide redundant access_ok()
checks, so in the common case:

  get_user()
    access_ok()
    __get_user()
      BUG_ON(!access_ok())
      <uaccess asm>

... the compiler can determine that the second access_ok() must return
true, and can elide it along with the BUG_ON(), leaving:

  get_user()
    access_ok()
      __get_user()
        <uaccess asm>

... and thus this sanity check can have no cost in the common case.

The compiler doesn't always have the visibility to do this (e.g. if the
two access_ok() checks are in different compilation units), but it seems
to manage to do this most of the time -- In testing with v4.14-rc5
defconfig this only increases the total Image size by 4KiB.

I had a go at turning this into a BUILD_BUG_ON(), to see if we could
catch this issue at compile time. However, my GCC wasn't able to remove
the BUILD_BUG() from some {get,put}_user cases. Maybe we can fix that,
or maybe we can have some static analysis catch this at build time.

It's entirely possible that I've made some catastrophic mistake in these
patches; I've only build-tested them so far, and I don't currently have
access to hardware to test on.

I also haven't yet modified __copy_{to,from}_user and friends along
similar lines, so this is incomplete. If there aren't any major
objections to this approach, I can fold those in for the next spin.

Thanks,
Mark.

[1] https://lwn.net/Articles/736348/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=96ca579a1ecc943b75beba58bebb0356f6cc4b51


Mark Rutland (2):
  arm64: write __range_ok() in C
  arm64: allow paranoid __{get,put}user

 arch/arm64/Kconfig               |  9 +++++++++
 arch/arm64/include/asm/uaccess.h | 27 +++++++++++++++++++--------
 2 files changed, 28 insertions(+), 8 deletions(-)

-- 
2.11.0

[RFC PATCH 1/2] arm64: write __range_ok() in C

[Mark Rutland]

Currently arm64's __range_ok() is written in assembly for efficiency.

This hides the logic from the compiler, preventing the compiler from
making some optimizations, such as re-ordering instructions or folding
multiple calls to __range_ok().

This patch uses GCC's __builtin_uaddl_overflow() to provide an
equivalent, efficient check, while giving the compiler the visibility it
needs to optimize the check. In testing with v4.14-rc5 using the Linaro
17.05 GCC 6.3.1 toolchain, this has no impact on the kernel Image size,
(but results in a smaller vmlinux).

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/uaccess.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index fc0f9eb66039..36f84ec92b9d 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -70,17 +70,20 @@ static inline void set_fs(mm_segment_t fs)
  *
  * This needs 65-bit arithmetic.
  */
+static bool __range_ok_c(unsigned long addr, unsigned long size)
+{
+	unsigned long result;
+
+	if (__builtin_uaddl_overflow(addr, size, &result))
+		return false;
+
+	return result < current_thread_info()->addr_limit;
+}
+
 #define __range_ok(addr, size)						\
 ({									\
-	unsigned long __addr = (unsigned long)(addr);			\
-	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, ls"		\
-		: "=&r" (flag), "=&r" (roksum)				\
-		: "1" (__addr), "Ir" (size),				\
-		  "r" (current_thread_info()->addr_limit)		\
-		: "cc");						\
-	flag;								\
+	__range_ok_c((unsigned long)(addr), (unsigned long)(size));	\
 })
 
 /*
-- 
2.11.0

[RFC PATCH 2/2] arm64: allow paranoid __{get,put}user

[Mark Rutland]

Now that the compiler can identify redundant access_ok() checks, we can
make __get-user() and __put_user() BUG()-out if there wasn't a preceding
access_ok() check. So long as that's in the same compilation unit, the
compiler should be able to get rid of the redundant second check and BUG
entry.

This will allow us to catch __{get,put}_user() calls which did not have
a preceding access_ok() check, but may adversely affect a small number
of callsites where GCC fails to spot that it can fold two access_ok()
checks together.

As these checks may impact performance and code size, they are only
enabled when CONFIG_ARM64_PARANOID_UACCESS is selected.

In testing with v4.14-rc5 with the Linaro 17.05 GCC 6.3.1 toolchain,
this makes the kernel Image ~4KiB bigger, and the vmlinux ~93k bigger. I
have no performance numbers so far.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig               | 9 +++++++++
 arch/arm64/include/asm/uaccess.h | 8 ++++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 0df64a6a56d4..34df81acda8e 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1028,6 +1028,15 @@ config RANDOMIZE_MODULE_REGION_FULL
 	  a limited range that contains the [_stext, _etext] interval of the
 	  core kernel, so branch relocations are always in range.
 
+config ARM64_PARANOID_UACCESS
+	bool "Use paranoid uaccess primitives"
+	help
+	  Forces access_ok() checks in __get_user(), __put_user(), and other
+	  low-level uaccess primitives which usually do not have checks. This
+	  can limit the effect of missing access_ok() checks in higher-level
+	  primitives, with a runtime performance overhead in some cases and a
+	  small code size overhead.
+
 endmenu
 
 menu "Boot options"
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 36f84ec92b9d..dbe8dfd46ceb 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -195,6 +195,12 @@ static inline void uaccess_enable_not_uao(void)
 	__uaccess_enable(ARM64_ALT_PAN_NOT_UAO);
 }
 
+#define verify_uaccess(dir, ptr)					\
+({									\
+	if (IS_ENABLED(CONFIG_ARM64_PARANOID_UACCESS))			\
+		BUG_ON(!access_ok(dir, (ptr), sizeof(*(ptr))));		\
+})
+
 /*
  * The "__xxx" versions of the user access functions do not verify the address
  * space - it must have been done previously with a separate "access_ok()"
@@ -222,6 +228,7 @@ static inline void uaccess_enable_not_uao(void)
 do {									\
 	unsigned long __gu_val;						\
 	__chk_user_ptr(ptr);						\
+	verify_uaccess(VERIFY_READ, ptr);				\
 	uaccess_enable_not_uao();					\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
@@ -287,6 +294,7 @@ do {									\
 do {									\
 	__typeof__(*(ptr)) __pu_val = (x);				\
 	__chk_user_ptr(ptr);						\
+	verify_uaccess(VERIFY_WRITE, ptr);				\
 	uaccess_enable_not_uao();					\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
-- 
2.11.0

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Will Deacon]

On Thu, Oct 26, 2017 at 10:09:40AM +0100, Mark Rutland wrote:
> Hi,
> 
> In Prague, Kees mentioned that it would be nice to have a mechanism to
> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
> issue with unsafe_put_user() in waitid().
> 
> These patches allow an optional access_ok() check to be dropped in
> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
> user pointer is passed (which should only happen in the absence of an
> earlier access_ok() check).
> 
> The first patch rewrites the arm64 access_ok() check in C. This gives
> the compiler the visibility it needs to elide redundant access_ok()
> checks, so in the common case:
> 
>   get_user()
>     access_ok()
>     __get_user()
>       BUG_ON(!access_ok())
>       <uaccess asm>
> 
> ... the compiler can determine that the second access_ok() must return
> true, and can elide it along with the BUG_ON(), leaving:
> 
>   get_user()
>     access_ok()
>       __get_user()
>         <uaccess asm>
> 
> ... and thus this sanity check can have no cost in the common case.

Probably a stupid question, but why not just move the access_ok check
into __{get,put}_user and remove it from {get,put}_user? We can also
then move the uaccess_{enable,disable}_not_uao calls out from the __
variants so that we can implement user_access_{begin,end}.

Will

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Mark Rutland]

On Fri, Oct 27, 2017 at 04:41:13PM +0100, Will Deacon wrote:
> On Thu, Oct 26, 2017 at 10:09:40AM +0100, Mark Rutland wrote:
> > Hi,
> > 
> > In Prague, Kees mentioned that it would be nice to have a mechanism to
> > catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
> > issue with unsafe_put_user() in waitid().
> > 
> > These patches allow an optional access_ok() check to be dropped in
> > arm64's __{get,put}_user() primitives. These will then BUG() if a bad
> > user pointer is passed (which should only happen in the absence of an
> > earlier access_ok() check).
> > 
> > The first patch rewrites the arm64 access_ok() check in C. This gives
> > the compiler the visibility it needs to elide redundant access_ok()
> > checks, so in the common case:
> > 
> >   get_user()
> >     access_ok()
> >     __get_user()
> >       BUG_ON(!access_ok())
> >       <uaccess asm>
> > 
> > ... the compiler can determine that the second access_ok() must return
> > true, and can elide it along with the BUG_ON(), leaving:
> > 
> >   get_user()
> >     access_ok()
> >       __get_user()
> >         <uaccess asm>
> > 
> > ... and thus this sanity check can have no cost in the common case.
> 
> Probably a stupid question, but why not just move the access_ok check
> into __{get,put}_user and remove it from {get,put}_user?

Good question.

I was considering this as a debug option, making it possible to catch unsafe
__{get,put}_user() uses via fuzzing or at build time.

As a hardening option, it would make more sense to always have the check in
__{get,put}_user().

> We can also then move the uaccess_{enable,disable}_not_uao calls out from the
> __ variants so that we can implement user_access_{begin,end}.

Mhmm. I'll take a look at this for v2, afer I've figured out precisely what
I've broken with this RFC.

I'd still like the option to scream on unsafe __{get,put}_user() calls, but it
should be possible to handle both cases with minimal IS_ENABLED() usage.

Thanks,
Mark.

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Russell King - ARM Linux]

On Fri, Oct 27, 2017 at 04:41:13PM +0100, Will Deacon wrote:
> Probably a stupid question, but why not just move the access_ok check
> into __{get,put}_user and remove it from {get,put}_user? We can also
> then move the uaccess_{enable,disable}_not_uao calls out from the __
> variants so that we can implement user_access_{begin,end}.

The intent of __{get,put}_user() is to have a fast accessor compared
to {get,put}_user() which does all the full checks.

However, with the uaccess stuff we have now by default, I don't think
it makes much sense - maybe we're better off using copy_{to,from}_user()
in those code paths and fixing up the struct in kernel space rather than
__{get,put}_user()?

I suspect that if we do have the full checks in __{get,put}_user() that
makes the case stronger for doing that - and maybe killing the __
accessors entirely.

Take a look at kernel/signal.c to see a typical usage of the __
accessors.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Laura Abbott]

On 10/26/2017 02:09 AM, Mark Rutland wrote:
> Hi,
> 
> In Prague, Kees mentioned that it would be nice to have a mechanism to
> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
> issue with unsafe_put_user() in waitid().
> 
> These patches allow an optional access_ok() check to be dropped in
> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
> user pointer is passed (which should only happen in the absence of an
> earlier access_ok() check).
> 
> The first patch rewrites the arm64 access_ok() check in C. This gives
> the compiler the visibility it needs to elide redundant access_ok()
> checks, so in the common case:
> 
>   get_user()
>     access_ok()
>     __get_user()
>       BUG_ON(!access_ok())
>       <uaccess asm>
> 
> ... the compiler can determine that the second access_ok() must return
> true, and can elide it along with the BUG_ON(), leaving:
> 
>   get_user()
>     access_ok()
>       __get_user()
>         <uaccess asm>
> 
> ... and thus this sanity check can have no cost in the common case.
> 
> The compiler doesn't always have the visibility to do this (e.g. if the
> two access_ok() checks are in different compilation units), but it seems
> to manage to do this most of the time -- In testing with v4.14-rc5
> defconfig this only increases the total Image size by 4KiB.
> 
> I had a go at turning this into a BUILD_BUG_ON(), to see if we could
> catch this issue at compile time. However, my GCC wasn't able to remove
> the BUILD_BUG() from some {get,put}_user cases. Maybe we can fix that,
> or maybe we can have some static analysis catch this at build time.
> 
> It's entirely possible that I've made some catastrophic mistake in these
> patches; I've only build-tested them so far, and I don't currently have
> access to hardware to test on.
> 
> I also haven't yet modified __copy_{to,from}_user and friends along
> similar lines, so this is incomplete. If there aren't any major
> objections to this approach, I can fold those in for the next spin.
> 
> Thanks,
> Mark.
> 
> [1] https://lwn.net/Articles/736348/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=96ca579a1ecc943b75beba58bebb0356f6cc4b51
> 
> 
> Mark Rutland (2):
>   arm64: write __range_ok() in C
>   arm64: allow paranoid __{get,put}user
> 
>  arch/arm64/Kconfig               |  9 +++++++++
>  arch/arm64/include/asm/uaccess.h | 27 +++++++++++++++++++--------
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 

Turning on the option fails as soon as we hit userspace. On my buildroot
based environment I get the help text for ld.so (????) and then a message
about attempting to kill init. I get a crash in init on the Hikey Android
environment as well. It almost seems like the __range_ok re-write
is triggering an error but it only seems to happen when the option is
enabled even when I take out the BUG. I'll see if I can get more useful
information.

Thanks,
Laura

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Mark Rutland]

On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
> On 10/26/2017 02:09 AM, Mark Rutland wrote:
> > In Prague, Kees mentioned that it would be nice to have a mechanism to
> > catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
> > issue with unsafe_put_user() in waitid().
> > 
> > These patches allow an optional access_ok() check to be dropped in
> > arm64's __{get,put}_user() primitives. These will then BUG() if a bad
> > user pointer is passed (which should only happen in the absence of an
> > earlier access_ok() check).

> Turning on the option fails as soon as we hit userspace. On my buildroot
> based environment I get the help text for ld.so (????) and then a message
> about attempting to kill init. 

Ouch. Thanks for the report, and sorry about this.

The problem is that I evaluate the ptr argument twice in
__{get,put}_user(), and this may have side effects.

e.g. when the ELF loader does things like:

  __put_user((elf_addr_t)p, sp++)

... we increment sp twice, and write to the wrong user address, leaving
sp corrupt.

I have an additional patch [1] to fix this, which is in my
arm64/access-ok branch [2].

Thanks,
Mark.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/access-ok&id=ebb7ff83eb53b8810395d5cf48712a4ae6d678543
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/access-ok

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Laura Abbott]

On 11/01/2017 05:05 AM, Mark Rutland wrote:
> On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
>> On 10/26/2017 02:09 AM, Mark Rutland wrote:
>>> In Prague, Kees mentioned that it would be nice to have a mechanism to
>>> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
>>> issue with unsafe_put_user() in waitid().
>>>
>>> These patches allow an optional access_ok() check to be dropped in
>>> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
>>> user pointer is passed (which should only happen in the absence of an
>>> earlier access_ok() check).
> 
>> Turning on the option fails as soon as we hit userspace. On my buildroot
>> based environment I get the help text for ld.so (????) and then a message
>> about attempting to kill init. 
> 
> Ouch. Thanks for the report, and sorry about this.
> 
> The problem is that I evaluate the ptr argument twice in
> __{get,put}_user(), and this may have side effects.
> 
> e.g. when the ELF loader does things like:
> 
>   __put_user((elf_addr_t)p, sp++)
> 
> ... we increment sp twice, and write to the wrong user address, leaving
> sp corrupt.
> 
> I have an additional patch [1] to fix this, which is in my
> arm64/access-ok branch [2].
> 
> Thanks,
> Mark.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/access-ok&id=ebb7ff83eb53b8810395d5cf48712a4ae6d678543
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/access-ok
> 

Thanks, the updated patch works. I wrote an LKDTM test to verify
the expected behavior (__{get,put}_user panic whereas {get,put}_user
do not). You're welcome to add Tested-by or I can wait for v2.

Thanks,
Laura

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Kees Cook]

On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 11/01/2017 05:05 AM, Mark Rutland wrote:
>> On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
>>> On 10/26/2017 02:09 AM, Mark Rutland wrote:
>>>> In Prague, Kees mentioned that it would be nice to have a mechanism to
>>>> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
>>>> issue with unsafe_put_user() in waitid().
>>>>
>>>> These patches allow an optional access_ok() check to be dropped in
>>>> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
>>>> user pointer is passed (which should only happen in the absence of an
>>>> earlier access_ok() check).
>>
>>> Turning on the option fails as soon as we hit userspace. On my buildroot
>>> based environment I get the help text for ld.so (????) and then a message
>>> about attempting to kill init.
>>
>> Ouch. Thanks for the report, and sorry about this.
>>
>> The problem is that I evaluate the ptr argument twice in
>> __{get,put}_user(), and this may have side effects.
>>
>> e.g. when the ELF loader does things like:
>>
>>   __put_user((elf_addr_t)p, sp++)
>>
>> ... we increment sp twice, and write to the wrong user address, leaving
>> sp corrupt.
>>
>> I have an additional patch [1] to fix this, which is in my
>> arm64/access-ok branch [2].
>>
>> Thanks,
>> Mark.
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/access-ok&id=ebb7ff83eb53b8810395d5cf48712a4ae6d678543
>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/access-ok
>>
>
> Thanks, the updated patch works. I wrote an LKDTM test to verify
> the expected behavior (__{get,put}_user panic whereas {get,put}_user
> do not). You're welcome to add Tested-by or I can wait for v2.

Nice. :) Out of curiosity, can you check if this correctly BUG()s on a
waitid() call when the fixes are reverted?

96ca579a1ecc ("waitid(): Avoid unbalanced user_access_end() on
access_ok() error")
1c9fec470b81 ("waitid(): Add missing access_ok() checks")

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Laura Abbott]

On 11/01/2017 03:28 PM, Kees Cook wrote:
> On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott <labbott@redhat.com> wrote:
>> On 11/01/2017 05:05 AM, Mark Rutland wrote:
>>> On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
>>>> On 10/26/2017 02:09 AM, Mark Rutland wrote:
>>>>> In Prague, Kees mentioned that it would be nice to have a mechanism to
>>>>> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
>>>>> issue with unsafe_put_user() in waitid().
>>>>>
>>>>> These patches allow an optional access_ok() check to be dropped in
>>>>> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
>>>>> user pointer is passed (which should only happen in the absence of an
>>>>> earlier access_ok() check).
>>>
>>>> Turning on the option fails as soon as we hit userspace. On my buildroot
>>>> based environment I get the help text for ld.so (????) and then a message
>>>> about attempting to kill init.
>>>
>>> Ouch. Thanks for the report, and sorry about this.
>>>
>>> The problem is that I evaluate the ptr argument twice in
>>> __{get,put}_user(), and this may have side effects.
>>>
>>> e.g. when the ELF loader does things like:
>>>
>>>   __put_user((elf_addr_t)p, sp++)
>>>
>>> ... we increment sp twice, and write to the wrong user address, leaving
>>> sp corrupt.
>>>
>>> I have an additional patch [1] to fix this, which is in my
>>> arm64/access-ok branch [2].
>>>
>>> Thanks,
>>> Mark.
>>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/access-ok&id=ebb7ff83eb53b8810395d5cf48712a4ae6d678543
>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/access-ok
>>>
>>
>> Thanks, the updated patch works. I wrote an LKDTM test to verify
>> the expected behavior (__{get,put}_user panic whereas {get,put}_user
>> do not). You're welcome to add Tested-by or I can wait for v2.
> 
> Nice. :) Out of curiosity, can you check if this correctly BUG()s on a
> waitid() call when the fixes are reverted?
> 
> 96ca579a1ecc ("waitid(): Avoid unbalanced user_access_end() on
> access_ok() error")
> 1c9fec470b81 ("waitid(): Add missing access_ok() checks")
> 
> -Kees
> 

Yep, we get a nice bug:

[   34.783912] ------------[ cut here ]------------
[   34.784484] kernel BUG at kernel/exit.c:1614!
[   34.785016] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[   34.785572] Modules linked in:
[   34.786177] CPU: 0 PID: 1324 Comm: a.out Not tainted 4.14.0-rc5-00005-ga3bb7b0f72d3 #69
[   34.786657] Hardware name: linux,dummy-virt (DT)
[   34.787093] task: ffff80003c4ed400 task.stack: ffff00000ade0000
[   34.788196] PC is at SyS_waitid+0x1d4/0x210
[   34.788534] LR is at SyS_waitid+0x20/0x210
[   34.788839] pc : [<ffff0000080cde1c>] lr : [<ffff0000080cdc68>] pstate: a0000145
[   34.789310] sp : ffff00000ade3e00
[   34.789578] x29: ffff00000ade3e00 x28: ffff80003c4ed400 
[   34.790039] x27: ffff0000089e1000 x26: 000000000000005f 
[   34.790397] x25: 0000000000000124 x24: 0000000000000015 
[   34.790649] x23: 0000000080000000 x22: 0000ffffb1eb6b24 
[   34.790897] x21: 00000000ffffffff x20: 000080003600c000 
[   34.791149] x19: ffff800000000000 x18: 0000000000000007 
[   34.791397] x17: 0000000000000001 x16: 0000000000000019 
[   34.791648] x15: 0000000000000033 x14: 000000000000004c 
[   34.791903] x13: 0000000000000068 x12: ffff000008af69d0 
[   34.792156] x11: ffff00000ade3c20 x10: 0000000000000000 
[   34.792451] x9 : 0000000000000000 x8 : 0000000000000000 
[   34.792706] x7 : 0000000000000000 x6 : ffff000008f42f18 
[   34.792965] x5 : dead000000000100 x4 : 0000000000000011 
[   34.793214] x3 : ffff80003c4ed400 x2 : ffff800000000004 
[   34.793462] x1 : 0001000000000000 x0 : 0000000000000000 
[   34.793743] Process a.out (pid: 1324, stack limit = 0xffff00000ade0000)
[   34.794098] Call trace:
[   34.794351] Exception stack(0xffff00000ade3cc0 to 0xffff00000ade3e00)
[   34.794722] 3cc0: 0000000000000000 0001000000000000 ffff800000000004 ffff80003c4ed400
[   34.795034] 3ce0: 0000000000000011 dead000000000100 ffff000008f42f18 0000000000000000
[   34.795297] 3d00: 0000000000000000 0000000000000000 0000000000000000 ffff00000ade3c20
[   34.795549] 3d20: ffff000008af69d0 0000000000000068 000000000000004c 0000000000000033
[   34.795803] 3d40: 0000000000000019 0000000000000001 0000000000000007 ffff800000000000
[   34.796066] 3d60: 000080003600c000 00000000ffffffff 0000ffffb1eb6b24 0000000080000000
[   34.796277] 3d80: 0000000000000015 0000000000000124 000000000000005f ffff0000089e1000
[   34.796477] 3da0: ffff80003c4ed400 ffff00000ade3e00 ffff0000080cdc68 ffff00000ade3e00
[   34.796677] 3dc0: ffff0000080cde1c 00000000a0000145 0000000000000000 ffff80003c4ed400
[   34.796884] 3de0: 0001000000000000 dead000000000100 ffff00000ade3e00 ffff0000080cde1c
[   34.797153] [<ffff0000080cde1c>] SyS_waitid+0x1d4/0x210
[   34.797298] Exception stack(0xffff00000ade3ec0 to 0xffff00000ade4000)
[   34.797470] 3ec0: 0000000000000000 0000000000000000 ffff800000000000 0000000000000004
[   34.797670] 3ee0: 0000000000000000 0400000055550400 0000ffffb1e0a011 0000000000000cbf
[   34.797873] 3f00: 000000000000005f 0000000000000a3b 0000000000000000 16170f120a1a1311
[   34.798073] 3f20: 00000000000001a8 0000ffffb1f8ecb8 0000ffffb1e1c0e0 0000000000000002
[   34.798272] 3f40: 0000ffffb1eb6af0 0000000000420018 0000000000000000 0000000000400740
[   34.798471] 3f60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[   34.798668] 3f80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[   34.798870] 3fa0: 0000000000000000 0000fffff7294200 00000000004006fc 0000fffff7294200
[   34.799068] 3fc0: 0000ffffb1eb6b24 0000000080000000 0000000000000000 000000000000005f
[   34.799265] 3fe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[   34.799474] [<ffff0000080837b0>] el0_svc_naked+0x24/0x28
[   34.799715] Code: f9400fb4 17ffff9a d503201f f9000fb4 (d4210000) 
[   34.800121] ---[ end trace a14ca5cd5d8f9b30 ]---

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Kees Cook]

On Wed, Nov 1, 2017 at 4:05 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 11/01/2017 03:28 PM, Kees Cook wrote:
>> On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott <labbott@redhat.com> wrote:
>>> On 11/01/2017 05:05 AM, Mark Rutland wrote:
>>>> On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
>>>>> On 10/26/2017 02:09 AM, Mark Rutland wrote:
>>>>>> In Prague, Kees mentioned that it would be nice to have a mechanism to
>>>>>> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
>>>>>> issue with unsafe_put_user() in waitid().
>>>>>>
>>>>>> These patches allow an optional access_ok() check to be dropped in
>>>>>> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
>>>>>> user pointer is passed (which should only happen in the absence of an
>>>>>> earlier access_ok() check).
>>>>
>>>>> Turning on the option fails as soon as we hit userspace. On my buildroot
>>>>> based environment I get the help text for ld.so (????) and then a message
>>>>> about attempting to kill init.
>>>>
>>>> Ouch. Thanks for the report, and sorry about this.
>>>>
>>>> The problem is that I evaluate the ptr argument twice in
>>>> __{get,put}_user(), and this may have side effects.
>>>>
>>>> e.g. when the ELF loader does things like:
>>>>
>>>>   __put_user((elf_addr_t)p, sp++)
>>>>
>>>> ... we increment sp twice, and write to the wrong user address, leaving
>>>> sp corrupt.
>>>>
>>>> I have an additional patch [1] to fix this, which is in my
>>>> arm64/access-ok branch [2].
>>>>
>>>> Thanks,
>>>> Mark.
>>>>
>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/access-ok&id=ebb7ff83eb53b8810395d5cf48712a4ae6d678543
>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/access-ok
>>>>
>>>
>>> Thanks, the updated patch works. I wrote an LKDTM test to verify
>>> the expected behavior (__{get,put}_user panic whereas {get,put}_user
>>> do not). You're welcome to add Tested-by or I can wait for v2.
>>
>> Nice. :) Out of curiosity, can you check if this correctly BUG()s on a
>> waitid() call when the fixes are reverted?
>>
>> 96ca579a1ecc ("waitid(): Avoid unbalanced user_access_end() on
>> access_ok() error")
>> 1c9fec470b81 ("waitid(): Add missing access_ok() checks")
>>
>> -Kees
>>
>
> Yep, we get a nice bug:
>
> [   34.783912] ------------[ cut here ]------------
> [   34.784484] kernel BUG at kernel/exit.c:1614!

Awesome! :)

I wonder how hard it might be to make this happen on x86 too (or
generically). Hmmm

-Kees

> [   34.785016] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> [   34.785572] Modules linked in:
> [   34.786177] CPU: 0 PID: 1324 Comm: a.out Not tainted 4.14.0-rc5-00005-ga3bb7b0f72d3 #69
> [   34.786657] Hardware name: linux,dummy-virt (DT)
> [   34.787093] task: ffff80003c4ed400 task.stack: ffff00000ade0000
> [   34.788196] PC is at SyS_waitid+0x1d4/0x210
> [   34.788534] LR is at SyS_waitid+0x20/0x210
> [   34.788839] pc : [<ffff0000080cde1c>] lr : [<ffff0000080cdc68>] pstate: a0000145
> [   34.789310] sp : ffff00000ade3e00
> [   34.789578] x29: ffff00000ade3e00 x28: ffff80003c4ed400
> [   34.790039] x27: ffff0000089e1000 x26: 000000000000005f
> [   34.790397] x25: 0000000000000124 x24: 0000000000000015
> [   34.790649] x23: 0000000080000000 x22: 0000ffffb1eb6b24
> [   34.790897] x21: 00000000ffffffff x20: 000080003600c000
> [   34.791149] x19: ffff800000000000 x18: 0000000000000007
> [   34.791397] x17: 0000000000000001 x16: 0000000000000019
> [   34.791648] x15: 0000000000000033 x14: 000000000000004c
> [   34.791903] x13: 0000000000000068 x12: ffff000008af69d0
> [   34.792156] x11: ffff00000ade3c20 x10: 0000000000000000
> [   34.792451] x9 : 0000000000000000 x8 : 0000000000000000
> [   34.792706] x7 : 0000000000000000 x6 : ffff000008f42f18
> [   34.792965] x5 : dead000000000100 x4 : 0000000000000011
> [   34.793214] x3 : ffff80003c4ed400 x2 : ffff800000000004
> [   34.793462] x1 : 0001000000000000 x0 : 0000000000000000
> [   34.793743] Process a.out (pid: 1324, stack limit = 0xffff00000ade0000)
> [   34.794098] Call trace:
> [   34.794351] Exception stack(0xffff00000ade3cc0 to 0xffff00000ade3e00)
> [   34.794722] 3cc0: 0000000000000000 0001000000000000 ffff800000000004 ffff80003c4ed400
> [   34.795034] 3ce0: 0000000000000011 dead000000000100 ffff000008f42f18 0000000000000000
> [   34.795297] 3d00: 0000000000000000 0000000000000000 0000000000000000 ffff00000ade3c20
> [   34.795549] 3d20: ffff000008af69d0 0000000000000068 000000000000004c 0000000000000033
> [   34.795803] 3d40: 0000000000000019 0000000000000001 0000000000000007 ffff800000000000
> [   34.796066] 3d60: 000080003600c000 00000000ffffffff 0000ffffb1eb6b24 0000000080000000
> [   34.796277] 3d80: 0000000000000015 0000000000000124 000000000000005f ffff0000089e1000
> [   34.796477] 3da0: ffff80003c4ed400 ffff00000ade3e00 ffff0000080cdc68 ffff00000ade3e00
> [   34.796677] 3dc0: ffff0000080cde1c 00000000a0000145 0000000000000000 ffff80003c4ed400
> [   34.796884] 3de0: 0001000000000000 dead000000000100 ffff00000ade3e00 ffff0000080cde1c
> [   34.797153] [<ffff0000080cde1c>] SyS_waitid+0x1d4/0x210
> [   34.797298] Exception stack(0xffff00000ade3ec0 to 0xffff00000ade4000)
> [   34.797470] 3ec0: 0000000000000000 0000000000000000 ffff800000000000 0000000000000004
> [   34.797670] 3ee0: 0000000000000000 0400000055550400 0000ffffb1e0a011 0000000000000cbf
> [   34.797873] 3f00: 000000000000005f 0000000000000a3b 0000000000000000 16170f120a1a1311
> [   34.798073] 3f20: 00000000000001a8 0000ffffb1f8ecb8 0000ffffb1e1c0e0 0000000000000002
> [   34.798272] 3f40: 0000ffffb1eb6af0 0000000000420018 0000000000000000 0000000000400740
> [   34.798471] 3f60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [   34.798668] 3f80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [   34.798870] 3fa0: 0000000000000000 0000fffff7294200 00000000004006fc 0000fffff7294200
> [   34.799068] 3fc0: 0000ffffb1eb6b24 0000000080000000 0000000000000000 000000000000005f
> [   34.799265] 3fe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [   34.799474] [<ffff0000080837b0>] el0_svc_naked+0x24/0x28
> [   34.799715] Code: f9400fb4 17ffff9a d503201f f9000fb4 (d4210000)
> [   34.800121] ---[ end trace a14ca5cd5d8f9b30 ]---



-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks

[Laura Abbott]

On 11/01/2017 04:29 PM, Kees Cook wrote:
> On Wed, Nov 1, 2017 at 4:05 PM, Laura Abbott <labbott@redhat.com> wrote:
>> On 11/01/2017 03:28 PM, Kees Cook wrote:
>>> On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott <labbott@redhat.com> wrote:
>>>> On 11/01/2017 05:05 AM, Mark Rutland wrote:
>>>>> On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
>>>>>> On 10/26/2017 02:09 AM, Mark Rutland wrote:
>>>>>>> In Prague, Kees mentioned that it would be nice to have a mechanism to
>>>>>>> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
>>>>>>> issue with unsafe_put_user() in waitid().
>>>>>>>
>>>>>>> These patches allow an optional access_ok() check to be dropped in
>>>>>>> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
>>>>>>> user pointer is passed (which should only happen in the absence of an
>>>>>>> earlier access_ok() check).
>>>>>
>>>>>> Turning on the option fails as soon as we hit userspace. On my buildroot
>>>>>> based environment I get the help text for ld.so (????) and then a message
>>>>>> about attempting to kill init.
>>>>>
>>>>> Ouch. Thanks for the report, and sorry about this.
>>>>>
>>>>> The problem is that I evaluate the ptr argument twice in
>>>>> __{get,put}_user(), and this may have side effects.
>>>>>
>>>>> e.g. when the ELF loader does things like:
>>>>>
>>>>>   __put_user((elf_addr_t)p, sp++)
>>>>>
>>>>> ... we increment sp twice, and write to the wrong user address, leaving
>>>>> sp corrupt.
>>>>>
>>>>> I have an additional patch [1] to fix this, which is in my
>>>>> arm64/access-ok branch [2].
>>>>>
>>>>> Thanks,
>>>>> Mark.
>>>>>
>>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/access-ok&id=ebb7ff83eb53b8810395d5cf48712a4ae6d678543
>>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/access-ok
>>>>>
>>>>
>>>> Thanks, the updated patch works. I wrote an LKDTM test to verify
>>>> the expected behavior (__{get,put}_user panic whereas {get,put}_user
>>>> do not). You're welcome to add Tested-by or I can wait for v2.
>>>
>>> Nice. :) Out of curiosity, can you check if this correctly BUG()s on a
>>> waitid() call when the fixes are reverted?
>>>
>>> 96ca579a1ecc ("waitid(): Avoid unbalanced user_access_end() on
>>> access_ok() error")
>>> 1c9fec470b81 ("waitid(): Add missing access_ok() checks")
>>>
>>> -Kees
>>>
>>
>> Yep, we get a nice bug:
>>
>> [   34.783912] ------------[ cut here ]------------
>> [   34.784484] kernel BUG at kernel/exit.c:1614!
> 
> Awesome! :)
> 
> I wonder how hard it might be to make this happen on x86 too (or
> generically). Hmmm
x86 looks like it needs the same ptr_argument fixup as arm64 but
seems to have a separate unsafe path so it's actually easier to
fix up. I have version of this that seems to work so I'll clean
it up and send it out tomorrow.

Thanks,
Laura

[RFC PATCH 1/2] x86: Avoid multiple evaluations in __{get,put}_user_size

[Laura Abbott]

Currently __{get,put}_user_size() expand their ptr argument in several
places, and some callers pass in expressions with side effects.

For example, fs/binfmt_elf.c, passes sp++ as the ptr argument to a chain
of __put_user() calls.

So far this isn't a problem, as each of these uses is mutually
exclusive. However, in subsequent patches we will need to make use of
the ptr argument several times, and ensure that we evaluate the ptr
expression exactly once to avoid corrupting the pointer.

In preparation for such uses, this patch reorganises
__{get,put}_user_size to evaluate the ptr argument into a temporary
__{gu,pu}_ptr variable, ensuring that side-effects occur exaclty once.

There should be no functional change as a result of this patch.

Based on work done for arm64 by Mark Rutland.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
This is setup patch for checking __{get,put}_user on x86 based on
Mark Rutland's work for arm64
lkml.kernel.org/r/<20171026090942.7041-1-mark.rutland@arm.com>
---
 arch/x86/include/asm/uaccess.h | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 574dff4d2913..d23fb5844404 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -275,21 +275,25 @@ extern void __put_user_8(void);
 
 #define __put_user_size(x, ptr, size, retval, errret)			\
 do {									\
+	typeof(ptr) __pu_ptr = (ptr);					\
 	retval = 0;							\
-	__chk_user_ptr(ptr);						\
+	__chk_user_ptr(__pu_ptr);					\
 	switch (size) {							\
 	case 1:								\
-		__put_user_asm(x, ptr, retval, "b", "b", "iq", errret);	\
+		__put_user_asm(x, __pu_ptr, retval, "b", "b", "iq",	\
+				errret);				\
 		break;							\
 	case 2:								\
-		__put_user_asm(x, ptr, retval, "w", "w", "ir", errret);	\
+		__put_user_asm(x, __pu_ptr, retval, "w", "w", "ir",	\
+				errret);				\
 		break;							\
 	case 4:								\
-		__put_user_asm(x, ptr, retval, "l", "k", "ir", errret);	\
+		__put_user_asm(x, __pu_ptr, retval, "l", "k", "ir",	\
+				errret);				\
 		break;							\
 	case 8:								\
-		__put_user_asm_u64((__typeof__(*ptr))(x), ptr, retval,	\
-				   errret);				\
+		__put_user_asm_u64((__typeof__(*ptr))(x), __pu_ptr,	\
+				retval,	\ errret);			\
 		break;							\
 	default:							\
 		__put_user_bad();					\
@@ -352,20 +356,24 @@ do {									\
 
 #define __get_user_size(x, ptr, size, retval, errret)			\
 do {									\
+	typeof(ptr) __gu_ptr = (ptr);					\
 	retval = 0;							\
-	__chk_user_ptr(ptr);						\
+	__chk_user_ptr(__gu_ptr);					\
 	switch (size) {							\
 	case 1:								\
-		__get_user_asm(x, ptr, retval, "b", "b", "=q", errret);	\
+		__get_user_asm(x, __gu_ptr, retval, "b", "b", "=q",	\
+				errret);				\
 		break;							\
 	case 2:								\
-		__get_user_asm(x, ptr, retval, "w", "w", "=r", errret);	\
+		__get_user_asm(x, __gu_ptr, retval, "w", "w", "=r",	\
+				errret);				\
 		break;							\
 	case 4:								\
-		__get_user_asm(x, ptr, retval, "l", "k", "=r", errret);	\
+		__get_user_asm(x, __gu_ptr, retval, "l", "k", "=r",	\
+				errret);				\
 		break;							\
 	case 8:								\
-		__get_user_asm_u64(x, ptr, retval, errret);		\
+		__get_user_asm_u64(x, __gu_ptr, retval, errret);	\
 		break;							\
 	default:							\
 		(x) = __get_user_bad();					\
-- 
2.13.5

[RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Laura Abbott]

__{get,put}_user calls are designed to be fast and have no checks,
relying on the caller to have made the appropriate calls previously.
It's very easy to forget a check though, leaving the kernel vulnerable
to exploits. Add an option to do the checks and kill the kernel if it
catches something bad.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
This is the actual implemtation for __{get,put}_user on x86 based on
Mark Rutland's work for arm66
lkml.kernel.org/r/<20171026090942.7041-1-mark.rutland@arm.com>

x86 turns out to be easier since the safe and unsafe paths are mostly
disjoint so we don't have to worry about gcc optimizing out access_ok.
I tweaked the Kconfig to someting a bit more generic.

The size increase was ~8K in text with a config I tested.
---
 arch/x86/Kconfig               |  3 +++
 arch/x86/include/asm/uaccess.h | 11 ++++++++++-
 security/Kconfig               | 11 +++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2fdb23313dd5..10c6e150a91e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -261,6 +261,9 @@ config RWSEM_XCHGADD_ALGORITHM
 config GENERIC_CALIBRATE_DELAY
 	def_bool y
 
+config ARCH_HAS_PARANOID_UACCESS
+	def_bool y
+
 config ARCH_HAS_CPU_RELAX
 	def_bool y
 
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index d23fb5844404..767febe1c720 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -132,6 +132,13 @@ extern int __get_user_bad(void);
 #define __inttype(x) \
 __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
+
+#define verify_uaccess(dir, ptr)                                        \
+({                                                                      \
+        if (IS_ENABLED(CONFIG_PARANOID_UACCESS))                  \
+                BUG_ON(!access_ok(dir, (ptr), sizeof(*(ptr))));         \
+})
+
 /**
  * get_user: - Get a simple variable from user space.
  * @x:   Variable to store result.
@@ -278,6 +285,7 @@ do {									\
 	typeof(ptr) __pu_ptr = (ptr);					\
 	retval = 0;							\
 	__chk_user_ptr(__pu_ptr);					\
+	verify_uaccess(VERIFY_WRITE, __pu_ptr);				\
 	switch (size) {							\
 	case 1:								\
 		__put_user_asm(x, __pu_ptr, retval, "b", "b", "iq",	\
@@ -293,7 +301,7 @@ do {									\
 		break;							\
 	case 8:								\
 		__put_user_asm_u64((__typeof__(*ptr))(x), __pu_ptr,	\
-				retval,	\ errret);			\
+				retval,	errret);			\
 		break;							\
 	default:							\
 		__put_user_bad();					\
@@ -359,6 +367,7 @@ do {									\
 	typeof(ptr) __gu_ptr = (ptr);					\
 	retval = 0;							\
 	__chk_user_ptr(__gu_ptr);					\
+	verify_uaccess(VERIFY_READ, __gu_ptr);				\
 	switch (size) {							\
 	case 1:								\
 		__get_user_asm(x, __gu_ptr, retval, "b", "b", "=q",	\
diff --git a/security/Kconfig b/security/Kconfig
index e8e449444e65..0a9ec1a4e86b 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -205,6 +205,17 @@ config STATIC_USERMODEHELPER_PATH
 	  If you wish for all usermode helper programs to be disabled,
 	  specify an empty string here (i.e. "").
 
+config PARANOID_UACCESS
+	bool "Use paranoid uaccess primitives"
+	depends on ARCH_HAS_PARANOID_UACCESS
+	help
+	  Forces access_ok() checks in __get_user(), __put_user(), and other
+	  low-level uaccess primitives which usually do not have checks. This
+	  can limit the effect of missing access_ok() checks in higher-level
+	  primitives, with a runtime performance overhead in some cases and a
+	  small code size overhead.
+
+
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
-- 
2.13.5

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 4:04 PM, Laura Abbott <labbott@redhat.com> wrote:
> __{get,put}_user calls are designed to be fast and have no checks,
> relying on the caller to have made the appropriate calls previously.
> It's very easy to forget a check though, leaving the kernel vulnerable
> to exploits. Add an option to do the checks and kill the kernel if it
> catches something bad.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> This is the actual implemtation for __{get,put}_user on x86 based on
> Mark Rutland's work for arm66
> lkml.kernel.org/r/<20171026090942.7041-1-mark.rutland@arm.com>
>
> x86 turns out to be easier since the safe and unsafe paths are mostly
> disjoint so we don't have to worry about gcc optimizing out access_ok.
> I tweaked the Kconfig to someting a bit more generic.
>
> The size increase was ~8K in text with a config I tested.

Specifically, this feature would have caught the waitid() bug in 4.13
immediately.

> ---
>  arch/x86/Kconfig               |  3 +++
>  arch/x86/include/asm/uaccess.h | 11 ++++++++++-
>  security/Kconfig               | 11 +++++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2fdb23313dd5..10c6e150a91e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -261,6 +261,9 @@ config RWSEM_XCHGADD_ALGORITHM
>  config GENERIC_CALIBRATE_DELAY
>         def_bool y
>
> +config ARCH_HAS_PARANOID_UACCESS
> +       def_bool y
> +
>  config ARCH_HAS_CPU_RELAX
>         def_bool y
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index d23fb5844404..767febe1c720 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -132,6 +132,13 @@ extern int __get_user_bad(void);
>  #define __inttype(x) \
>  __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>
> +
> +#define verify_uaccess(dir, ptr)                                        \
> +({                                                                      \
> +        if (IS_ENABLED(CONFIG_PARANOID_UACCESS))                  \
> +                BUG_ON(!access_ok(dir, (ptr), sizeof(*(ptr))));         \
> +})
> +
>  /**
>   * get_user: - Get a simple variable from user space.
>   * @x:   Variable to store result.
> @@ -278,6 +285,7 @@ do {                                                                        \
>         typeof(ptr) __pu_ptr = (ptr);                                   \
>         retval = 0;                                                     \
>         __chk_user_ptr(__pu_ptr);                                       \
> +       verify_uaccess(VERIFY_WRITE, __pu_ptr);                         \
>         switch (size) {                                                 \
>         case 1:                                                         \
>                 __put_user_asm(x, __pu_ptr, retval, "b", "b", "iq",     \
> @@ -293,7 +301,7 @@ do {                                                                        \
>                 break;                                                  \
>         case 8:                                                         \
>                 __put_user_asm_u64((__typeof__(*ptr))(x), __pu_ptr,     \
> -                               retval, \ errret);                      \
> +                               retval, errret);                        \
>                 break;                                                  \
>         default:                                                        \
>                 __put_user_bad();                                       \

Which tree is this against? I don't see the weird line break in my tree?

> @@ -359,6 +367,7 @@ do {                                                                        \
>         typeof(ptr) __gu_ptr = (ptr);                                   \
>         retval = 0;                                                     \
>         __chk_user_ptr(__gu_ptr);                                       \
> +       verify_uaccess(VERIFY_READ, __gu_ptr);                          \
>         switch (size) {                                                 \
>         case 1:                                                         \
>                 __get_user_asm(x, __gu_ptr, retval, "b", "b", "=q",     \

Does __put/get_user_size_ex() need additions too? (And does
put/get_user_ex() lack access_ok() checks as-is? Looks like the users
are have access_ok() checks, but that naming really shouldn't be
aliased to "put/get_user_ex" -- otherwise it gives the impression it's
doing access_ok() checks...)

> diff --git a/security/Kconfig b/security/Kconfig
> index e8e449444e65..0a9ec1a4e86b 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -205,6 +205,17 @@ config STATIC_USERMODEHELPER_PATH
>           If you wish for all usermode helper programs to be disabled,
>           specify an empty string here (i.e. "").
>
> +config PARANOID_UACCESS
> +       bool "Use paranoid uaccess primitives"
> +       depends on ARCH_HAS_PARANOID_UACCESS
> +       help
> +         Forces access_ok() checks in __get_user(), __put_user(), and other
> +         low-level uaccess primitives which usually do not have checks. This
> +         can limit the effect of missing access_ok() checks in higher-level
> +         primitives, with a runtime performance overhead in some cases and a
> +         small code size overhead.
> +
> +
>  source security/selinux/Kconfig
>  source security/smack/Kconfig
>  source security/tomoyo/Kconfig
> --
> 2.13.5
>

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Al Viro]

On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > x86 turns out to be easier since the safe and unsafe paths are mostly
> > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > I tweaked the Kconfig to someting a bit more generic.
> >
> > The size increase was ~8K in text with a config I tested.
> 
> Specifically, this feature would have caught the waitid() bug in 4.13
> immediately.

You mean, as soon as waitid() was given a kernel address.  At which point
you'd get a shiny way to generate a BUG(), and if something like that
happened under a mutex - it's even more fun...

> > +config PARANOID_UACCESS
> > +       bool "Use paranoid uaccess primitives"
> > +       depends on ARCH_HAS_PARANOID_UACCESS
> > +       help
> > +         Forces access_ok() checks in __get_user(), __put_user(), and other
> > +         low-level uaccess primitives which usually do not have checks. This
> > +         can limit the effect of missing access_ok() checks in higher-level
> > +         primitives, with a runtime performance overhead in some cases and a
> > +         small code size overhead.

IMO that's the wrong way to go - what we need is to reduce the amount of
__get_user()/__put_user(), rather than "instrumenting" them that way.

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Al Viro]

On Sat, Nov 04, 2017 at 12:24:30AM +0000, Al Viro wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > > x86 turns out to be easier since the safe and unsafe paths are mostly
> > > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > > I tweaked the Kconfig to someting a bit more generic.
> > >
> > > The size increase was ~8K in text with a config I tested.
> > 
> > Specifically, this feature would have caught the waitid() bug in 4.13
> > immediately.
> 
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...
> 
> > > +config PARANOID_UACCESS
> > > +       bool "Use paranoid uaccess primitives"
> > > +       depends on ARCH_HAS_PARANOID_UACCESS
> > > +       help
> > > +         Forces access_ok() checks in __get_user(), __put_user(), and other
> > > +         low-level uaccess primitives which usually do not have checks. This
> > > +         can limit the effect of missing access_ok() checks in higher-level
> > > +         primitives, with a runtime performance overhead in some cases and a
> > > +         small code size overhead.
> 
> IMO that's the wrong way to go - what we need is to reduce the amount of
> __get_user()/__put_user(), rather than "instrumenting" them that way.

FWIW, unsafe variants ought to be encapsulated in as few places as possible.
And that includes both unsafe_... and __... stuff.  waitid() had been a dumb
fuckup (by me) and it should've been done as

static int waitid_put_siginfo(struct siginfo __user *si, struct waitid_info *info,
				int signo)
{
	if (!si)
		return 0;
	if (!access_ok(VERIFY_WRITE, si, sizeof(struct siginfo)))
		return -EFAULT;
        user_access_begin();
        unsafe_put_user(signo, &si->si_signo, Efault);
        unsafe_put_user(0, &si->si_errno, Efault);
        unsafe_put_user(info->cause, &si->si_code, Efault);
        unsafe_put_user(info->pid, &si->si_pid, Efault);
        unsafe_put_user(info->uid, &si->si_uid, Efault);
        unsafe_put_user(info->status, &si->si_status, Efault);
        user_access_end();
        return 0;
Efault:
        user_access_end();
        return -EFAULT;
}

instead, rather than mixing it with the rest.  Basically, any unsafe... or __...
should be
	* used as little as possible
	* accompanied by access_ok() in the same function
	* not mixed with other stuff within the same function

We are obviously not there yet, but __get_user()/__put_user() *are* getting killed
off.

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 5:24 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
>> > x86 turns out to be easier since the safe and unsafe paths are mostly
>> > disjoint so we don't have to worry about gcc optimizing out access_ok.
>> > I tweaked the Kconfig to someting a bit more generic.
>> >
>> > The size increase was ~8K in text with a config I tested.
>>
>> Specifically, this feature would have caught the waitid() bug in 4.13
>> immediately.
>
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...

Nope, any usage at all would BUG. This would have been immediately noticed. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Kees Cook]

On Fri, Nov 3, 2017 at 6:39 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Nov 3, 2017 at 5:24 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
>>> > x86 turns out to be easier since the safe and unsafe paths are mostly
>>> > disjoint so we don't have to worry about gcc optimizing out access_ok.
>>> > I tweaked the Kconfig to someting a bit more generic.
>>> >
>>> > The size increase was ~8K in text with a config I tested.
>>>
>>> Specifically, this feature would have caught the waitid() bug in 4.13
>>> immediately.
>>
>> You mean, as soon as waitid() was given a kernel address.  At which point
>> you'd get a shiny way to generate a BUG(), and if something like that
>> happened under a mutex - it's even more fun...
>
> Nope, any usage at all would BUG. This would have been immediately noticed. :)

Sorry, ignore that; yes, on any kernel address. But as always
reduction of impact is important: from exploitable flaw to DoS. Much
better!

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Mark Rutland]

On Sat, Nov 04, 2017 at 12:24:30AM +0000, Al Viro wrote:
> On Fri, Nov 03, 2017 at 05:14:05PM -0700, Kees Cook wrote:
> > > x86 turns out to be easier since the safe and unsafe paths are mostly
> > > disjoint so we don't have to worry about gcc optimizing out access_ok.
> > > I tweaked the Kconfig to someting a bit more generic.
> > >
> > > The size increase was ~8K in text with a config I tested.
> > 
> > Specifically, this feature would have caught the waitid() bug in 4.13
> > immediately.
> 
> You mean, as soon as waitid() was given a kernel address.  At which point
> you'd get a shiny way to generate a BUG(), and if something like that
> happened under a mutex - it's even more fun...

FWIW, on the arm64 version I'd used BUG() since my initial focus was fuzzing.

We can make this safe for hardening by returning -EFAULT instead.

> > > +config PARANOID_UACCESS
> > > +       bool "Use paranoid uaccess primitives"
> > > +       depends on ARCH_HAS_PARANOID_UACCESS
> > > +       help
> > > +         Forces access_ok() checks in __get_user(), __put_user(), and other
> > > +         low-level uaccess primitives which usually do not have checks. This
> > > +         can limit the effect of missing access_ok() checks in higher-level
> > > +         primitives, with a runtime performance overhead in some cases and a
> > > +         small code size overhead.
> 
> IMO that's the wrong way to go - what we need is to reduce the amount of
> __get_user()/__put_user(), rather than "instrumenting" them that way.

Certainly that's where we want to get to.

However, in the mean time, I'd argue that there's little downside to providing
the option to check the nominally unchecked primitives, provided this is
optional.

In investigating this for arm/arm64, I found at least one other bug. I would
not be surprised to find that I had missed others, nor would I be surprised to
find that new bugs get introduced in future.

Thanks,
Mark.

Re: [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user

[Laura Abbott]

On 11/03/2017 05:14 PM, Kees Cook wrote:
> On Fri, Nov 3, 2017 at 4:04 PM, Laura Abbott <labbott@redhat.com> wrote:
>> __{get,put}_user calls are designed to be fast and have no checks,
>> relying on the caller to have made the appropriate calls previously.
>> It's very easy to forget a check though, leaving the kernel vulnerable
>> to exploits. Add an option to do the checks and kill the kernel if it
>> catches something bad.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> This is the actual implemtation for __{get,put}_user on x86 based on
>> Mark Rutland's work for arm66
>> lkml.kernel.org/r/<20171026090942.7041-1-mark.rutland@arm.com>
>>
>> x86 turns out to be easier since the safe and unsafe paths are mostly
>> disjoint so we don't have to worry about gcc optimizing out access_ok.
>> I tweaked the Kconfig to someting a bit more generic.
>>
>> The size increase was ~8K in text with a config I tested.
> 
> Specifically, this feature would have caught the waitid() bug in 4.13
> immediately.
> 
>> ---
>>  arch/x86/Kconfig               |  3 +++
>>  arch/x86/include/asm/uaccess.h | 11 ++++++++++-
>>  security/Kconfig               | 11 +++++++++++
>>  3 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 2fdb23313dd5..10c6e150a91e 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -261,6 +261,9 @@ config RWSEM_XCHGADD_ALGORITHM
>>  config GENERIC_CALIBRATE_DELAY
>>         def_bool y
>>
>> +config ARCH_HAS_PARANOID_UACCESS
>> +       def_bool y
>> +
>>  config ARCH_HAS_CPU_RELAX
>>         def_bool y
>>
>> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
>> index d23fb5844404..767febe1c720 100644
>> --- a/arch/x86/include/asm/uaccess.h
>> +++ b/arch/x86/include/asm/uaccess.h
>> @@ -132,6 +132,13 @@ extern int __get_user_bad(void);
>>  #define __inttype(x) \
>>  __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>>
>> +
>> +#define verify_uaccess(dir, ptr)                                        \
>> +({                                                                      \
>> +        if (IS_ENABLED(CONFIG_PARANOID_UACCESS))                  \
>> +                BUG_ON(!access_ok(dir, (ptr), sizeof(*(ptr))));         \
>> +})
>> +
>>  /**
>>   * get_user: - Get a simple variable from user space.
>>   * @x:   Variable to store result.
>> @@ -278,6 +285,7 @@ do {                                                                        \
>>         typeof(ptr) __pu_ptr = (ptr);                                   \
>>         retval = 0;                                                     \
>>         __chk_user_ptr(__pu_ptr);                                       \
>> +       verify_uaccess(VERIFY_WRITE, __pu_ptr);                         \
>>         switch (size) {                                                 \
>>         case 1:                                                         \
>>                 __put_user_asm(x, __pu_ptr, retval, "b", "b", "iq",     \
>> @@ -293,7 +301,7 @@ do {                                                                        \
>>                 break;                                                  \
>>         case 8:                                                         \
>>                 __put_user_asm_u64((__typeof__(*ptr))(x), __pu_ptr,     \
>> -                               retval, \ errret);                      \
>> +                               retval, errret);                        \
>>                 break;                                                  \
>>         default:                                                        \
>>                 __put_user_bad();                                       \
> 
> Which tree is this against? I don't see the weird line break in my tree?
> 

Uggggh I meant to fold this into the previous patch.

>> @@ -359,6 +367,7 @@ do {                                                                        \
>>         typeof(ptr) __gu_ptr = (ptr);                                   \
>>         retval = 0;                                                     \
>>         __chk_user_ptr(__gu_ptr);                                       \
>> +       verify_uaccess(VERIFY_READ, __gu_ptr);                          \
>>         switch (size) {                                                 \
>>         case 1:                                                         \
>>                 __get_user_asm(x, __gu_ptr, retval, "b", "b", "=q",     \
> 
> Does __put/get_user_size_ex() need additions too? (And does
> put/get_user_ex() lack access_ok() checks as-is? Looks like the users
> are have access_ok() checks, but that naming really shouldn't be
> aliased to "put/get_user_ex" -- otherwise it gives the impression it's
> doing access_ok() checks...)
> 

Possibly? A better approach might be to add the check to uaccess_try
which is where all the users already are. The users of these APIs are
pretty limited and I doubt they'd get used randomly.

Thanks,
Laura

Re: [RFC PATCH 1/2] arm64: write __range_ok() in C

[Will Deacon]

On Thu, Oct 26, 2017 at 10:09:41AM +0100, Mark Rutland wrote:
> Currently arm64's __range_ok() is written in assembly for efficiency.
> 
> This hides the logic from the compiler, preventing the compiler from
> making some optimizations, such as re-ordering instructions or folding
> multiple calls to __range_ok().
> 
> This patch uses GCC's __builtin_uaddl_overflow() to provide an
> equivalent, efficient check, while giving the compiler the visibility it
> needs to optimize the check. In testing with v4.14-rc5 using the Linaro
> 17.05 GCC 6.3.1 toolchain, this has no impact on the kernel Image size,
> (but results in a smaller vmlinux).
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/include/asm/uaccess.h | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index fc0f9eb66039..36f84ec92b9d 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -70,17 +70,20 @@ static inline void set_fs(mm_segment_t fs)
>   *
>   * This needs 65-bit arithmetic.
>   */
> +static bool __range_ok_c(unsigned long addr, unsigned long size)
> +{
> +	unsigned long result;
> +
> +	if (__builtin_uaddl_overflow(addr, size, &result))

I'm not sure if you're planning to revisit this series, but thought I'd
give you a heads up that apparently GCC 4.x doesn't have support for this
builtin, so we'll need to carry the asm at least for that toolchain.

Will

Re: [RFC PATCH 1/2] arm64: write __range_ok() in C

[Mark Rutland]

On Thu, Nov 16, 2017 at 03:28:19PM +0000, Will Deacon wrote:
> On Thu, Oct 26, 2017 at 10:09:41AM +0100, Mark Rutland wrote:
> > +static bool __range_ok_c(unsigned long addr, unsigned long size)
> > +{
> > +	unsigned long result;
> > +
> > +	if (__builtin_uaddl_overflow(addr, size, &result))
> 
> I'm not sure if you're planning to revisit this series, but thought I'd
> give you a heads up that apparently GCC 4.x doesn't have support for this
> builtin, so we'll need to carry the asm at least for that toolchain.

Thanks for the heads-up. I see my Linaro 14.09 GCC 4.9 generates an
out-of-line call to a __builtin_uaddl_overflow helper.

We can avoid the builtin, and write the test in C instead, e.g.

static inline bool __range_ok_c(unsigned long addr, unsigned long size)
{
        unsigned long end = addr + size;

        if (end < addr)
                return false;

        return end <= current_thread_info()->addr_limit;
}

... in my standalone test-case, that generates code that's almost
identical to the builtin, except that the compiler chooses to look at a
different flag.

Thanks,
Mark.