From: Russell Currey <ruscur@russell.cc>
To: Christophe Leroy <christophe.leroy@c-s.fr>,
linuxppc-dev@lists.ozlabs.org
Cc: mikey@neuling.org, mpe@ellerman.id.au, benh@kernel.crashing.org,
npiggin@gmail.com, kernel-hardening@lists.openwall.com
Subject: Re: [PATCH v2 2/3] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm()
Date: Wed, 20 Feb 2019 22:57:59 +1100 [thread overview]
Message-ID: <f4fc28e2f6807ef0669d1de8a72fcb5f5b5cfc43.camel@russell.cc> (raw)
In-Reply-To: <5ff8b24e-a748-19d3-8651-b626dd676ea4@c-s.fr>
On Fri, 2019-01-25 at 12:45 +0100, Christophe Leroy wrote:
> Hi Russel,
>
> Le 17/12/2018 à 08:09, Christophe Leroy a écrit :
> > Hi Russel,
> >
> > Le 10/12/2018 à 08:00, Russell Currey a écrit :
> > > __patch_instruction() is called in early boot, and uses
> > > __put_user_size(), which includes the locks and unlocks for KUAP,
> > > which could either be called too early, or in the Radix case,
> > > forced to
> > > use "early_" versions of functions just to safely handle this one
> > > case.
> >
> > Looking at x86, I see that __put_user_size() doesn't includes the
> > locks.
> > The lock/unlock is do by callers. I'll do the same.
> >
> >
> > > __put_user_asm() does not do this, and thus is safe to use both
> > > in early
> > > boot, and later on since in this case it should only ever be
> > > touching
> > > kernel memory.
> > >
> > > __patch_instruction() was previously refactored to use
> > > __put_user_size()
> > > in order to be able to return -EFAULT, which would allow the
> > > kernel to
> > > patch instructions in userspace, which should never happen. This
> > > has
> > > the functional change of causing faults on userspace addresses if
> > > KUAP
> > > is turned on, which should never happen in practice.
> > >
> > > A future enhancement could be to double check the patch address
> > > is
> > > definitely allowed to be tampered with by the kernel.
> >
> > This makes me realise that we are calling lock_user_access() with
> > kernel
> > addresses. That most likely breaks protection on kernel addresses
> > for
> > book3s/32. I'll have to work around it.
> >
> > Another thing I realised also is that get_user() at least is called
> > in
> > some exceptions/trap handlers. Which means it can be called nested
> > with
> > an ongoing user access. It means that get_paca()-
> > >user_access_allowed
> > might be modified during those exceptions/traps.
>
> Any comment about that ? Isn't it a problem ?
Yes, I think so. I wonder why I haven't hit this issue, though. Which
handlers is this an issue with?
Maybe we could do something like...
unlock_user_access() checks if user access is already unlocked (== 1),
if so sets user_access_allowed to 2
lock_user_access() sees that user_access_allowed is 2, and knows it's
nested and sets user_access_allowed back to 1 instead of its usual 0.
...that's pretty gross, though. It also means that every
implementation has to figure out how to cope with that.
I've done a lot of testing where a) user access hasn't been left
unlocked and b) faults haven't happened where they shouldn't, so I
wonder how I could try and hit such a case.
Could also have a get_user() without locking that's only allowed to be
used by exception handlers...
I dunno. Open to better ideas.
- Russell
>
> Christophe
>
> > Christophe
> >
> > > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > > ---
> > > arch/powerpc/lib/code-patching.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/powerpc/lib/code-patching.c
> > > b/arch/powerpc/lib/code-patching.c
> > > index 89502cbccb1b..15e8c6339960 100644
> > > --- a/arch/powerpc/lib/code-patching.c
> > > +++ b/arch/powerpc/lib/code-patching.c
> > > @@ -26,9 +26,9 @@
> > > static int __patch_instruction(unsigned int *exec_addr,
> > > unsigned int
> > > instr,
> > > unsigned int *patch_addr)
> > > {
> > > - int err;
> > > + int err = 0;
> > > - __put_user_size(instr, patch_addr, 4, err);
> > > + __put_user_asm(instr, patch_addr, err, "stw");
> > > if (err)
> > > return err;
> > >
next prev parent reply other threads:[~2019-02-20 11:57 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-10 7:00 [PATCH v2 0/3] [PATCH v2 0/3] Kernel Userspace Protection for Radix MMU Russell Currey
2018-12-10 7:00 ` [PATCH v2 1/3] powerpc/mm/radix: Use KUEP API " Russell Currey
2018-12-10 7:00 ` [PATCH v2 2/3] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm() Russell Currey
2018-12-17 7:09 ` Christophe Leroy
2019-01-25 11:45 ` Christophe Leroy
2019-02-20 11:57 ` Russell Currey [this message]
2018-12-10 7:00 ` [PATCH v2 3/3] powerpc/64s: Implement KUAP for Radix MMU Russell Currey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f4fc28e2f6807ef0669d1de8a72fcb5f5b5cfc43.camel@russell.cc \
--to=ruscur@russell.cc \
--cc=benh@kernel.crashing.org \
--cc=christophe.leroy@c-s.fr \
--cc=kernel-hardening@lists.openwall.com \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mikey@neuling.org \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).