Message-ID: <20240222202015.33zx7t2zgaev7jos@amd.com>
Date: Thu, 22 Feb 2024 14:20:15 -0600
From: Michael Roth <michael.roth@....com>
To: Kevin Loughlin <kevinloughlin@...gle.com>
CC: Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Ard Biesheuvel
	<ardb@...nel.org>, Baoquan He <bhe@...hat.com>, Borislav Petkov
	<bp@...en8.de>, Brijesh Singh <brijesh.singh@....com>, Dave Hansen
	<dave.hansen@...ux.intel.com>, Dionna Glaze <dionnaglaze@...gle.com>,
	"H.Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>, Josh
 Poimboeuf <jpoimboe@...nel.org>, Kai Huang <kai.huang@...el.com>, Peter
 Zijlstra <peterz@...radead.org>, Ross Lagerwall <ross.lagerwall@...rix.com>,
	"Thomas Gleixner" <tglx@...utronix.de>, Tom Lendacky
	<thomas.lendacky@....com>, "Yuntao Wang" <ytcoode@...il.com>,
	<linux-kernel@...r.kernel.org>, <x86@...nel.org>, Adam Dunlap
	<acdunlap@...gle.com>, Peter Gonda <pgonda@...gle.com>, Jacob Xu
	<jacobhxu@...gle.com>, Sidharth Telang <sidtelang@...gle.com>, Conrad Grobler
	<grobler@...gle.com>, Andri Saar <andrisaar@...gle.com>
Subject: Re: [PATCH] x86/kernel: Validate ROM before DMI scanning when
 SEV-SNP is active

