Skip to content
This repository was archived by the owner on Nov 8, 2023. It is now read-only.

Commit 0f4a1e8

Browse files
kevinloughlinbp3tk0v
authored andcommitted
x86/sev: Skip ROM range scans and validation for SEV-SNP guests
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 sufficient nor necessary for the following reasons: * With regards to sufficiency, 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. For example, Project Oak Stage 0 provides a minimal guest firmware that currently meets these configuration conditions, meaning guests booting atop Oak Stage 0 firmware encounter a problematic call chain during dmi_setup() -> dmi_scan_machine() that results in a crash during boot if SEV-SNP is enabled. * With regards to necessity, SEV-SNP guests generally 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). In one exceptional case, EISA probing treats the ROM range as unencrypted data, which is inconsistent with other probing. Continuing to allow SEV-SNP guests to use garbage and to inconsistently classify ROM range encryption status can trigger undesirable behavior. For instance, if garbage bytes appear to be a valid signature, memory may be unnecessarily reserved for the ROM range. Future code or other use cases may result in more problematic (arbitrary) behavior that should be avoided. While one solution would be to overhaul the early PMD mapping to always treat the ROM region of the PMD as unencrypted, SEV-SNP guests do not currently rely on data from the ROM region during early boot (and even if they did, they would be mostly relying on garbage data anyways). As a simpler solution, skip the ROM range scans (and the otherwise- necessary range validation) during SEV-SNP guest early boot. The potential SEV-SNP guest crash due to lack of ROM range validation is thus avoided by simply not accessing the ROM range. In most cases, skip the scans by overriding problematic x86_init functions during sme_early_init() to SNP-safe variants, which can be likened to x86_init overrides done for other platforms (ex: Xen); such overrides also avoid the spread of cc_platform_has() checks throughout the tree. In the exceptional EISA case, still use cc_platform_has() for the simplest change, given (1) checks for guest type (ex: Xen domain status) are already performed here, and (2) these checks occur in a subsys initcall instead of an x86_init function. [ bp: Massage commit message, remove "we"s. ] Fixes: 9704c07 ("x86/kernel: Validate ROM memory before accessing when SEV-SNP is active") Signed-off-by: Kevin Loughlin <kevinloughlin@google.com> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Cc: <stable@kernel.org> Link: https://lore.kernel.org/r/20240313121546.2964854-1-kevinloughlin@google.com
1 parent 4969d75 commit 0f4a1e8

File tree

8 files changed

+39
-31
lines changed

8 files changed

+39
-31
lines changed

