From: Christophe Leroy <christophe.leroy@csgroup.eu> To: Michael Ellerman <mpe@ellerman.id.au>, Linus Torvalds <torvalds@linux-foundation.org>, Al Viro <viro@zeniv.linux.org.uk> Cc: Josh Poimboeuf <jpoimboe@redhat.com>, Peter Zijlstra <peterz@infradead.org>, the arch/x86 maintainers <x86@kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org> Subject: Re: objtool clac/stac handling change.. Date: Fri, 3 Jul 2020 07:27:32 +0200 [thread overview] Message-ID: <a449e55b-7a8d-5acd-fc2f-acd1eb9ae252@csgroup.eu> (raw) In-Reply-To: <87h7up70e5.fsf@mpe.ellerman.id.au> Le 03/07/2020 à 05:17, Michael Ellerman a écrit : > Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> Le 02/07/2020 à 15:34, Michael Ellerman a écrit : >>> Linus Torvalds <torvalds@linux-foundation.org> writes: >>>> On Wed, Jul 1, 2020 at 12:59 PM Al Viro <viro@zeniv.linux.org.uk> 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
WARNING: multiple messages have this Message-ID (diff)
From: Christophe Leroy <christophe.leroy@csgroup.eu> To: Michael Ellerman <mpe@ellerman.id.au>, Linus Torvalds <torvalds@linux-foundation.org>, Al Viro <viro@zeniv.linux.org.uk> Cc: Peter Zijlstra <peterz@infradead.org>, the arch/x86 maintainers <x86@kernel.org>, "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Josh Poimboeuf <jpoimboe@redhat.com> Subject: Re: objtool clac/stac handling change.. Date: Fri, 3 Jul 2020 07:27:32 +0200 [thread overview] Message-ID: <a449e55b-7a8d-5acd-fc2f-acd1eb9ae252@csgroup.eu> (raw) In-Reply-To: <87h7up70e5.fsf@mpe.ellerman.id.au> Le 03/07/2020 à 05:17, Michael Ellerman a écrit : > Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> Le 02/07/2020 à 15:34, Michael Ellerman a écrit : >>> Linus Torvalds <torvalds@linux-foundation.org> writes: >>>> On Wed, Jul 1, 2020 at 12:59 PM Al Viro <viro@zeniv.linux.org.uk> 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
next prev parent reply other threads:[~2020-07-03 5:27 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-01 18:22 objtool clac/stac handling change Linus Torvalds 2020-07-01 18:29 ` Andy Lutomirski 2020-07-01 19:35 ` Linus Torvalds 2020-07-01 20:36 ` Andy Lutomirski 2020-07-01 20:51 ` Josh Poimboeuf 2020-07-01 21:02 ` Linus Torvalds 2020-07-02 0:00 ` Josh Poimboeuf 2020-07-02 8:05 ` Peter Zijlstra 2020-07-01 20:51 ` Linus Torvalds 2020-07-02 0:47 ` Andy Lutomirski 2020-07-02 2:30 ` Linus Torvalds 2020-07-02 2:35 ` Linus Torvalds 2020-07-02 3:08 ` Andy Lutomirski 2020-07-01 18:41 ` Al Viro 2020-07-01 19:04 ` Linus Torvalds 2020-07-01 19:59 ` Al Viro 2020-07-01 20:25 ` Linus Torvalds 2020-07-02 13:34 ` Michael Ellerman 2020-07-02 14:01 ` Al Viro 2020-07-02 14:04 ` Al Viro 2020-07-02 15:13 ` Christophe Leroy 2020-07-02 20:13 ` Linus Torvalds 2020-07-03 3:59 ` Michael Ellerman 2020-07-03 3:17 ` Michael Ellerman 2020-07-03 5:27 ` Christophe Leroy [this message] 2020-07-03 5:27 ` Christophe Leroy 2020-07-02 19:52 ` Linus Torvalds 2020-07-02 20:17 ` Al Viro 2020-07-02 20:32 ` Linus Torvalds 2020-07-02 20:59 ` Al Viro 2020-07-02 21:55 ` Linus Torvalds 2020-07-03 1:33 ` Al Viro 2020-07-03 3:32 ` Linus Torvalds 2020-07-03 21:02 ` Al Viro 2020-07-03 21:10 ` Linus Torvalds 2020-07-03 21:41 ` Andy Lutomirski 2020-07-03 22:25 ` Al Viro 2020-07-03 21:59 ` Al Viro 2020-07-03 22:04 ` Al Viro 2020-07-03 22:12 ` Al Viro 2020-07-04 0:49 ` Al Viro 2020-07-04 1:54 ` Linus Torvalds 2020-07-04 2:30 ` Al Viro 2020-07-04 3:06 ` Linus Torvalds 2020-07-04 2:11 ` Al Viro 2020-07-07 12:35 ` David Laight 2020-07-10 22:37 ` Linus Torvalds 2020-07-13 9:32 ` David Laight
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=a449e55b-7a8d-5acd-fc2f-acd1eb9ae252@csgroup.eu \ --to=christophe.leroy@csgroup.eu \ --cc=jpoimboe@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=peterz@infradead.org \ --cc=torvalds@linux-foundation.org \ --cc=viro@zeniv.linux.org.uk \ --cc=x86@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.