Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i1568416f:Fastmail
Date:   Fri, 13 Jan 2023 23:52:59 +0100
From:   Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= 
        <marmarek@invisiblethingslab.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Juergen Gross <jgross@suse.com>, linux-kernel@vger.kernel.org,
        x86@kernel.org, linux-pm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Len Brown <len.brown@intel.com>,
        Pavel Machek <pavel@ucw.cz>,
        Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        xen-devel@lists.xenproject.org
Subject: Re: [PATCH] x86/acpi: fix suspend with Xen
Message-ID: <Y8Hgy4UHxKqh0T2T@mail-itl>
References: <20230113140610.7132-1-jgross@suse.com>
 <CAJZ5v0gP_NUeQimn21tJuUjpMAOW_wFrRe4jstN13So_4_T4QQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="4m4xLSz0QPn9icEC"
Content-Disposition: inline
In-Reply-To: <CAJZ5v0gP_NUeQimn21tJuUjpMAOW_wFrRe4jstN13So_4_T4QQ@mail.gmail.com>
Precedence: bulk


--4m4xLSz0QPn9icEC
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Date: Fri, 13 Jan 2023 23:52:59 +0100
From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= <marmarek@invisiblethingslab.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Juergen Gross <jgross@suse.com>, linux-kernel@vger.kernel.org,
	x86@kernel.org, linux-pm@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>, Len Brown <len.brown@intel.com>,
	Pavel Machek <pavel@ucw.cz>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] x86/acpi: fix suspend with Xen

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 <jgross@suse.com> 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 sec=
onds) done.
> >     [  348.292457] OOM killer disabled.
> >     [  348.292462] Freezing remaining freezable tasks ... (elapsed 0.10=
4 seconds) done.
> >     [  348.396612] printk: Suspending console(s) (use no_console_suspen=
d 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: 00000=
0000000001c
> >     [  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=
=2E3-1.fc32.qubes.x86_64 #1
> >     [  348.816092] Hardware name: Star Labs StarBook/StarBook, BIOS 8.0=
1 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).
>=20
> 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=C3=B3recki <marmarek@invisiblethingsl=
ab.com>
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> > ---
> >  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 aft=
er the
> >   * system awakes from S3, e.g. for programming into the firmware wakin=
g 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) =3D 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 =3D xen_acpi_suspend_lowlevel;
> > +#ifdef CONFIG_ACPI_SLEEP
> > +               acpi_get_wakeup_address =3D xen_acpi_get_wakeup_address;
> > +#endif
> >         }
> >  }
> >  #else
> > --
> > 2.35.3
> >

--=20
Best Regards,
Marek Marczykowski-G=C3=B3recki
Invisible Things Lab

--4m4xLSz0QPn9icEC
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAmPB4MsACgkQ24/THMrX
1yw2swgAk/5GqFX39AKLVH88v8kknH2HL3VTuIzqJY8+1FDQ5c7aWElrJp/WmgGR
EaqALVjRocUTHRX6/rIyg5l7R3drsEOfNLa8tNd5jlWWXRDwHlrf+eXBTlQZs9zX
a8mLaFADPonMM4XkiPfQutk/qpklvNf2ijAEfDiwsbODHa28Uzif2ysL2ZlTF2CA
rVxwY4YY9ndRkaBSR7Y/UCMP5P/f7NtbjD2UPcZVDHl0xNp89geMBuSNVHVEcMXn
tOivOdQ6IpczX6bCME1R3ewDNud+aFznTPyW9Q+ZQmYPLKXF+nzO1lQR5a/gBxyi
y1OfQ7J0s3r6JaQYvmF6O7JCgHNQ+A==
=8flr
-----END PGP SIGNATURE-----

--4m4xLSz0QPn9icEC--