Received-SPF: pass (google.com: domain of linux-kernel+bounces-97279-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <CAGdbjmK9WoOQSbKUvcTdLJDW-RB=qe2tHFDZ-MeD266xZOxh7w@mail.gmail.com>
 <20240222202404.36206-1-kevinloughlin@google.com> <20240229165458.GLZeC24r5_-joAqNLp@fat_crate.local>
In-Reply-To: <20240229165458.GLZeC24r5_-joAqNLp@fat_crate.local>
From: Kevin Loughlin <kevinloughlin@google.com>
Date: Fri, 8 Mar 2024 11:14:27 -0500
Message-ID: <CAGdbjmLog2vU-enar+CDKU5wzeDqToib9QHVs6Q=N8XUOD9TcQ@mail.gmail.com>
Subject: Re: [PATCH v2] x86/kernel: skip ROM range scans and validation for
 SEV-SNP guests
To: Borislav Petkov <bp@alien8.de>
Cc: acdunlap@google.com, alexander.shishkin@linux.intel.com, 
	andrisaar@google.com, ardb@kernel.org, bhe@redhat.com, brijesh.singh@amd.com, 
	dave.hansen@linux.intel.com, dionnaglaze@google.com, grobler@google.com, 
	hpa@zytor.com, jacobhxu@google.com, jpoimboe@kernel.org, kai.huang@intel.com, 
	linux-kernel@vger.kernel.org, michael.roth@amd.com, mingo@redhat.com, 
	peterz@infradead.org, pgonda@google.com, ross.lagerwall@citrix.com, 
	sidtelang@google.com, tglx@linutronix.de, thomas.lendacky@amd.com, 
	x86@kernel.org, ytcoode@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 29, 2024 at 11:55=E2=80=AFAM Borislav Petkov <bp@alien8.de> wro=
te:
>
> On Thu, Feb 22, 2024 at 08:24:04PM +0000, Kevin Loughlin 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 not
> > pre-validated by the BIOS. Therefore, if a SEV-SNP guest kernel wishes
> > to access this range, the guest must first validate the range.
> >
> > The current SEV-SNP code does indeed scan the ROM range during early
> > boot and thus attempts to validate the ROM range in probe_roms().
> > However, this behavior is neither necessary nor sufficient.
>
> Why is this not necessary, all of a sudden?
>
> > With regards to sufficiency, if EFI_CONFIG_TABLES are not enabled and
> > CONFIG_DMI_SCAN_MACHINE_NON_EFI_FALLBACK is set, the kernel will
>
> What is that use case exactly?
>
> CONFIG_DMI_... is usually enabled but the absence of EFI_CONFIG_TABLES
> tells me that you're booting some guest with some special OVMF which
> doesn't sport such tables.
>
> Why?
>
> /me scrolls upthread
>
> Aha, some project oak thing doing a minimal fw. I can see why but this
> should be explained here as to why is this a relevant use case and what
> it is using and so on so that future readers can piece it all together.

Will do in v3 commit message, thanks.

>
> > 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.
> >
> > With regards to necessity, SEV-SNP guests currently read garbage (which
> > 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 early
> > boot (where the PMD is marked as encrypted due to potentially mapping
> > actually-encrypted data in other PMD-contained ranges).
>
> I don't mind ripping that ROM probing thing but that thread we're on
> here talks more about why it could be problematic to keep doing so so
> pls summarize that here.
>
> A commit should contain all arguments for why it has been arrived at
> the decision to do it this way.

Ditto.

>
> > While one solution would be to overhaul the early PMD mapping to treat
> > the ROM region of the PMD as unencrypted, SEV-SNP guests do not rely on
> > data from the legacy ROM region during early boot (nor can they
> > currently, since the data would be garbage that changes across boots).
>
> That's better.
>
> > As such, this patch opts for the simpler approach of skipping the ROM
>
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
>
> Also, do
>
> $ git grep 'This patch' Documentation/process
>
> for more details.

Ack, will fix.

>
> > 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 range
>   ^^^^^^^^^^
>
> Please introduce a spellchecker into your patch creation workflow.

Woops, thanks. I'll fix that.

>
> > validation is avoided by simply not accessing the ROM range.
> >
> > Fixes: 9704c07bf9f7 ("x86/kernel: Validate ROM memory before accessing =
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/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 long ba=
se, 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 re=
gion,
> > +      * as this memory is not pre-validated and would thus cause a cra=
sh.
> > +      */
> > +     if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && base < 0x100000 && =
base + length >=3D 0xC0000)
> > +             return 0;
>
> I don't like spreading around CoCo checks everywhere around the tree.
>
> Think of a better way pls.

Will do. I'll follow up on this in a message in reply to the
subsequent discussion involving Ard.