linux-kernel - Re: [PATCH] sysrq: Use panic() to force a crash

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOd=X1K4Gm0GyoDpmPTrTPQ_6+Nvzg9J8y4gdy-R=67_72A@mail.gmail.com>
Date:   Wed, 19 Sep 2018 10:59:51 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>, jslaby@...e.com,
        LKML <linux-kernel@...r.kernel.org>, evgreen@...omium.org,
        saiprakash.ranjan@...eaurora.org,
        Doug Anderson <dianders@...omium.org>, swboyd@...omium.org,
        manojgupta@...omium.org, ani@...sta.com
Subject: Re: [PATCH] sysrq: Use panic() to force a crash

On Tue, Sep 18, 2018 at 5:32 PM Matthias Kaehlcke <mka@...omium.org> wrote:
>
> sysrq_handle_crash() currently forces a crash by dereferencing a
> NULL pointer, which is undefined behavior in C. Just call panic()
> instead, which is simpler and doesn't depend on compiler specific
> handling of the undefined behavior.
>
> Suggested-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> ---
> Not sure if it is strictly needed to release the RCU read lock now
> that panic() is invoked directly (I couldn't repro the warning
> without rcu_read_unlock()), but since this is a forced crash it
> seems good practice to keep doing it.
>
> The commit that added rcu_read_unlock() and the comment is:
>
> commit 984cf355aeaa8f2eda3861b50d0e8d3e3f77e83b
> Author: Ani Sinha <ani@...sta.com>
> Date:   Thu Dec 17 17:15:10 2015 -0800
>
>     sysrq: Fix warning in sysrq generated crash.
>
>     Commit 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq") replaced
>     spin_lock_irqsave() calls with rcu_read_lock() calls in sysrq. Since
>     rcu_read_lock() does not disable preemption, faulthandler_disabled() in
>     __do_page_fault() in x86/fault.c returns false. When the code later calls
>     might_sleep() in the pagefault handler, we get the following warning:
>
>     BUG: sleeping function called from invalid context at ../arch/x86/mm/fault.c:1187
>     in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
>     Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
>
>     To fix this, we release the RCU read lock before we crash.
>
>     Tested this patch on linux 3.18 by booting off one of our boards.
> ---
>  drivers/tty/sysrq.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 06ed20dd01ba..d779a51499a0 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -134,17 +134,10 @@ static struct sysrq_key_op sysrq_unraw_op = {
>
>  static void sysrq_handle_crash(int key)
>  {
> -       char *killer = NULL;
> -
> -       /* we need to release the RCU read lock here,
> -        * otherwise we get an annoying
> -        * 'BUG: sleeping function called from invalid context'
> -        * complaint from the kernel before the panic.
> -        */
> +       /* release the RCU read lock before crashing */

The comment probably could have stayed as is; folks will have to get
context from git blame on the line immediately below now; while you
added context in the patch file, it's below the line so wont be part
of the commit message.

>         rcu_read_unlock();
> -       panic_on_oops = 1;      /* force panic */
> -       wmb();
> -       *killer = 1;
> +
> +       panic("sysrq triggered crash\n");

Otherwise this part looks good. Maybe GKH can apply just this part
rather than a v2 (if we even care about git blame on comments)?
Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>

>  }
>  static struct sysrq_key_op sysrq_crash_op = {
>         .handler        = sysrq_handle_crash,
> --
> 2.19.0.397.gdd90340f6a-goog
>


-- 
Thanks,
~Nick Desaulniers