linux-kernel - Re: [PATCH] panic/reboot: allow specifying reboot

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] [day] [month] [year] [list]
Message-ID: <CAGXu5jJxKhWQOxyOOb2kQHCdmK4XV8GvhgaHeded6vG-82AgVQ@mail.gmail.com>
Date:   Fri, 8 Feb 2019 01:05:46 +0000
From:   Kees Cook <keescook@...omium.org>
To:     Aaro Koskinen <aaro.koskinen@....fi>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Aaro Koskinen <aaro.koskinen@...ia.com>
Subject: Re: [PATCH] panic/reboot: allow specifying reboot_mode for panic only

On Thu, Feb 7, 2019 at 8:59 PM Aaro Koskinen <aaro.koskinen@....fi> wrote:
>
> From: Aaro Koskinen <aaro.koskinen@...ia.com>
>
> Allow specifying reboot_mode for panic only. This is needed on systems
> where ramoops is used to store panic logs, and user wants to use warm
> reset to preserve those, while still having cold reset on normal reboots.

Seems reasonable to me. I'd make the "-1" be a specific #define,
though. Open-coded literals should generally be avoided.

-Kees

>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@...ia.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  4 +++-
>  include/linux/reboot.h                        |  1 +
>  kernel/panic.c                                |  2 ++
>  kernel/reboot.c                               | 20 ++++++++++++++-----
>  4 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index b799bcf67d7b..033c93048a95 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3950,7 +3950,9 @@
>                                 [[,]s[mp]#### \
>                                 [[,]b[ios] | a[cpi] | k[bd] | t[riple] | e[fi] | p[ci]] \
>                                 [[,]f[orce]
> -                       Where reboot_mode is one of warm (soft) or cold (hard) or gpio,
> +                       Where reboot_mode is one of warm (soft) or cold (hard) or gpio
> +                                       (prefix with 'panic_' to set mode for panic
> +                                       reboot only),
>                               reboot_type is one of bios, acpi, kbd, triple, efi, or pci,
>                               reboot_force is either force or not specified,
>                               reboot_cpu is s[mp]#### with #### being the processor
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index e63799a6e895..61962f9826fa 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -21,6 +21,7 @@ enum reboot_mode {
>         REBOOT_GPIO,
>  };
>  extern enum reboot_mode reboot_mode;
> +extern enum reboot_mode panic_reboot_mode;
>
>  enum reboot_type {
>         BOOT_TRIPLE     = 't',
> diff --git a/kernel/panic.c b/kernel/panic.c
> index f121e6ba7e11..c4285e3a27ef 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -306,6 +306,8 @@ void panic(const char *fmt, ...)
>                  * shutting down.  But if there is a chance of
>                  * rebooting the system it will be rebooted.
>                  */
> +               if (panic_reboot_mode != -1)
> +                       reboot_mode = panic_reboot_mode;
>                 emergency_restart();
>         }
>  #ifdef __sparc__
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index e1b79b6a2735..d5627a92508d 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -31,6 +31,7 @@ EXPORT_SYMBOL(cad_pid);
>  #define DEFAULT_REBOOT_MODE
>  #endif
>  enum reboot_mode reboot_mode DEFAULT_REBOOT_MODE;
> +enum reboot_mode panic_reboot_mode = -1;
>
>  /*
>   * This variable is used privately to keep track of whether or not
> @@ -519,6 +520,8 @@ EXPORT_SYMBOL_GPL(orderly_reboot);
>  static int __init reboot_setup(char *str)
>  {
>         for (;;) {
> +               enum reboot_mode *mode;
> +
>                 /*
>                  * Having anything passed on the command line via
>                  * reboot= will cause us to disable DMI checking
> @@ -526,17 +529,24 @@ static int __init reboot_setup(char *str)
>                  */
>                 reboot_default = 0;
>
> +               if (!strncmp(str, "panic_", 6)) {
> +                       mode = &panic_reboot_mode;
> +                       str += 6;
> +               } else {
> +                       mode = &reboot_mode;
> +               }
> +
>                 switch (*str) {
>                 case 'w':
> -                       reboot_mode = REBOOT_WARM;
> +                       *mode = REBOOT_WARM;
>                         break;
>
>                 case 'c':
> -                       reboot_mode = REBOOT_COLD;
> +                       *mode = REBOOT_COLD;
>                         break;
>
>                 case 'h':
> -                       reboot_mode = REBOOT_HARD;
> +                       *mode = REBOOT_HARD;
>                         break;
>
>                 case 's':
> @@ -553,11 +563,11 @@ static int __init reboot_setup(char *str)
>                                 if (rc)
>                                         return rc;
>                         } else
> -                               reboot_mode = REBOOT_SOFT;
> +                               *mode = REBOOT_SOFT;
>                         break;
>                 }
>                 case 'g':
> -                       reboot_mode = REBOOT_GPIO;
> +                       *mode = REBOOT_GPIO;
>                         break;
>
>                 case 'b':
> --
> 2.17.0
>


-- 
Kees Cook