| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <67eedc35be77d_464ec29462@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 3 Apr 2025 12:06:29 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: "Naveen N Rao (AMD)" <naveen@...nel.org>, <linux-kernel@...r.kernel.org>,
<x86@...nel.org>, <linux-coco@...ts.linux.dev>
CC: Dave Hansen <dave.hansen@...ux.intel.com>, Borislav Petkov <bp@...en8.de>,
Vishal Annapurve <vannapurve@...gle.com>, Kirill Shutemov
<kirill.shutemov@...ux.intel.com>, Dan Williams <dan.j.williams@...el.com>,
Nikolay Borisov <nik.borisov@...e.com>, Tom Lendacky
<thomas.lendacky@....com>, Kevin Loughlin <kevinloughlin@...gle.com>
Subject: Re: [RFC PATCH] x86/sev: Disallow userspace access to BIOS region
for SEV-SNP guests
Naveen N Rao (AMD) wrote:
> Commit 9704c07bf9f7 ("x86/kernel: Validate ROM memory before accessing
> when SEV-SNP is active") added code to validate the ROM region from
> 0xc0000 to 0xfffff in a SEV-SNP guest since that region can be accessed
> during kernel boot. That address range is not part of the system RAM, so
> it needed to be validated separately.
>
> Commit 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for
> SEV-SNP guests") reverted those changes and instead chose to prevent the
> guest from accessing the ROM region since SEV-SNP guests did not rely on
> data from that region. However, while the kernel itself no longer
> accessed the ROM region, there are userspace programs that probe this
> region through /dev/mem and they started crashing due to this change. In
> particular, fwupd (up until versions released last year that no longer
> link against libsmbios) and smbios utilities such as smbios-sys-info
> crash with a cryptic message in dmesg:
> Wrong/unhandled opcode bytes: 0x8b, exit_code: 0x404, rIP: 0x7fe5404d3840
> SEV: Unsupported exit-code 0x404 in #VC exception (IP: 0x7fe5404d3840)
>
> Deny access to the BIOS region (rather than just the video ROM range)
> via /dev/mem to address this. Restrict changes to CONFIG_STRICT_DEVMEM=y
> which is enabled by default on x86. Add a new x86_platform_ops callback
> so Intel can customize the address range to block.
>
> Fixes: 0f4a1e80989a ("x86/sev: Skip ROM range scans and validation for SEV-SNP guests")
> Signed-off-by: Naveen N Rao (AMD) <naveen@...nel.org>
> ---
> arch/x86/coco/sev/core.c | 13 +++++++++++++
> arch/x86/include/asm/sev.h | 2 ++
> arch/x86/include/asm/x86_init.h | 2 ++
> arch/x86/kernel/x86_init.c | 2 ++
> arch/x86/mm/init.c | 3 +++
> arch/x86/mm/mem_encrypt_amd.c | 1 +
> 6 files changed, 23 insertions(+)
>
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index b0c1a7a57497..4e10701536d4 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -43,6 +43,7 @@
> #include <asm/apic.h>
> #include <asm/cpuid.h>
> #include <asm/cmdline.h>
> +#include <asm/e820/types.h>
>
> #define DR7_RESET_VALUE 0x400
>
> @@ -761,6 +762,18 @@ static u64 __init get_jump_table_addr(void)
> return ret;
> }
>
> +bool sev_snp_pfn_access_allowed(unsigned long pfn)
> +{
> + /*
> + * Reject access to BIOS address range (0xa0000 to 0x100000) for SEV-SNP guests
> + * as that address range is not validated, so access can cause #VC exception
> + */
> + if (pfn << PAGE_SHIFT >= BIOS_BEGIN && pfn << PAGE_SHIFT < BIOS_END)
> + return 0;
> +
> + return 1;
> +}
> +
> static void __head
> early_set_pages_state(unsigned long vaddr, unsigned long paddr,
> unsigned long npages, enum psc_op op)
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index ba7999f66abe..721498c0a055 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -454,6 +454,7 @@ static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate)
> struct snp_guest_request_ioctl;
>
> void setup_ghcb(void);
> +bool sev_snp_pfn_access_allowed(unsigned long pfn);
> void early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr,
> unsigned long npages);
> void early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr,
> @@ -496,6 +497,7 @@ static inline void sev_enable(struct boot_params *bp) { }
> static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
> static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
> static inline void setup_ghcb(void) { }
> +static inline bool sev_snp_pfn_access_allowed(unsigned long pfn) { return true; }
> static inline void __init
> early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, unsigned long npages) { }
> static inline void __init
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 36698cc9fb44..d559587dee48 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -307,6 +307,7 @@ struct x86_hyper_runtime {
> * @realmode_reserve: reserve memory for realmode trampoline
> * @realmode_init: initialize realmode trampoline
> * @hyper: x86 hypervisor specific runtime callbacks
> + * @pfn_access_allowed: filter accesses to pages
> */
> struct x86_platform_ops {
> unsigned long (*calibrate_cpu)(void);
> @@ -324,6 +325,7 @@ struct x86_platform_ops {
> void (*set_legacy_features)(void);
> void (*realmode_reserve)(void);
> void (*realmode_init)(void);
> + bool (*pfn_access_allowed)(unsigned long pfn);
> struct x86_hyper_runtime hyper;
> struct x86_guest guest;
> };
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 0a2bbd674a6d..3679a92a3881 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -142,6 +142,7 @@ static bool enc_cache_flush_required_noop(void) { return false; }
> static void enc_kexec_begin_noop(void) {}
> static void enc_kexec_finish_noop(void) {}
> static bool is_private_mmio_noop(u64 addr) {return false; }
> +static bool pfn_access_allowed_noop(unsigned long pfn) { return true; }
>
> struct x86_platform_ops x86_platform __ro_after_init = {
> .calibrate_cpu = native_calibrate_cpu_early,
> @@ -156,6 +157,7 @@ struct x86_platform_ops x86_platform __ro_after_init = {
> .restore_sched_clock_state = tsc_restore_sched_clock_state,
> .realmode_reserve = reserve_real_mode,
> .realmode_init = init_real_mode,
> + .pfn_access_allowed = pfn_access_allowed_noop,
Is there any driving need to allow devmem at all for TVM access at this
point?
I would be in favor of making this clearly tied to devmem, call it
".devmem_is_allowed" for symmetry with the mm/init.c helper, and make
the default implementation be:
static bool platform_devmem_is_allowed(unsigned long pfn)
{
return !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
}
...if a TVM technology wants more leniency, it can override.
Powered by blists - more mailing lists