[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 20 Feb 2019 22:57:59 +1100
From: Russell Currey <ruscur@...sell.cc>
To: Christophe Leroy <christophe.leroy@....fr>, linuxppc-dev@...ts.ozlabs.org
Cc: mikey@...ling.org, mpe@...erman.id.au, benh@...nel.crashing.org,
npiggin@...il.com, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v2 2/3] powerpc/lib: Refactor __patch_instruction() to
use __put_user_asm()
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@...sell.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;
> > >
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.
