Re: [PATCH] x86/kernel: Validate ROM before DMI scanning when SEV-SNP is active

[Kevin Loughlin]

On Fri, Feb 16, 2024 at 2:50 PM Michael Roth <michael.roth@xxxxxxx> 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@amdcom> 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.

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@xxxxxxxxxx/
[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.