From: Mark Rutland <mark.rutland@arm.com> Date: 2017-10-26 09:09:54
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2017-10-26 09:10:01
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 <redacted>
Cc: Laura Abbott <redacted>
Cc: Will Deacon <redacted>
---
arch/arm64/include/asm/uaccess.h | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
From: Mark Rutland <mark.rutland@arm.com> Date: 2017-10-26 09:10:03
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 <redacted>
Cc: Laura Abbott <redacted>
Cc: Will Deacon <redacted>
---
arch/arm64/Kconfig | 9 +++++++++
arch/arm64/include/asm/uaccess.h | 8 ++++++++
2 files changed, 17 insertions(+)
From: Will Deacon <hidden> Date: 2017-10-27 15:41:14
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2017-10-28 08:38:03
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:
quoted
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.
From: Russell King - ARM Linux <linux@armlinux.org.uk> Date: 2017-10-28 08:47:45
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
From: Laura Abbott <hidden> Date: 2017-10-31 23:56:45
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2017-11-01 12:06:01
On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
On 10/26/2017 02:09 AM, Mark Rutland wrote:
quoted
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.
From: Laura Abbott <hidden> Date: 2017-11-01 21:13:50
On 11/01/2017 05:05 AM, Mark Rutland wrote:
On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
quoted
On 10/26/2017 02:09 AM, Mark Rutland wrote:
quoted
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).
quoted
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.
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
On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott [off-list ref] wrote:
On 11/01/2017 05:05 AM, Mark Rutland wrote:
quoted
On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
quoted
On 10/26/2017 02:09 AM, Mark Rutland wrote:
quoted
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).
quoted
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.
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
From: Laura Abbott <hidden> Date: 2017-11-01 23:05:47
On 11/01/2017 03:28 PM, Kees Cook wrote:
On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott [off-list ref] wrote:
quoted
On 11/01/2017 05:05 AM, Mark Rutland wrote:
quoted
On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
quoted
On 10/26/2017 02:09 AM, Mark Rutland wrote:
quoted
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).
quoted
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.
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
On Wed, Nov 1, 2017 at 4:05 PM, Laura Abbott [off-list ref] wrote:
On 11/01/2017 03:28 PM, Kees Cook wrote:
quoted
On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott [off-list ref] wrote:
quoted
On 11/01/2017 05:05 AM, Mark Rutland wrote:
quoted
On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
quoted
On 10/26/2017 02:09 AM, Mark Rutland wrote:
quoted
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).
quoted
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.
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
From: Laura Abbott <hidden> Date: 2017-11-02 01:26:10
On 11/01/2017 04:29 PM, Kees Cook wrote:
On Wed, Nov 1, 2017 at 4:05 PM, Laura Abbott [off-list ref] wrote:
quoted
On 11/01/2017 03:28 PM, Kees Cook wrote:
quoted
On Wed, Nov 1, 2017 at 2:13 PM, Laura Abbott [off-list ref] wrote:
quoted
On 11/01/2017 05:05 AM, Mark Rutland wrote:
quoted
On Tue, Oct 31, 2017 at 04:56:39PM -0700, Laura Abbott wrote:
quoted
On 10/26/2017 02:09 AM, Mark Rutland wrote:
quoted
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).
quoted
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.
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
From: Will Deacon <hidden> Date: 2017-11-16 15:28:49
On Thu, Oct 26, 2017 at 10:09:41AM +0100, Mark Rutland wrote:
quoted hunk
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 <redacted>
Cc: Laura Abbott <redacted>
Cc: Will Deacon <redacted>
---
arch/arm64/include/asm/uaccess.h | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
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
From: Mark Rutland <mark.rutland@arm.com> Date: 2017-11-20 12:22:40
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:
quoted
+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.