On Wed, Feb 21, 2024 at 02:50:00PM -0800, Kevin Loughlin wrote:
> On Fri, Feb 16, 2024 at 2:50 PM Michael Roth <michael.roth@....com> wrote:
> >
> > On Tue, Feb 13, 2024 at 03:10:46PM -0800, Kevin Loughlin wrote:
> > > On Tue, Feb 13, 2024 at 12:03 PM Michael Roth <michael.roth@....com> wrote:
> > > >
> > > > Quoting Kevin Loughlin (2024-02-12 22:07:46)
> > > > > SEV-SNP requires encrypted memory to be validated before access. The
> > > > > kernel is responsible for validating the ROM memory range because the
> > > > > range is not part of the e820 table and therefore not pre-validated by
> > > > > the BIOS.
> > > > >
> > > > > While the current SEV-SNP code attempts to validate the ROM range in
> > > > > probe_roms(), this does not suffice for all existing use cases. In
> > > > > particular, if EFI_CONFIG_TABLES are not enabled and
> > > > > CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK is set, the kernel will
> > > > > attempt to access the memory at SMBIOS_ENTRY_POINT_SCAN_START (which
> > > > > falls in the ROM range) prior to validation. The specific problematic
> > > > > call chain occurs during dmi_setup() -> dmi_scan_machine() and results
> > > > > in a crash during boot if SEV-SNP is enabled under these conditions.
> > > >
> > > > AFAIK, QEMU doesn't actually include any legacy ROMs as part of the initial
> > > > encrypted guest image, and I'm not aware of any VMM implementations that
> > > > do this either.
> > >
> > > I'm using a VMM implementation that uses (non-EFI) Oak stage0 firmware [0].
> > >
> > > [0] https://github.com/project-oak/oak/tree/main/stage0_bin
> > >
> > > > If dmi_setup() similarly scans these ranges, it seems likely the same
> > > > issue would be present: the validated/private regions would only contain
> > > > ciphertext rather than the expected ROM data. Does that agree with the
> > > > behavior you are seeing?
> > > >
> > > > If so, maybe instead probe_roms should just be skipped in the case of SNP?
> > >
> > > If probe_roms() is skipped, SEV-SNP guest boot also currently crashes;
> > > I just quickly tried that (though admittedly haven't looked into why).
> >
> > default_find_smp_config() will also call smp_scan_config() on
> > 0xF0000-0x10000, so that might be the additional issue you're hitting.
> > If I skip that for in addition to probe_roms, then boot works for me.
> 
> Yeah, smp_scan_config() was the culprit. Thanks.
> 
> > It seems the currently handling has a bug that has been in place since the
> > original SEV guest code was added. If you dump the data that probe_roms()
> > sees while it is scanning for instances of ROMSIGNATURE (0xaa55) in the
> > region, you'll see that it is random data that changes on every boot.
> > The root issue is that this region does not contain encrypted data, and
> > is only being accessed that way because the early page table has the
> > encryption bit set for this range.
> >
> > The effects are subtle: if the code ever sees a pair of bytes that look
> > like ROMSIGNATURE, it will reserve that memory so it can be accessed
> > later, generally just 0xc0000-0xc7fff. In extremely rare cases where the
> > ciphertext's data has a checksum that happens to match the contents, it
> > will use a random byte, multiple it by 512, and reserve up to 64k for
> > this bogus ROM region.
> >
> > For SNP this resulted in a more obvious failure: a #VC exception because
> > the supposedly encrypted memory was in fact not encrypted, and thus not
> > PVALIDATED. Unfortunately the fix you linked to involved maintaining the
> > broken SEV behavior rather than fixing this mismatch.
> >
> > >
> > > > And perhaps dmi_setup() should similarly skip the legacy ROM ranges for
> > > > the kernel configs in question?
> > >
> > > Given (a) non-EFI firmware is supported in other SME/SEV boot code
> > > patches [2], (b) this patch does not seem to introduce significant
> > > complexity (it just moves [1] to earlier in the boot process to
> > > additionally handle the non-EFI case), and (c) skipping
> > > probe_roms()+dmi_setup() doesn't work without additional changes, I'm
> > > currently still inclined to simply validate the legacy ROM ranges
> > > early enough to prevent this issue (as is already done when using EFI
> > > firmware).
> >
> > The 2 options I see are:
> >
> >   a) Skipping accesses to these regions for SEV. It is vaguely possible
> >      some implementation out there actually did measure/load the ROM as
> >      part of the initial guest image for SEV, but for SNP this would
> >      have been impossible since it would have lead to the guest crashing
> >      when snp_prep_roms() was called, since RMPUPDATE on the host only
> >      rescinds the validated bit if there is a change to the RMP entry.
> >      If it was already assigned/private/validated then the guest code
> >      would detected that PVALIDATE resulted in no changes, and so it
> >      would have failed with PVALIDATE_FAIL_NOUPDATE. So if you want to
> >      be super sure you don't break legacy SEV implementations then you
> >      could limit the change to SNP guests where it's essentially
> >      guaranteed these regions are not being utilized in any functional
> >      way.
> 
> Based on your explanation, I agree that (at a minimum) it makes sense
> to rectify the behavior for SEV-SNP guests.
> 
> On that note, as you describe here, I skipped the 3 ROM region scans
> on platforms with CC_ATTR_GUEST_SEV_SNP (and deleted the call to
> snp_prep_memory()) and successfully booted. I can send that as v2.

Sounds good. Please add me to the Cc, happy to test/review.

> 
> Note that I have *not* tried skipping the scans for all SEV guest
> variants (CC_ATTR_GUEST_MEM_ENCRYPT) since those boots appear to be
> functioning without the change (and there is a risk of breaking the
> sorts of implementations that you described); also note that
> clang-built SEV-SNP guests still require [0] and [1] to function.
> 
> [0] https://lore.kernel.org/all/20240206223620.1833276-1-acdunlap@google.com/
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=1c811d403afd73f04bde82b83b24c754011bd0e8
> 
> >   b) Modifying the early page table setup by early_make_pgtable() to
> >      clear the encrypted bit for 0xC0000-0x100000 legacy region. The
> >      challenge there is everything is PMD-mapped at that stage of boot
> >      and there's no infrastructure for splitting page tables to handle
> >      non-2MB-aligned/sized regions.
> 
> If ever needed/desired, a slight variant of this second option might
> also be providing a temporary unencrypted mapping on the fly during
> the few times the regions are scanned during early boot, similar to
> how __sme_early_map_unmap_mem() is already used for sme_map_bootdata()
> in head64.c. I haven't tried it, but I just wanted to note it down in
> case it becomes relevant.

True, that might be another option to consider if needed.

-Mike

Powered by blists - more mailing lists