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

[Russell Currey]

On Wed, 2018-10-31 at 17:58 +0100, LEROY Christophe wrote:
> 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.

My small brain saw the number 8 and assumed you were talking about
POWER8, I didn't know what 8xx was until now.

Working on a refactor to make things a bit more generic, and removing
the radix name and dependency from the config option.

> 
> > > 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
> 
>