| Message ID | bb6dc00bf7974e3e75145bd91ed00070529c0a7c.camel@infradead.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] Honour ACPI hardware signature to prevent hibernation resume | expand |
On Do, 2019-02-14 at 15:48 +0100, David Woodhouse wrote: > When the hardware_signature changes, we're supposed to gracefully > decline to attempt the resume: > > "If the signature has changed, OSPM will not restore the system > context and can boot from scratch" > > Linux originally ignored the hardware signature completely. > > Starting from commit bdfe6b7c681 in July 2008, Linux changed behaviour > to resume into the original image anyway, and then panic as soon as it > got there. That made no sense. It deviates from spec without benefit. > That was changed again in commit 5c551e624ab in 2014, to just print a > warning and "success doubtful!" after resume instead of panicking, with > a note that Windows 8 doesn't refuse to resume when the signature > changes. I did that change because actual hardware and the spec deviate, arguably. There were machines whose signature depends on the lid switch. 2014 machines are still in use. If you change this, we will regress. > We actually do have a use case where we want the behaviour that the > spec requires — when resuming a hibernated guest on a hypervisor which > has become sufficiently incompatible that we don't expect it to work, > we'd really like to be able to tell it to boot cleanly, not resume. Then you should panic during the reboot. > This untested proof-of-concept patch does precisely that, putting the > hardware signature into the swsusp header and checking it in the boot > kernel before deciding to resume at all. > > Before I clean it up and actually test it, I'd welcome some opinions: > > • Do we want to pretend the signature in the swsusp_header will ever > be used for anything other than ACPI signatures, and make it larger > than a u32? No. There is just no benefit. It matches or not. 32 bit is large enough for that. > • Do we make it opt-in based on the platforms where this behaviour is > particularly desired, or opt-out based on... DMI matches of the > laptops which are known to change the signature gratuitously? You can be sure that nobody records these warnings. You will cause regressions if you change default behavior. To make this perfectly clear, I am afraid this patch is fundamentally flawed, not just in implementation, but in what it does. It pains me to say so, but in that regard there is just one viable method: Do as Windows does. Regards Oliver
On Mon, 2019-02-18 at 12:09 +0100, Oliver Neukum wrote: > > We actually do have a use case where we want the behaviour that the > > spec requires — when resuming a hibernated guest on a hypervisor which > > has become sufficiently incompatible that we don't expect it to work, > > we'd really like to be able to tell it to boot cleanly, not resume. > > Then you should panic during the reboot. ... > > • Do we make it opt-in based on the platforms where this behaviour is > > particularly desired, or opt-out based on... DMI matches of the > > laptops which are known to change the signature gratuitously? > > You can be sure that nobody records these warnings. You will cause > regressions if you change default behavior. > > To make this perfectly clear, I am afraid this patch is fundamentally > flawed, not just in implementation, but in what it does. It pains me > to say so, but in that regard there is just one viable method: > Do as Windows does. That's not really an ideal solution here. The use case is for virtual machines which are hibernated. Most of the time, they can come back just fine and the hypervisor is sufficiently compatible. Sometimes — especially if we've had to remove or change features for security reasons — we know that a given image cannot successfully resume on the new host. Letting the customer bring back their image and then follow your advice to "panic during the reboot" is not really the experience we aspire to. What you say makes sense for real hardware, but for the specific case of known types of virtual machine it would be really useful to have Linux actually follow the specification. I'll redo the patch to make it opt-in, so it doesn't affect the general case. I'll rip out the pointless warning and the entirely gratuitous command line option which enables/disables it too, while I'm at it.
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index 403c4ff15349..844e737c24eb 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -1248,8 +1248,10 @@ static void acpi_sleep_hibernate_setup(void) return; acpi_get_table(ACPI_SIG_FACS, 1, (struct acpi_table_header **)&facs); - if (facs) + if (facs) { s4_hardware_signature = facs->hardware_signature; + swsusp_hardware_signature = facs->hardware_signature; + } } #else /* !CONFIG_HIBERNATION */ static inline void acpi_sleep_hibernate_setup(void) {} diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 3f529ad9a9d2..0e830ccd67dc 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -389,6 +389,7 @@ extern unsigned long get_safe_page(gfp_t gfp_mask); extern asmlinkage int swsusp_arch_suspend(void); extern asmlinkage int swsusp_arch_resume(void); +extern u32 swsusp_hardware_signature; extern void hibernation_set_ops(const struct platform_hibernation_ops *ops); extern int hibernate(void); extern bool system_entering_hibernation(void); diff --git a/kernel/power/power.h b/kernel/power/power.h index 9e58bdc8a562..d447571484e9 100644 --- a/kernel/power/power.h +++ b/kernel/power/power.h @@ -170,6 +170,7 @@ extern int swsusp_swap_in_use(void); #define SF_PLATFORM_MODE 1 #define SF_NOCOMPRESS_MODE 2 #define SF_CRC32_MODE 4 +#define SF_HW_SIG 8 /* kernel/power/hibernate.c */ extern int swsusp_check(void); diff --git a/kernel/power/swap.c b/kernel/power/swap.c index d7f6c1a288d3..bd37784b522f 100644 --- a/kernel/power/swap.c +++ b/kernel/power/swap.c @@ -38,6 +38,8 @@ #define HIBERNATE_SIG "S1SUSPEND" +u32 swsusp_hardware_signature; + /* * When reading an {un,}compressed image, we may restore pages in place, * in which case some architectures need these pages cleaning before they @@ -106,7 +108,8 @@ struct swap_map_handle { struct swsusp_header { char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) - - sizeof(u32)]; + sizeof(u32) - sizeof(u32)]; + u32 hw_sig; u32 crc32; sector_t image; unsigned int flags; /* Flags to pass to the "boot" kernel */ @@ -315,6 +317,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags) memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10); memcpy(swsusp_header->sig, HIBERNATE_SIG, 10); swsusp_header->image = handle->first_sector; + if (swsusp_hardware_signature) { + swsusp_header->hw_sig = swsusp_hardware_signature; + flags |= SF_HW_SIG; + } swsusp_header->flags = flags; if (flags & SF_CRC32_MODE) swsusp_header->crc32 = handle->crc32; @@ -1533,6 +1539,12 @@ int swsusp_check(void) } else { error = -EINVAL; } + if (!error && swsusp_header->flags & SF_HW_SIG && + swsusp_header->hw_sig != swsusp_hardware_signature) { + pr_info("Suspend image hardware signature mismatch (%08x now %08x); aborting resume.\n", + swsusp_header->hw_sig, swsusp_hardware_signature); + error = -EINVAL; + } put: if (error)
When the hardware_signature changes, we're supposed to gracefully decline to attempt the resume: "If the signature has changed, OSPM will not restore the system context and can boot from scratch" Linux originally ignored the hardware signature completely. Starting from commit bdfe6b7c681 in July 2008, Linux changed behaviour to resume into the original image anyway, and then panic as soon as it got there. That was changed again in commit 5c551e624ab in 2014, to just print a warning and "success doubtful!" after resume instead of panicking, with a note that Windows 8 doesn't refuse to resume when the signature changes. We actually do have a use case where we want the behaviour that the spec requires — when resuming a hibernated guest on a hypervisor which has become sufficiently incompatible that we don't expect it to work, we'd really like to be able to tell it to boot cleanly, not resume. This untested proof-of-concept patch does precisely that, putting the hardware signature into the swsusp header and checking it in the boot kernel before deciding to resume at all. Before I clean it up and actually test it, I'd welcome some opinions: • Do we want to pretend the signature in the swsusp_header will ever be used for anything other than ACPI signatures, and make it larger than a u32? • Do we make it opt-in based on the platforms where this behaviour is particularly desired, or opt-out based on... DMI matches of the laptops which are known to change the signature gratuitously? • In the case where we don't refuse to resume based on a signature mismatch, should we still print the warning? Unless someone feels strongly, I'll probably go with "no" and "opt-in" as my answers to the first two of those questions, and make the third up as I go along. Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>