Received-SPF: pass (google.com: domain of linux-kernel+bounces-75622-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240213040747.1745939-1-kevinloughlin@google.com>
 <170785457569.2934648.10119965441921727215@amd.com> <CAGdbjmJAE5Uo5LA=hkMFwp+LdnNxxKgSwgkpdWZ69kwS-tLkFw@mail.gmail.com>
 <20240216225019.upcxwd6vkxv5dp4n@amd.com>
In-Reply-To: <20240216225019.upcxwd6vkxv5dp4n@amd.com>
From: Kevin Loughlin <kevinloughlin@google.com>
Date: Wed, 21 Feb 2024 14:50:00 -0800
Message-ID: <CAGdbjmK9WoOQSbKUvcTdLJDW-RB=qe2tHFDZ-MeD266xZOxh7w@mail.gmail.com>
Subject: Re: [PATCH] x86/kernel: Validate ROM before DMI scanning when SEV-SNP
 is active
To: Michael Roth <michael.roth@amd.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>, Ard Biesheuvel <ardb@kernel.org>, 
	Baoquan He <bhe@redhat.com>, Borislav Petkov <bp@alien8.de>, Brijesh Singh <brijesh.singh@amd.com>, 
	Dave Hansen <dave.hansen@linux.intel.com>, Dionna Glaze <dionnaglaze@google.com>, 
	"H.Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, Josh Poimboeuf <jpoimboe@kernel.org>, 
	Kai Huang <kai.huang@intel.com>, Peter Zijlstra <peterz@infradead.org>, 
	Ross Lagerwall <ross.lagerwall@citrix.com>, Thomas Gleixner <tglx@linutronix.de>, 
	Tom Lendacky <thomas.lendacky@amd.com>, Yuntao Wang <ytcoode@gmail.com>, 
	linux-kernel@vger.kernel.org, x86@kernel.org, 
	Adam Dunlap <acdunlap@google.com>, Peter Gonda <pgonda@google.com>, Jacob Xu <jacobhxu@google.com>, 
	Sidharth Telang <sidtelang@google.com>, Conrad Grobler <grobler@google.com>, 
	Andri Saar <andrisaar@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 16, 2024 at 2:50=E2=80=AFPM Michael Roth <michael.roth@amd.com>=
 wrote:
>
> On Tue, Feb 13, 2024 at 03:10:46PM -0800, Kevin Loughlin wrote:
> > On Tue, Feb 13, 2024 at 12:03=E2=80=AFPM Michael Roth <michael.roth@amd=
com> wrote:
> > >
> > > Quoting Kevin Loughlin (2024-02-12 22:07:46)
> > > > SEV-SNP requires encrypted memory to be validated before access. Th=
e
> > > > kernel is responsible for validating the ROM memory range because t=
he
> > > > 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 i=
n
> > > > 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 (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=
.
> > >
> > > AFAIK, QEMU doesn't actually include any legacy ROMs as part of the i=
nitial
> > > encrypted guest image, and I'm not aware of any VMM implementations t=
hat
> > > 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 cont=
ain
> > > ciphertext rather than the expected ROM data. Does that agree with th=
e
> > > 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 th=
e
> 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 f=
or
> > > 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@google.co=
m/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/com=
mit/?id=3D1c811d403afd73f04bde82b83b24c754011bd0e8

>   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.