lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 30 Aug 2018 05:23:01 +0000 From: Nadav Amit <namit@...are.com> To: Masami Hiramatsu <mhiramat@...nel.org>, Sean Christopherson <sean.j.christopherson@...el.com> CC: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...hat.com>, X86 ML <x86@...nel.org>, Arnd Bergmann <arnd@...db.de>, linux-arch <linux-arch@...r.kernel.org>, Andy Lutomirski <luto@...nel.org>, Kees Cook <keescook@...omium.org>, Jason Wessel <jason.wessel@...driver.com> Subject: Re: [RFC PATCH 1/6] x86/alternative: assert text_mutex is taken at 7:26 PM, Masami Hiramatsu <mhiramat@...nel.org> wrote: > On Wed, 29 Aug 2018 14:00:06 -0700 > Sean Christopherson <sean.j.christopherson@...el.com> wrote: > >> On Wed, Aug 29, 2018 at 08:44:47PM +0000, Nadav Amit wrote: >>> at 1:13 PM, Sean Christopherson <sean.j.christopherson@...el.com> wrote: >>> >>>> On Wed, Aug 29, 2018 at 07:36:22PM +0000, Nadav Amit wrote: >>>>> at 10:11 AM, Nadav Amit <namit@...are.com> wrote: >>>>> >>>>>> at 1:59 AM, Masami Hiramatsu <mhiramat@...nel.org> wrote: >>>>>> >>>>>>> On Wed, 29 Aug 2018 01:11:42 -0700 >>>>>>> Nadav Amit <namit@...are.com> wrote: >>>>>>> >>>>>>>> Use lockdep to ensure that text_mutex is taken when text_poke() is >>>>>>>> called. >>>>>>>> >>>>>>>> Actually it is not always taken, specifically when it is called by kgdb, >>>>>>>> so take the lock in these cases. >>>>>>> >>>>>>> Can we really take a mutex in kgdb context? >>>>>>> >>>>>>> kgdb_arch_remove_breakpoint >>>>>>> <- dbg_deactivate_sw_breakpoints >>>>>>> <- kgdb_reenter_check >>>>>>> <- kgdb_handle_exception >>>>>>> <- __kgdb_notify >>>>>>> <- kgdb_ll_trap >>>>>>> <- do_int3 >>>>>>> <- kgdb_notify >>>>>>> <- die notifier >>>>>>> >>>>>>> kgdb_arch_set_breakpoint >>>>>>> <- dbg_activate_sw_breakpoints >>>>>>> <- kgdb_reenter_check >>>>>>> <- kgdb_handle_exception >>>>>>> ... >>>>>>> >>>>>>> Both seems called in exception context, so we can not take a mutex lock. >>>>>>> I think kgdb needs a special path. >>>>>> >>>>>> You are correct, but I don’t want a special path. Presumably text_mutex is >>>>>> guaranteed not to be taken according to the code. >>>>>> >>>>>> So I guess the only concern is lockdep. Do you see any problem if I change >>>>>> mutex_lock() into mutex_trylock()? It should always succeed, and I can add a >>>>>> warning and a failure path if it fails for some reason. >>>>> >>>>> Err.. This will not work. I think I will drop this patch, since I cannot >>>>> find a proper yet simple assertion. Creating special path just for the >>>>> assertion seems wrong. >>>> >>>> It's probably worth expanding the comment for text_poke() to call out >>>> the kgdb case and reference kgdb_arch_{set,remove}_breakpoint(), whose >>>> code and comments make it explicitly clear why its safe for them to >>>> call text_poke() without acquiring the lock. Might prevent someone >>>> from going down this path again in the future. >>> >>> I thought that the whole point of the patch was to avoid comments, and >>> instead enforce the right behavior. I don’t understand well enough kgdb >>> code, so I cannot attest it does the right thing. What happens if >>> kgdb_do_roundup==0? >> >> As is, the comment is wrong because there are obviously cases where >> text_poke() is called without text_mutex being held. I can't attest >> to the kgdb code either. My thought was to document the exception so >> that if someone does want to try and enforce the right behavior they >> can dive right into the problem instead of having to learn of the kgdb >> gotcha the hard way. Maybe a FIXME is the right approach? > > No, kgdb ensures that the text_mutex has not been held right before > calling text_poke. So they also take care the text_mutex. I guess > kgdb_arch_{set,remove}_breakpoint() is supposed to be run under > a special circumstance, like stopping all other threads/cores. > In that case, we can just check the text_mutex is not locked. I assumed so too, but after looking at the code, I am not sure that this is the case when gdb_do_roundup==0. > Anyway, kgdb is a very rare courner case. I think if CONFIG_KGDB is > enabled, lockdep and any assertion should be disabled, since kgdb > can tweak anything in the kernel with unexpected ways... Call me lazy, but I really do not want to debug syzkaller failures due to this issue (now or in the future). If the assertion is known to be incorrect, even in a corner case, I see no reason to have it and I certainly do not want to be the one that added it…
Powered by blists - more mailing lists