Received-SPF: pass (google.com: domain of linux-kernel+bounces-97603-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <CAGdbjmK9WoOQSbKUvcTdLJDW-RB=qe2tHFDZ-MeD266xZOxh7w@mail.gmail.com>
 <20240222202404.36206-1-kevinloughlin@google.com> <4463AFB9-58AC-4E65-BB04-EB2C9F2D9301@broadcom.com>
 <CAGdbjmJ+xiqycaDU9XMUEj2z70Adx-+ZCOiAFGXoNCojt_X=qA@mail.gmail.com> <20240308204151.jny3hyo6nyjqk6qt@amd.com>
In-Reply-To: <20240308204151.jny3hyo6nyjqk6qt@amd.com>
From: Kevin Loughlin <kevinloughlin@google.com>
Date: Fri, 8 Mar 2024 16:30:55 -0500
Message-ID: <CAGdbjmK=68Gte2MOvstMa_BoKxX2SEhbXeuRYpO_bdd2FY_xxw@mail.gmail.com>
Subject: Re: [PATCH v2] x86/kernel: skip ROM range scans and validation for
 SEV-SNP guests
To: Michael Roth <michael.roth@amd.com>
Cc: Mike Stunes <mike.stunes@broadcom.com>, acdunlap@google.com, 
	alexander.shishkin@linux.intel.com, andrisaar@google.com, ardb@kernel.org, 
	bhe@redhat.com, Borislav Petkov <bp@alien8.de>, Brijesh Singh <brijesh.singh@amd.com>, 
	dave.hansen@linux.intel.com, Dionna Amalie Glaze <dionnaglaze@google.com>, grobler@google.com, 
	hpa@zytor.com, jacobhxu@google.com, jpoimboe@kernel.org, 
	"Huang, Kai" <kai.huang@intel.com>, linux-kernel@vger.kernel.org, mingo@redhat.com, 
	peterz@infradead.org, Peter Gonda <pgonda@google.com>, ross.lagerwall@citrix.com, 
	sidtelang@google.com, tglx@linutronix.de, 
	Thomas Lendacky <thomas.lendacky@amd.com>, x86@kernel.org, ytcoode@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 8, 2024 at 3:44=E2=80=AFPM Michael Roth <michael.roth@amd.com> =
wrote:
>
> On Fri, Mar 08, 2024 at 11:10:43AM -0500, Kevin Loughlin wrote:
> > On Mon, Feb 26, 2024 at 2:16=E2=80=AFPM Mike Stunes <mike.stunes@broadc=
om.com> wrote:
> > >
> > > Hi,
> > >
> > > > On Feb 22, 2024, at 12:24 PM, Kevin Loughlin <kevinloughlin@google.=
com> wrote:
> > > >
> > > > SEV-SNP requires encrypted memory to be validated before access.
> > > > Because the ROM memory range is not part of the e820 table, it is n=
ot
> > > > pre-validated by the BIOS. Therefore, if a SEV-SNP guest kernel wis=
hes
> > > > to access this range, the guest must first validate the range.
> > > >
> > > > The current SEV-SNP code does indeed scan the ROM range during earl=
y
> > > > boot and thus attempts to validate the ROM range in probe_roms().
> > > > However, this behavior is neither necessary nor sufficient.
> > > >
> > > > With regards to sufficiency, if EFI_CONFIG_TABLES are not enabled a=
nd
> > > > CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK is set, the kernel will
> > > > attempt to access the memory at SMBIOS_ENTRY_POINT_SCAN_START (whic=
h
> > > > falls in the ROM range) prior to validation. The specific problemat=
ic
> > > > call chain occurs during dmi_setup() -> dmi_scan_machine() and resu=
lts
> > > > in a crash during boot if SEV-SNP is enabled under these conditions=
.
> > > >
> > > > With regards to necessity, SEV-SNP guests currently read garbage (w=
hich
> > > > changes across boots) from the ROM range, meaning these scans are
> > > > unnecessary. The guest reads garbage because the legacy ROM range
> > > > is unencrypted data but is accessed via an encrypted PMD during ear=
ly
> > > > boot (where the PMD is marked as encrypted due to potentially mappi=
ng
> > > > actually-encrypted data in other PMD-contained ranges).
> > > >
> > > > While one solution would be to overhaul the early PMD mapping to tr=
eat
> > > > the ROM region of the PMD as unencrypted, SEV-SNP guests do not rel=
y on
> > > > data from the legacy ROM region during early boot (nor can they
> > > > currently, since the data would be garbage that changes across boot=
s).
> > > > As such, this patch opts for the simpler approach of skipping the R=
OM
> > > > range scans (and the otherwise-necessary range validation) during
> > > > SEV-SNP guest early boot.
> > > >
> > > > Ultimatly, the potential SEV-SNP guest crash due to lack of ROM ran=
ge
> > > > validation is avoided by simply not accessing the ROM range.
> > > >
> > > > Fixes: 9704c07bf9f7 ("x86/kernel: Validate ROM memory before access=
ing when SEV-SNP is active")
> > > > Signed-off-by: Kevin Loughlin <kevinloughlin@google.com>
> > > > ---
> > > > arch/x86/include/asm/sev.h   |  2 --
> > > > arch/x86/kernel/mpparse.c    |  7 +++++++
> > > > arch/x86/kernel/probe_roms.c | 11 ++++-------
> > > > arch/x86/kernel/sev.c        | 15 ---------------
> > > > drivers/firmware/dmi_scan.c  |  7 ++++++-
> > > > 5 files changed, 17 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.=
h
> > > > index 5b4a1ce3d368..474c24ba0f6f 100644
> > > > --- a/arch/x86/include/asm/sev.h
> > > > +++ b/arch/x86/include/asm/sev.h
> > > > @@ -203,7 +203,6 @@ void __init early_snp_set_memory_private(unsign=
ed long vaddr, unsigned long padd
> > > > unsigned long npages);
> > > > void __init early_snp_set_memory_shared(unsigned long vaddr, unsign=
ed long paddr,
> > > > unsigned long npages);
> > > > -void __init snp_prep_memory(unsigned long paddr, unsigned int sz, =
enum psc_op op);
> > > > void snp_set_memory_shared(unsigned long vaddr, unsigned long npage=
s);
> > > > void snp_set_memory_private(unsigned long vaddr, unsigned long npag=
es);
> > > > void snp_set_wakeup_secondary_cpu(void);
> > > > @@ -227,7 +226,6 @@ static inline void __init
> > > > early_snp_set_memory_private(unsigned long vaddr, unsigned long pad=
dr, unsigned long npages) { }
> > > > static inline void __init
> > > > early_snp_set_memory_shared(unsigned long vaddr, unsigned long padd=
r, unsigned long npages) { }
> > > > -static inline void __init snp_prep_memory(unsigned long paddr, uns=
igned int sz, enum psc_op op) { }
> > > > static inline void snp_set_memory_shared(unsigned long vaddr, unsig=
ned long npages) { }
> > > > static inline void snp_set_memory_private(unsigned long vaddr, unsi=
gned long npages) { }
> > > > static inline void snp_set_wakeup_secondary_cpu(void) { }
> > > > diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> > > > index b223922248e9..39ea771e2d4c 100644
> > > > --- a/arch/x86/kernel/mpparse.c
> > > > +++ b/arch/x86/kernel/mpparse.c
> > > > @@ -553,6 +553,13 @@ static int __init smp_scan_config(unsigned lon=
g base, unsigned long length)
> > > >    base, base + length - 1);
> > > > BUILD_BUG_ON(sizeof(*mpf) !=3D 16);
> > > >
> > > > + /*
> > > > + * Skip scan in SEV-SNP guest if it would touch the legacy ROM reg=
ion,
> > > > + * as this memory is not pre-validated and would thus cause a cras=
h.
> > > > + */
> > > > + if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && base < 0x100000 && =
base + length >=3D 0xC0000)
> > > > + return 0;
> > > > +
> > > > while (length > 0) {
> > > > bp =3D early_memremap(base, length);
> > > > mpf =3D (struct mpf_intel *)bp;
> > > > diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_r=
oms.c
> > > > index 319fef37d9dc..84ff4b052fc1 100644
> > > > --- a/arch/x86/kernel/probe_roms.c
> > > > +++ b/arch/x86/kernel/probe_roms.c
> > > > @@ -204,14 +204,11 @@ void __init probe_roms(void)
> > > > int i;
> > > >
> > > > /*
> > > > - * The ROM memory range is not part of the e820 table and is there=
fore not
> > > > - * pre-validated by BIOS. The kernel page table maps the ROM regio=
n as encrypted
> > > > - * memory, and SNP requires encrypted memory to be validated befor=
e access.
> > > > - * Do that here.
> > > > + * These probes are skipped in SEV-SNP guests because the ROM rang=
e
> > > > + * is not pre-validated, meaning access would cause a crash.
> > > > */
> > > > - snp_prep_memory(video_rom_resource.start,
> > > > - ((system_rom_resource.end + 1) - video_rom_resource.start),
> > > > - SNP_PAGE_STATE_PRIVATE);
> > > > + if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> > > > + return;
> > > >
> > > > /* video rom */
> > > > upper =3D adapter_rom_resources[0].start;
> > > > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> > > > index c67285824e82..d2362631da91 100644
> > > > --- a/arch/x86/kernel/sev.c
> > > > +++ b/arch/x86/kernel/sev.c
> > > > @@ -774,21 +774,6 @@ void __init early_snp_set_memory_shared(unsign=
ed long vaddr, unsigned long paddr
> > > > early_set_pages_state(vaddr, paddr, npages, SNP_PAGE_STATE_SHARED);
> > > > }
> > > >
> > > > -void __init snp_prep_memory(unsigned long paddr, unsigned int sz, =
enum psc_op op)
> > > > -{
> > > > - unsigned long vaddr, npages;
> > > > -
> > > > - vaddr =3D (unsigned long)__va(paddr);
> > > > - npages =3D PAGE_ALIGN(sz) >> PAGE_SHIFT;
> > > > -
> > > > - if (op =3D=3D SNP_PAGE_STATE_PRIVATE)
> > > > - early_snp_set_memory_private(vaddr, paddr, npages);
> > > > - else if (op =3D=3D SNP_PAGE_STATE_SHARED)
> > > > - early_snp_set_memory_shared(vaddr, paddr, npages);
> > > > - else
> > > > - WARN(1, "invalid memory op %d\n", op);
> > > > -}
> > > > -
> > > > static unsigned long __set_pages_state(struct snp_psc_desc *data, u=
nsigned long vaddr,
> > > >       unsigned long vaddr_end, int op)
> > > > {
> > > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_sca=
n.c
> > > > index 015c95a825d3..22e27087eb5b 100644
> > > > --- a/drivers/firmware/dmi_scan.c
> > > > +++ b/drivers/firmware/dmi_scan.c
> > > > @@ -703,7 +703,12 @@ static void __init dmi_scan_machine(void)
> > > > dmi_available =3D 1;
> > > > return;
> > > > }
> > > > - } else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK)) =
{
> > > > + } else if (IS_ENABLED(CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK) &=
&
> > > > + !cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
> > > > + /*
> > > > + * This scan is skipped in SEV-SNP guests because the ROM range
> > > > + * is not pre-validated, meaning access would cause a crash.
> > > > + */
> > > > p =3D dmi_early_remap(SMBIOS_ENTRY_POINT_SCAN_START, 0x10000);
> > > > if (p =3D=3D NULL)
> > > > goto error;
> > > > --
> > > > 2.44.0.rc0.258.g7320e95886-goog
> > > >
> > > >
> > >
> > > In addition to these changes, I also had to skip pirq_find_routing_ta=
ble if SEV-SNP is active.
> >
> > Thanks. I will update this in v3.
>
> There's also another access a bit later in boot:
>
>   static __init int eisa_bus_probe(void)
>   {
>     ...
>     ioremap(0x0FFFD9, 4);
>   }
>
> This time it's via ioremap() with the encryption bit *unset*, so it
> won't necessarily cause a crash but it's inconsistent with the early
> page table having that region set as encrypted.
>
> We discussed unsetting the encryption bit in early page table with
> security folks and the general consensus was that *if* any VMM/firmware
> ever came along that does want to make use of legacy region for any reaso=
n
> (such as providing DMI/SMBIOS info) it would be safest to require that th=
ey
> encrypt the data in the region before handing off to guest kernel, so it
> makes sense to patch away unecrypted accesses to the legacy region so the
> don't cause problems down the road (like causing implicit page state
> change from private->shared and throwing away data in the region later
> in boot).

Sounds good, thanks. Since this one won't cause crashes, I will place
it in a separate patch in the series to separate (current) functional
fixes from cleanup, especially since there may be similar legacy
probes to cleanup in various types of guests. Please let me know if
you feel differently or have additional thoughts.