| 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
| ||
|
Message-ID: <CAJZ5v0g=ZDeNVCbR7suZzK0KsAM-4msUZSr3oyxSf0NYg2yezQ@mail.gmail.com>
Date: Wed, 16 Mar 2022 19:31:10 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: David Woodhouse <dwmw2@...radead.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
linux-pm <linux-pm@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
benh <benh@...nel.crashing.org>,
"van der Linden, Frank" <fllinden@...zon.com>,
Amit Shah <aams@...zon.com>
Subject: Re: [PATCH] PM / hibernate: Honour ACPI hardware signature by default
for virtual guests
On Fri, Mar 11, 2022 at 8:20 PM David Woodhouse <dwmw2@...radead.org> wrote:
>
> From: David Woodhouse <dwmw@...zon.co.uk>
>
> The ACPI specification says that OSPM should refuse to restore from
> hibernate if the hardware signature changes, and should boot from
> scratch. However, real BIOSes often vary the hardware signature in cases
> where we *do* want to resume from hibernate, so Linux doesn't follow the
> spec by default.
>
> However, in a virtual environment there's no reason for the VMM to vary
> the hardware signature *unless* it wants to trigger a clean reboot as
> defined by the ACPI spec. So enable the check by default if a hypervisor
> is detected.
>
> Signed-off-by: David Woodhouse <dwmw@...zon.co.uk>
> ---
>
> On Wed, 2021-12-08 at 16:07 +0100, Rafael J. Wysocki wrote:
> > On Mon, Nov 8, 2021 at 5:09 PM David Woodhouse <dwmw2@...radead.org> wrote:
> > > A follow-up patch may do this automatically for certain "known good"
> > > machines based on a DMI match, or perhaps just for all hypervisor
> > > guests since there's no good reason a hypervisor would change the
> > > hardware_signature that it exposes to guests *unless* it wants them
> > > to obey the ACPI specification.
> > >
> > > Signed-off-by: David Woodhouse <dwmw@...zon.co.uk>
> >
> > Applied as 5.17 material, sorry for the delay.
>
> Here's the threatened follow-up. I think that a blanket enablement for
> all hypervisors is sane enough; there's no reason why a virtual
> environment would vary the hardware signature *unless* it wanted to
> trigger the ACPI defined hibernate behaviour, is there?
I don't think so.
> arch/x86/kernel/acpi/sleep.c | 23 +++++++++++++++++++++--
> drivers/acpi/sleep.c | 11 +++--------
> include/linux/acpi.h | 2 +-
> 3 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 1e97f944b47d..3b7f4cdbf2e0 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -15,6 +15,7 @@
> #include <asm/desc.h>
> #include <asm/cacheflush.h>
> #include <asm/realmode.h>
> +#include <asm/hypervisor.h>
>
> #include <linux/ftrace.h>
> #include "../../realmode/rm/wakeup.h"
> @@ -140,9 +141,9 @@ static int __init acpi_sleep_setup(char *str)
> acpi_realmode_flags |= 4;
> #ifdef CONFIG_HIBERNATION
> if (strncmp(str, "s4_hwsig", 8) == 0)
> - acpi_check_s4_hw_signature(1);
> + acpi_check_s4_hw_signature = 1;
> if (strncmp(str, "s4_nohwsig", 10) == 0)
> - acpi_check_s4_hw_signature(0);
> + acpi_check_s4_hw_signature = 0;
> #endif
> if (strncmp(str, "nonvs", 5) == 0)
> acpi_nvs_nosave();
> @@ -160,3 +161,21 @@ static int __init acpi_sleep_setup(char *str)
> }
>
> __setup("acpi_sleep=", acpi_sleep_setup);
> +
> +#if defined(CONFIG_HIBERNATION) && defined(CONFIG_HYPERVISOR_GUEST)
> +static int __init init_s4_sigcheck(void)
> +{
> + /*
> + * If running on a hypervisor, honour the ACPI specification
> + * by default and trigger a clean reboot when the hardware
> + * signature in FACS is changed after hibernation.
> + */
> + if (acpi_check_s4_hw_signature == -1 &&
> + !hypervisor_is_type(X86_HYPER_NATIVE))
> + acpi_check_s4_hw_signature = 1;
> +
> + return 0;
> +}
> +/* This must happen before acpi_init() which is a subsys initcall */
> +arch_initcall(init_s4_sigcheck);
> +#endif
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index a60ff5dfed3a..4c498e1051e9 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -874,12 +874,7 @@ static inline void acpi_sleep_syscore_init(void) {}
> #ifdef CONFIG_HIBERNATION
> static unsigned long s4_hardware_signature;
> static struct acpi_table_facs *facs;
> -static int sigcheck = -1; /* Default behaviour is just to warn */
> -
> -void __init acpi_check_s4_hw_signature(int check)
> -{
> - sigcheck = check;
> -}
> +int acpi_check_s4_hw_signature = -1; /* Default behaviour is just to warn */
>
> static int acpi_hibernation_begin(pm_message_t stage)
> {
> @@ -1004,7 +999,7 @@ static void acpi_sleep_hibernate_setup(void)
> hibernation_set_ops(old_suspend_ordering ?
> &acpi_hibernation_ops_old : &acpi_hibernation_ops);
> sleep_states[ACPI_STATE_S4] = 1;
> - if (!sigcheck)
> + if (!acpi_check_s4_hw_signature)
> return;
>
> acpi_get_table(ACPI_SIG_FACS, 1, (struct acpi_table_header **)&facs);
> @@ -1016,7 +1011,7 @@ static void acpi_sleep_hibernate_setup(void)
> */
> s4_hardware_signature = facs->hardware_signature;
>
> - if (sigcheck > 0) {
> + if (acpi_check_s4_hw_signature > 0) {
> /*
> * If we're actually obeying the ACPI specification
> * then the signature is written out as part of the
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 6274758648e3..766dbcb82df1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -526,7 +526,7 @@ acpi_status acpi_release_memory(acpi_handle handle, struct resource *res,
> int acpi_resources_are_enforced(void);
>
> #ifdef CONFIG_HIBERNATION
> -void __init acpi_check_s4_hw_signature(int check);
> +extern int acpi_check_s4_hw_signature;
> #endif
>
> #ifdef CONFIG_PM_SLEEP
> --
Applied as 5.18 material, thanks!
Powered by blists - more mailing lists