Re: objtool clac/stac handling change..
From: Christophe Leroy
Date: Fri Jul 03 2020 - 01:27:47 EST
Le 03/07/2020 Ã 05:17, Michael Ellerman a ÃcritÂ:
Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
Le 02/07/2020 Ã 15:34, Michael Ellerman a ÃcritÂ:
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
On Wed, Jul 1, 2020 at 12:59 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
On Wed, Jul 01, 2020 at 12:04:36PM -0700, Linus Torvalds wrote:
That's actually for the access granting. Shutting the access down ends
up always doing the same thing anyway..
#define user_read_access_end prevent_current_read_from_user
#define user_write_access_end prevent_current_write_to_user
static inline void prevent_current_read_from_user(void)
{
prevent_user_access(NULL, NULL, ~0UL, KUAP_CURRENT_READ);
}
static inline void prevent_current_write_to_user(void)
{
prevent_user_access(NULL, NULL, ~0UL, KUAP_CURRENT_WRITE);
}
and prevent_user_access() has instances that do care about the direction...
Go and look closer.
There are three cases:
(a) the 32-bit book3s case. It looks like it cares, but when you look
closer, it ends up not caring about the read side, and saving the
"which address to I allow user writes to" in current->thread.kuap
(b) the nohash 32-bit case - doesn't care
(c) the 64-bit books case - doesn't care
So yes, in the (a) case it does make a difference between reads and
writes, but at least as far as I can tell, it ignores the read case,
and has code to avoid the unnecessary "disable user writes" case when
there was only a read enable done.
Yeah that's my understanding too.
Christophe is the expert on that code so I'll defer to him if I'm wrong.
Now, it's possible that I'm wrong, but the upshot of that is that even
on powerpc, I think that if we just made the rule be that "taking a
user exception should automatically do the 'user_access_end()' for us"
is trivial.
I think we can do something to make it work.
We don't have an equivalent of x86's ex_handler_uaccess(), so it's not
quite as easy as whacking a user_access_end() in there.
Isn't it something easy to do in bad_page_fault() ?
We'd need to do it there at least.
But I'm not convinced that's the only place we'd need to do it. We could
theoretically take a machine check on a user access, and those are
handled differently on each sub-(sub-sub)-platform, and I think all or
most of them don't call bad_page_fault().
Indeed, it needs to be done everywhere we do
regs->nip = extable_fixup(entry)
There are half a dozen of places that do that, in additional of
bad_page_fault() that's mainly machine checks, also kprobe.
I think we can create a fixup_exception() function which takes regs and
entry as parameters and does the nip fixup and kuap closuse.
Not exactly a call to user_access_end() but altering regs->kuap so that
user access is not restored on exception exit.
Yes.
Probably the simplest option for us is to just handle it in our
unsafe_op_wrap(). I'll try and come up with something tomorrow.
unsafe_op_wrap() is not used anymore for unsafe_put_user() as we are now
using asm goto.
Sure, but we could change it back to use unsafe_op_wrap().
But the whole purpose of using goto in unsafe_???_user() is to allow the
use of asm goto. See explanations in commit
https://github.com/linuxppc/linux/commit/1bd4403d86a1c06cb6cc9ac87664a0c9d3413d51#diff-eba084de047bb8a9087dac10c06f44bc
I did a quick hack to do that and see no difference in the generated
code, but your commit adding put_user_goto() did show better code
generation, so possibly it depends on compiler version, or my example
wasn't complicated enough (filldir()).
Yes as explained above it should remove the error checking in the caller
so your exemple was most likely too trivial.
Christophe