arch/x86/include/asm/sev.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,12 +218,12 @@ void early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr,
218218
unsigned long npages);
219219
void early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr,
220220
unsigned long npages);
221-
void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op);
222221
void snp_set_memory_shared(unsigned long vaddr, unsigned long npages);
223222
void snp_set_memory_private(unsigned long vaddr, unsigned long npages);
224223
void snp_set_wakeup_secondary_cpu(void);
225224
bool snp_init(struct boot_params *bp);
226225
void __noreturn snp_abort(void);
226+
void snp_dmi_setup(void);
227227
int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
228228
void snp_accept_memory(phys_addr_t start, phys_addr_t end);
229229
u64 snp_get_unsupported_features(u64 status);
@@ -244,12 +244,12 @@ static inline void __init
244244
early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, unsigned long npages) { }
245245
static inline void __init
246246
early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr, unsigned long npages) { }
247-
static inline void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op) { }
248247
static inline void snp_set_memory_shared(unsigned long vaddr, unsigned long npages) { }
249248
static inline void snp_set_memory_private(unsigned long vaddr, unsigned long npages) { }
250249
static inline void snp_set_wakeup_secondary_cpu(void) { }
251250
static inline bool snp_init(struct boot_params *bp) { return false; }
252251
static inline void snp_abort(void) { }
252+
static inline void snp_dmi_setup(void) { }
253253
static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio)
254254
{
255255
return -ENOTTY;

arch/x86/include/asm/x86_init.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,13 @@ struct x86_init_mpparse {
3030
* @reserve_resources: reserve the standard resources for the
3131
* platform
3232
* @memory_setup: platform specific memory setup
33-
*
33+
* @dmi_setup: platform specific DMI setup
3434
*/
3535
struct x86_init_resources {
3636
void (*probe_roms)(void);
3737
void (*reserve_resources)(void);
3838
char *(*memory_setup)(void);
39+
void (*dmi_setup)(void);
3940
};
4041

4142
/**

arch/x86/kernel/eisa.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
/*
33
* EISA specific code
44
*/
5+
#include <linux/cc_platform.h>
56
#include <linux/ioport.h>
67
#include <linux/eisa.h>
78
#include <linux/io.h>
@@ -12,7 +13,7 @@ static __init int eisa_bus_probe(void)
1213
{
1314
void __iomem *p;
1415

15-
if (xen_pv_domain() && !xen_initial_domain())
16+
if ((xen_pv_domain() && !xen_initial_domain()) || cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
1617
return 0;
1718

1819
p = ioremap(0x0FFFD9, 4);

arch/x86/kernel/probe_roms.c

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -203,16 +203,6 @@ void __init probe_roms(void)
203203
unsigned char c;
204204
int i;
205205

206-
/*
207-
* The ROM memory range is not part of the e820 table and is therefore not
208-
* pre-validated by BIOS. The kernel page table maps the ROM region as encrypted
209-
* memory, and SNP requires encrypted memory to be validated before access.
210-
* Do that here.
211-
*/
212-
snp_prep_memory(video_rom_resource.start,
213-
((system_rom_resource.end + 1) - video_rom_resource.start),
214-
SNP_PAGE_STATE_PRIVATE);
215-
216206
/* video rom */
217207
upper = adapter_rom_resources[0].start;
218208
for (start = video_rom_resource.start; start < upper; start += 2048) {

arch/x86/kernel/setup.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
#include <linux/console.h>
1010
#include <linux/crash_dump.h>
1111
#include <linux/dma-map-ops.h>
12-
#include <linux/dmi.h>
1312
#include <linux/efi.h>
1413
#include <linux/ima.h>
1514
#include <linux/init_ohci1394_dma.h>
@@ -902,7 +901,7 @@ void __init setup_arch(char **cmdline_p)
902901
efi_init();
903902

904903
reserve_ibft_region();
905-
dmi_setup();
904+
x86_init.resources.dmi_setup();
906905

907906
/*
908907
* VMware detection requires dmi to be available, so this

arch/x86/kernel/sev.c

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <linux/platform_device.h>
2424
#include <linux/io.h>
2525
#include <linux/psp-sev.h>
26+
#include <linux/dmi.h>
2627
#include <uapi/linux/sev-guest.h>
2728

2829
#include <asm/init.h>
@@ -795,21 +796,6 @@ void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr
795796
early_set_pages_state(vaddr, paddr, npages, SNP_PAGE_STATE_SHARED);
796797
}
797798

798-
void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op)
799-
{
800-
unsigned long vaddr, npages;
801-
802-
vaddr = (unsigned long)__va(paddr);
803-
npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
804-
805-
if (op == SNP_PAGE_STATE_PRIVATE)
806-
early_snp_set_memory_private(vaddr, paddr, npages);
807-
else if (op == SNP_PAGE_STATE_SHARED)
808-
early_snp_set_memory_shared(vaddr, paddr, npages);
809-
else
810-
WARN(1, "invalid memory op %d\n", op);
811-
}
812-
813799
static unsigned long __set_pages_state(struct snp_psc_desc *data, unsigned long vaddr,
814800
unsigned long vaddr_end, int op)
815801
{
@@ -2136,6 +2122,17 @@ void __head __noreturn snp_abort(void)
21362122
sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
21372123
}
21382124

2125+
/*
2126+
* SEV-SNP guests should only execute dmi_setup() if EFI_CONFIG_TABLES are
2127+
* enabled, as the alternative (fallback) logic for DMI probing in the legacy
2128+
* ROM region can cause a crash since this region is not pre-validated.
2129+
*/
2130+
void __init snp_dmi_setup(void)
2131+
{
2132+
if (efi_enabled(EFI_CONFIG_TABLES))
2133+
dmi_setup();
2134+
}
2135+
21392136
static void dump_cpuid_table(void)
21402137
{
21412138
const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();

arch/x86/kernel/x86_init.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*
44
* For licencing details see kernel-base/COPYING
55
*/
6+
#include <linux/dmi.h>
67
#include <linux/init.h>
78
#include <linux/ioport.h>
89
#include <linux/export.h>
@@ -66,6 +67,7 @@ struct x86_init_ops x86_init __initdata = {
6667
.probe_roms = probe_roms,
6768
.reserve_resources = reserve_standard_io_resources,
6869
.memory_setup = e820__memory_setup_default,
70+
.dmi_setup = dmi_setup,
6971
},
7072

7173
.mpparse = {

arch/x86/mm/mem_encrypt_amd.c

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,24 @@ void __init sme_early_init(void)
492492
*/
493493
if (sev_status & MSR_AMD64_SEV_ENABLED)
494494
ia32_disable();
495+
496+
/*
497+
* Override init functions that scan the ROM region in SEV-SNP guests,
498+
* as this memory is not pre-validated and would thus cause a crash.
499+
*/
500+
if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
501+
x86_init.mpparse.find_mptable = x86_init_noop;
502+
x86_init.pci.init_irq = x86_init_noop;
503+
x86_init.resources.probe_roms = x86_init_noop;
504+
505+
/*
506+
* DMI setup behavior for SEV-SNP guests depends on
507+
* efi_enabled(EFI_CONFIG_TABLES), which hasn't been
508+
* parsed yet. snp_dmi_setup() will run after that
509+
* parsing has happened.
510+
*/
511+
x86_init.resources.dmi_setup = snp_dmi_setup;
512+
}
495513
}
496514

497515
void __init mem_encrypt_free_decrypted_mem(void)

0 commit comments

Comments
 (0)