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: iac594737:Fastmail
Date:   Tue, 13 Sep 2022 09:27:53 -0400
From:   Demi Marie Obenour <demi@invisiblethingslab.com>
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     Juergen Gross <jgross@suse.com>,
        Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
        xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2] Add support for ESRT loading under Xen
Message-ID: <YyCFb7J6cJm5Ji9M@itl-email>
References: <20220828025158.1455-1-demi@invisiblethingslab.com>
 <CAMj1kXF5eH-HE1dkAEGGZ1qfG1eRThsNK7ayWkRmaHSO36sjfA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="37mLKXCcYa6M0yBY"
Content-Disposition: inline
In-Reply-To: <CAMj1kXF5eH-HE1dkAEGGZ1qfG1eRThsNK7ayWkRmaHSO36sjfA@mail.gmail.com>
Precedence: bulk


--37mLKXCcYa6M0yBY
Content-Type: text/plain; protected-headers=v1; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Date: Tue, 13 Sep 2022 09:27:53 -0400
From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2] Add support for ESRT loading under Xen

On Mon, Sep 05, 2022 at 01:46:54PM +0200, Ard Biesheuvel wrote:
> On Sun, 28 Aug 2022 at 04:52, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > This is needed for fwupd to work in Qubes OS.
> >
>=20
> Please elaborate on:

Will do in v3.

> - the current situation

The ESRT is not available in dom0 under Xen.

> - why this is a problem

fwupd requires the ESRT to be available in dom0.  Without it, users
cannot update their firmware.

> - why your approach is a reasonable solution.

It is the approach already chosen by Xen upstream.  See below for
details.

> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > ---
> > Changes since v1:
> >
> > - Use a different type (struct xen_efi_mem_info) for memory information
> >   provided by Xen, as Xen reports it in a different way than the
> >   standard Linux functions do.
> >
> >  drivers/firmware/efi/esrt.c | 49 +++++++++++++++++++++++++++----------
> >  drivers/xen/efi.c           | 32 ++++++++++++++++++++++++++
> >  include/linux/efi.h         | 18 ++++++++++++++
> >  3 files changed, 86 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..c0fc149a838044cc16bb08a=
374a0c8ea6b7dcbff 100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -243,27 +243,50 @@ void __init efi_esrt_init(void)
> >         void *va;
> >         struct efi_system_resource_table tmpesrt;
> >         size_t size, max, entry_size, entries_size;
> > -       efi_memory_desc_t md;
> > -       int rc;
> >         phys_addr_t end;
> > -
> > -       if (!efi_enabled(EFI_MEMMAP))
> > -               return;
> > +       uint32_t type;
> >
> >         pr_debug("esrt-init: loading.\n");
> >         if (!esrt_table_exists())
> >                 return;
> >
> > -       rc =3D efi_mem_desc_lookup(efi.esrt, &md);
> > -       if (rc < 0 ||
> > -           (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> > -            md.type !=3D EFI_BOOT_SERVICES_DATA &&
> > -            md.type !=3D EFI_RUNTIME_SERVICES_DATA)) {
> > -               pr_warn("ESRT header is not in the memory map.\n");
> > +       if (efi_enabled(EFI_MEMMAP)) {
> > +               efi_memory_desc_t md;
> > +
> > +               if (efi_mem_desc_lookup(efi.esrt, &md) < 0 ||
> > +                   (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> > +                    md.type !=3D EFI_BOOT_SERVICES_DATA &&
> > +                    md.type !=3D EFI_RUNTIME_SERVICES_DATA)) {
> > +                       pr_warn("ESRT header is not in the memory map.\=
n");
> > +                       return;
> > +               }
> > +
> > +               type =3D md.type;
> > +               max =3D efi_mem_desc_end(&md);
> > +       } else if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVI=
RT)) {
> > +               struct xen_efi_mem_info info;
> > +
> > +               if (!xen_efi_mem_info_query(efi.esrt, &info)) {
> > +                       pr_warn("Failed to lookup ESRT header in Xen me=
mory map\n");
> > +                       return;
> > +               }
> > +
> > +               type =3D info.type;
> > +               max =3D info.addr + info.size;
> > +
> > +               /* Recent Xen versions relocate the ESRT to memory of t=
ype
> > +                * EfiRuntimeServicesData, which Xen will not reuse.  I=
f the ESRT
>=20
> This violates the EFI spec, which spells out very clearly that the
> ESRT must be in EfiBootServicesData memory. Why are you deviating from
> this?

Xen will freely use EfiBootServicesData memory for its own purposes
after calling ExitBootServices().  In particular, such memory may be
allocated to, and become writable by, other guests.  Since the ESRT is
(of necessity) trusted, it cannot be used by Linux unless it is
guaranteed to not be writable by other guests.

Earlier patches to Xen just reserved the region containing the ESRT in
the EFI memory map.  However, this was tricky to implement correctly and
required a new platform op to alert dom0 that the ESRT had been reserved
by Xen.  The final patch accepted upstream instead checks if the ESRT is
in EfiBootServicesData memory, and if it is, relocates it to
EfiRuntimeServicesData memory.  This allowes using existing hypercalls
to check if the ESRT has been reserved by Xen, which is exactly what
this patch does.

If I recall correctly, some firmware already allocates the ESRT from
EfiRuntimeServicesData memory, so operating systems already need to
support this case.  Furthermore, the ESRT must not be clobbered by the
OS or hypervisor, so EfiRuntimeServicesData is a more logical choice
anyway.
--=20
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

--37mLKXCcYa6M0yBY
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmMghW8ACgkQsoi1X/+c
IsFXOA/+L49ME+tYwPJiJQHmriPMx44MCB0a+MT8Q1JIsuTPZnCxWeT2gs6FN5q5
zPncQzYMOOOzyFpKsAR9Q2AHKu8qIf3qFoX1bc1Dsc36lAW3ADWtQBSRDqMfK8WL
Q4oQX2ypgW4x3v9OHhKMSwHPIWxTsxLLoh7hLjm83phikRHy7I0EZYs2AviOYsen
u04rNCH/ZitfzDimI1BEy2Fqp2f0hskkn46TObnjrz3crtZndkld8n3f7AN5bXFi
8dE5t9pHKKzGyj/R2HnDa4YV+agG5+87MW+LQrM0CgZC2aHNW5MRmmRaz/KAWOO1
B7LYbcL0WWmhAMVaDegZDeQpTSVKHYR80z3JmKXEqL59SoNE+kC7ld2FlTdeFrFs
/+vo9UxLYX7VW4FLGFh/iMnozy6wf4VK82Il1nDNCJGnsgiRXoS4DCn7FbjSyidh
STdSZ6JE/OHtqUjmY8RUlZH9jUIiLUMfHa30xXP30mvP3yQ/ulvJgp9bVC3HxioG
lM9Ik6pZD4lSfFaOtdQRf/h8zL1grd+d2SEDbfIBlKd7jkid6d5gjoPjHIEUyETq
xxPJkEWGk33t/hSLhvlrB+FEcu5Egs11e+VMHt49bveiSOdMxPN5r5DUxjcwUvFI
iV0wewWHAEfemwiX5D6H8Tmx285a5EwLGWJd/zIogQwZ2W10r2k=
=78/p
-----END PGP SIGNATURE-----

--37mLKXCcYa6M0yBY--