[PATCH 0/5] Guarded Userspace Access Prevention on Radix
LEROY Christophe
christophe.leroy at c-s.fr
Thu Nov 1 03:58:45 AEDT 2018
Russell Currey <ruscur at russell.cc> a écrit :
> On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote:
>> Russell Currey <ruscur at russell.cc> a écrit :
>>
>> > Guarded Userspace Access Prevention is a security mechanism that
>> > prevents
>> > the kernel from being able to read and write userspace addresses
>> > outside of
>> > the allowed paths, most commonly copy_{to/from}_user().
>> >
>> > At present, the only CPU that supports this is POWER9, and only
>> > while using
>> > the Radix MMU. Privileged reads and writes cannot access user data
>> > when
>> > key 0 of the AMR is set. This is described in the "Radix Tree
>> > Translation
>> > Storage Protection" section of the POWER ISA as of version 3.0.
>>
>> It is not right that only power9 can support that.
>
> It's true that not only P9 can support it, but there are more
> considerations under hash than radix, implementing this for radix is a
> first step.
I don't know much about hash, but I was talking about the 8xx which is
a nohash ppc32. I'll see next week if I can do something with it on
top of your serie.
>
>>
>> The 8xx has mmu access protection registers which can serve the
>> same
>> purpose. Today on the 8xx kernel space is group 0 and user space is
>> group 1. Group 0 is set to "page defined access permission" in
>> MD_AP
>> and MI_AP registers, and group 1 is set to "all accesses done with
>> supervisor rights". By setting group 1 to "user and supervisor
>> interpretation swapped" we can forbid kernel access to user space
>> while still allowing user access to it. Then by simply changing
>> group
>> 1 mode at dedicated places we can lock/unlock kernel access to user
>> space.
>>
>> Could you implement something as generic as possible having that in
>> mind for a future patch ?
>
> I don't think anything in this series is particularly problematic in
> relation to future work for hash. I am interested in doing a hash
> implementation in future too.
I think we have to look at that carrefuly to avoid uggly ifdefs
Christophe
>
> - Russell
>
>>
>> Christophe
>>
>> > GUAP code sets key 0 of the AMR (thus disabling accesses of user
>> > data)
>> > early during boot, and only ever "unlocks" access prior to certain
>> > operations, like copy_{to/from}_user(), futex ops, etc. Setting
>> > this does
>> > not prevent unprivileged access, so userspace can operate fine
>> > while access
>> > is locked.
>> >
>> > There is a performance impact, although I don't consider it
>> > heavy. Running
>> > a worst-case benchmark of a 1GB copy 1 byte at a time (and thus
>> > constant
>> > read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower
>> > than
>> > when disabled. In most cases, the difference is negligible. The
>> > main
>> > performance impact is the mtspr instruction, which is quite slow.
>> >
>> > There are a few caveats with this series that could be improved
>> > upon in
>> > future. Right now there is no saving and restoring of the AMR
>> > value -
>> > there is no userspace exploitation of the AMR on Radix in POWER9,
>> > but if
>> > this were to change in future, saving and restoring the value would
>> > be
>> > necessary.
>> >
>> > No attempt to optimise cases of repeated calls - for example, if
>> > some
>> > code was repeatedly calling copy_to_user() for small sizes very
>> > frequently,
>> > it would be slower than the equivalent of wrapping that code in an
>> > unlock
>> > and lock and only having to modify the AMR once.
>> >
>> > There are some interesting cases that I've attempted to handle,
>> > such as if
>> > the AMR is unlocked (i.e. because a copy_{to_from}_user is in
>> > progress)...
>> >
>> > - and an exception is taken, the kernel would then be running
>> > with the
>> > AMR unlocked and freely able to access userspace again. I am
>> > working
>> > around this by storing a flag in the PACA to indicate if the
>> > AMR is
>> > unlocked (to save a costly SPR read), and if so, locking the
>> > AMR in
>> > the exception entry path and unlocking it on the way out.
>> >
>> > - and gets context switched out, goes into a path that locks
>> > the AMR,
>> > then context switches back, access will be disabled and will
>> > fault.
>> > As a result, I context switch the AMR between tasks as if it
>> > was used
>> > by userspace like hash (which already implements this).
>> >
>> > Another consideration is use of the isync instruction. Without an
>> > isync
>> > following the mtspr instruction, there is no guarantee that the
>> > change
>> > takes effect. The issue is that isync is very slow, and so I tried
>> > to
>> > avoid them wherever necessary. In this series, the only place an
>> > isync
>> > gets used is after *unlocking* the AMR, because if an access takes
>> > place
>> > and access is still prevented, the kernel will fault.
>> >
>> > On the flipside, a slight delay in unlocking caused by skipping an
>> > isync
>> > potentially allows a small window of vulnerability. It is my
>> > opinion
>> > that this window is practically impossible to exploit, but if
>> > someone
>> > thinks otherwise, please do share.
>> >
>> > This series is my first attempt at POWER assembly so all feedback
>> > is very
>> > welcome.
>> >
>> > The official theme song of this series can be found here:
>> > https://www.youtube.com/watch?v=QjTrnKAcYjE
>> >
>> > Russell Currey (5):
>> > powerpc/64s: Guarded Userspace Access Prevention
>> > powerpc/futex: GUAP support for futex ops
>> > powerpc/lib: checksum GUAP support
>> > powerpc/64s: Disable GUAP with nosmap option
>> > powerpc/64s: Document that PPC supports nosmap
>> >
>> > .../admin-guide/kernel-parameters.txt | 2 +-
>> > arch/powerpc/include/asm/exception-64e.h | 3 +
>> > arch/powerpc/include/asm/exception-64s.h | 19 ++++++-
>> > arch/powerpc/include/asm/futex.h | 6 ++
>> > arch/powerpc/include/asm/mmu.h | 7 +++
>> > arch/powerpc/include/asm/paca.h | 3 +
>> > arch/powerpc/include/asm/reg.h | 1 +
>> > arch/powerpc/include/asm/uaccess.h | 57
>> > ++++++++++++++++---
>> > arch/powerpc/kernel/asm-offsets.c | 1 +
>> > arch/powerpc/kernel/dt_cpu_ftrs.c | 4 ++
>> > arch/powerpc/kernel/entry_64.S | 17 +++++-
>> > arch/powerpc/lib/checksum_wrappers.c | 6 +-
>> > arch/powerpc/mm/fault.c | 9 +++
>> > arch/powerpc/mm/init_64.c | 15 +++++
>> > arch/powerpc/mm/pgtable-radix.c | 2 +
>> > arch/powerpc/mm/pkeys.c | 7 ++-
>> > arch/powerpc/platforms/Kconfig.cputype | 15 +++++
>> > 17 files changed, 158 insertions(+), 16 deletions(-)
>> >
>> > --
>> > 2.19.1
>>
>>
More information about the Linuxppc-dev
mailing list