On Fri, Jan 13, 2023 at 08:40:15PM +0100, Rafael J. Wysocki wrote: > On Fri, Jan 13, 2023 at 3:06 PM Juergen Gross <[email protected]> wrote: > > > > Commit f1e525009493 ("x86/boot: Skip realmode init code when running as > > Xen PV guest") missed one code path accessing real_mode_header, leading > > to dereferencing NULL when suspending the system under Xen: > > > > [ 348.284004] PM: suspend entry (deep) > > [ 348.289532] Filesystems sync: 0.005 seconds > > [ 348.291545] Freezing user space processes ... (elapsed 0.000 > > seconds) done. > > [ 348.292457] OOM killer disabled. > > [ 348.292462] Freezing remaining freezable tasks ... (elapsed 0.104 > > seconds) done. > > [ 348.396612] printk: Suspending console(s) (use no_console_suspend to > > debug) > > [ 348.749228] PM: suspend devices took 0.352 seconds > > [ 348.769713] ACPI: EC: interrupt blocked > > [ 348.816077] BUG: kernel NULL pointer dereference, address: > > 000000000000001c > > [ 348.816080] #PF: supervisor read access in kernel mode > > [ 348.816081] #PF: error_code(0x0000) - not-present page > > [ 348.816083] PGD 0 P4D 0 > > [ 348.816086] Oops: 0000 [#1] PREEMPT SMP NOPTI > > [ 348.816089] CPU: 0 PID: 6764 Comm: systemd-sleep Not tainted > > 6.1.3-1.fc32.qubes.x86_64 #1 > > [ 348.816092] Hardware name: Star Labs StarBook/StarBook, BIOS 8.01 > > 07/03/2022 > > [ 348.816093] RIP: e030:acpi_get_wakeup_address+0xc/0x20 > > > > Fix that by adding an indirection for acpi_get_wakeup_address() which > > Xen PV dom0 can use to return a dummy non-zero wakeup address (this > > address won't ever be used, as the real suspend handling is done by the > > hypervisor). > > How exactly does this help?
By not accessing calling acpi_get_wakeup_address() (with the patch
renamed to x86_acpi_get_wakeup_address()) during PV dom0 suspend, which
otherwise would access not initialized real_mode_header.
I confirm this patch fixes the issue.
> > Fixes: f1e525009493 ("x86/boot: Skip realmode init code when running as Xen
> > PV guest")
> > Reported-by: Marek Marczykowski-Górecki <[email protected]>
> > Signed-off-by: Juergen Gross <[email protected]>
> > ---
> > arch/x86/include/asm/acpi.h | 2 +-
> > arch/x86/kernel/acpi/sleep.c | 3 ++-
> > include/xen/acpi.h | 9 +++++++++
> > 3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> > index 65064d9f7fa6..137259ff8f03 100644
> > --- a/arch/x86/include/asm/acpi.h
> > +++ b/arch/x86/include/asm/acpi.h
> > @@ -61,7 +61,7 @@ static inline void acpi_disable_pci(void)
> > extern int (*acpi_suspend_lowlevel)(void);
> >
> > /* Physical address to resume after wakeup */
> > -unsigned long acpi_get_wakeup_address(void);
> > +extern unsigned long (*acpi_get_wakeup_address)(void);
> >
> > /*
> > * Check if the CPU can handle C2 and deeper
> > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> > index 3b7f4cdbf2e0..1a3cd5e24cd0 100644
> > --- a/arch/x86/kernel/acpi/sleep.c
> > +++ b/arch/x86/kernel/acpi/sleep.c
> > @@ -33,10 +33,11 @@ static char temp_stack[4096];
> > * Returns the physical address where the kernel should be resumed after
> > the
> > * system awakes from S3, e.g. for programming into the firmware waking
> > vector.
> > */
> > -unsigned long acpi_get_wakeup_address(void)
> > +static unsigned long x86_acpi_get_wakeup_address(void)
> > {
> > return ((unsigned long)(real_mode_header->wakeup_start));
> > }
> > +unsigned long (*acpi_get_wakeup_address)(void) =
> > x86_acpi_get_wakeup_address;
> >
> > /**
> > * x86_acpi_enter_sleep_state - enter sleep state
> > diff --git a/include/xen/acpi.h b/include/xen/acpi.h
> > index b1e11863144d..7e1e5dbfb77c 100644
> > --- a/include/xen/acpi.h
> > +++ b/include/xen/acpi.h
> > @@ -56,6 +56,12 @@ static inline int xen_acpi_suspend_lowlevel(void)
> > return 0;
> > }
> >
> > +static inline unsigned long xen_acpi_get_wakeup_address(void)
> > +{
> > + /* Just return a dummy non-zero value, it will never be used. */
> > + return 1;
> > +}
> > +
> > static inline void xen_acpi_sleep_register(void)
> > {
> > if (xen_initial_domain()) {
> > @@ -65,6 +71,9 @@ static inline void xen_acpi_sleep_register(void)
> > &xen_acpi_notify_hypervisor_extended_sleep);
> >
> > acpi_suspend_lowlevel = xen_acpi_suspend_lowlevel;
> > +#ifdef CONFIG_ACPI_SLEEP
> > + acpi_get_wakeup_address = xen_acpi_get_wakeup_address;
> > +#endif
> > }
> > }
> > #else
> > --
> > 2.35.3
> >
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
signature.asc
Description: PGP signature
