Date: Tue, 13 Feb 2024 14:02:55 -0600
From: Michael Roth <michael.roth@....com>
To: 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>, Kevin
 Loughlin <kevinloughlin@...gle.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>
CC: 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

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. As a result, it seems like snp_prep_rom_range() would
only result in the guest seeing ciphertext in these ranges.

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?
And perhaps dmi_setup() should similarly skip the legacy ROM ranges for
the kernel configs in question?

-Mike

> 
> This commit thus provides the simple solution of moving the ROM range
> validation from probe_roms() to before dmi_setup(), such that a SEV-SNP
> guest satisfying the above use case now successfully boots.
> 
> Fixes: 9704c07bf9f7 ("x86/kernel: Validate ROM memory before accessing when SEV-SNP is active")
> Signed-off-by: Kevin Loughlin <kevinloughlin@...gle.com>
> ---
>  arch/x86/include/asm/setup.h |  6 ++++++
>  arch/x86/kernel/probe_roms.c | 19 +++++++++----------
>  arch/x86/kernel/setup.c      | 10 ++++++++++
>  3 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index 5c83729c8e71..5c8f5b0d0f9f 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -117,6 +117,12 @@ void *extend_brk(size_t size, size_t align);
>         __section(".bss..brk") __aligned(1) __used      \
>         static char __brk_##name[size]
>  
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +void snp_prep_rom_range(void);
> +#else
> +static inline void snp_prep_rom_range(void) { }
> +#endif
> +
>  extern void probe_roms(void);
>  
>  void clear_bss(void);
> diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
> index 319fef37d9dc..83b192f5e3cc 100644
> --- a/arch/x86/kernel/probe_roms.c
> +++ b/arch/x86/kernel/probe_roms.c
> @@ -196,6 +196,15 @@ static int __init romchecksum(const unsigned char *rom, unsigned long length)
>         return !length && !sum;
>  }
>  
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +void __init snp_prep_rom_range(void)
> +{
> +       snp_prep_memory(video_rom_resource.start,
> +                       ((system_rom_resource.end + 1) - video_rom_resource.start),
> +                       SNP_PAGE_STATE_PRIVATE);
> +}
> +#endif
> +
>  void __init probe_roms(void)
>  {
>         unsigned long start, length, upper;
> @@ -203,16 +212,6 @@ void __init probe_roms(void)
>         unsigned char c;
>         int i;
>  
> -       /*
> -        * The ROM memory range is not part of the e820 table and is therefore not
> -        * pre-validated by BIOS. The kernel page table maps the ROM region as encrypted
> -        * memory, and SNP requires encrypted memory to be validated before access.
> -        * Do that here.
> -        */
> -       snp_prep_memory(video_rom_resource.start,
> -                       ((system_rom_resource.end + 1) - video_rom_resource.start),
> -                       SNP_PAGE_STATE_PRIVATE);
> -
>         /* video rom */
>         upper = adapter_rom_resources[0].start;
>         for (start = video_rom_resource.start; start < upper; start += 2048) {
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 84201071dfac..19f870728486 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -902,6 +902,16 @@ void __init setup_arch(char **cmdline_p)
>                 efi_init();
>  
>         reserve_ibft_region();
> +
> +       /*
> +        * The ROM memory range is not part of the e820 table and is therefore not
> +        * pre-validated by BIOS. The kernel page table maps the ROM region as encrypted
> +        * memory, and SNP requires encrypted memory to be validated before access.
> +        * This should be done before dmi_setup(), which may access the ROM region
> +        * even before probe_roms() is called.
> +        */
> +       snp_prep_rom_range();
> +
>         dmi_setup();
>  
>         /*
> -- 
> 2.43.0.687.g38aa6559b0-goog
> 
>

Powered by blists - more mailing lists