[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 12 Sep 2016 16:09:59 +0100
From: Mark Rutland <mark.rutland@....com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: Kees Cook <keescook@...omium.org>, kernel-hardening@...ts.openwall.com,
Will Deacon <will.deacon@....com>,
AKASHI Takahiro <takahiro.akashi@...aro.org>,
James Morse <james.morse@....com>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into
separate uaccess_* macros
On Mon, Sep 12, 2016 at 03:52:19PM +0100, Catalin Marinas wrote:
> On Mon, Sep 05, 2016 at 04:38:28PM +0100, Mark Rutland wrote:
> > On Fri, Sep 02, 2016 at 04:02:07PM +0100, Catalin Marinas wrote:
> > > /*
> > > + * User access enabling/disabling.
> > > + */
> > > +#define uaccess_disable(alt) \
> > > +do { \
> > > + asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt, \
> > > + CONFIG_ARM64_PAN)); \
> > > +} while (0)
> > > +
> > > +#define uaccess_enable(alt) \
> > > +do { \
> > > + asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt, \
> > > + CONFIG_ARM64_PAN)); \
> > > +} while (0)
> >
> > Passing the alternative down is somewhat confusing. e.g. in the futex
> > case it looks like we're only doing something when PAN is present,
> > whereas we'll manipulate TTBR0 in the absence of PAN.
>
> I agree it's confusing (I got it wrong first time as well and used the
> wrong alternative for futex).
>
> > If I've understood correctly, we need this to distinguish regular
> > load/store uaccess sequences (eg. the futex code) from potentially
> > patched unprivileged load/store sequences (e.g. {get,put}_user) when
> > poking PSTATE.PAN.
> >
> > So perhaps we could ahve something like:
> >
> > * privileged_uaccess_{enable,disable}()
> > Which toggle TTBR0, or PAN (always).
> > These would handle cases like the futex/swp code.
> >
> > * (unprivileged_)uaccess_{enable,disable}()
> > Which toggle TTBR0, or PAN (in the absence of UAO).
> > These would handle cases like the {get,put}_user sequences.
> >
> > Though perhaps that is just as confusing. ;)
>
> I find it more confusing.
Fair enough. :)
> In the non-UAO case, get_user etc. would
> normally have to use privileged_uaccess_enable() since ldr is not
> replaced with ldtr. Maybe uaccess_enable_for_exclusives() but it doesn't
> look any better.
I strongly prefer uaccess_enable_exclusives(), or something of that sort
to both of the above. ;)
> I think adding some comments to the code (uaccess_enable macro) would
> work better, clarifying what the alternative is for.
That will make things smoewhat clearer, though only after one reads the
comments. In contrast, uaccess_enable_exclusives() would be
self-documenting w.r.t. the intented use-case.
Do we ever want to use the 8.1 atomics for futexes? If so, perhaps
uaccess_enable_atomics()?
Thanks,
Mark.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
