Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix

[LEROY Christophe <christophe.leroy@c-s.fr>]

Russell Currey <ruscur@russell.cc> a écrit :

> On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote:
>> Russell Currey <ruscur@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
>>
>>