| 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: <l2crzyeoux2pammbifkivrhp637gza7piumd3s6j66mezsfvdy@nwczgs2hkq4f>
Date: Mon, 14 Apr 2025 23:52:45 +0530
From: Naveen N Rao <naveen@...nel.org>
To: Dan Williams <dan.j.williams@...el.com>
Cc: dave.hansen@...ux.intel.com, x86@...nel.org,
Ingo Molnar <mingo@...nel.org>, Vishal Annapurve <vannapurve@...gle.com>,
Kirill Shutemov <kirill.shutemov@...ux.intel.com>, Nikolay Borisov <nik.borisov@...e.com>, stable@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] x86/devmem: Restrict /dev/mem access for
potentially unaccepted memory by default
On Thu, Apr 10, 2025 at 06:22:38PM -0700, Dan Williams wrote:
> Nikolay reports [1] that accessing BIOS data (first 1MB of the physical
> address space) via /dev/mem results in an SEPT violation.
>
> The cause is ioremap() (via xlate_dev_mem_ptr()) establishes an
> unencrypted mapping where the kernel had established an encrypted
> mapping previously.
>
> An initial attempt to fix this revealed that TDX and SEV-SNP have
> different expectations about which and when address ranges can be mapped
> via /dev/mem.
>
> Rather than develop a precise set of allowed /dev/mem capable TVM
> address ranges, teach devmem_is_allowed() to always restrict access to
> the BIOS data space.
This patch does more than just restrict the BIOS data space - it rejects
all accesses to /dev/mem _apart_ from the first 1MB. That should be made
clear here.
> This means return 0s for read(), drop write(), and
> -EPERM mmap(). This can still be later relaxed as specific needs arise,
> but in the meantime, close off this source of mismatched
> IORES_MAP_ENCRYPTED expectations.
>
> Cc: <x86@...nel.org>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: Vishal Annapurve <vannapurve@...gle.com>
> Cc: Kirill Shutemov <kirill.shutemov@...ux.intel.com>
> Reported-by: Nikolay Borisov <nik.borisov@...e.com>
> Closes: http://lore.kernel.org/20250318113604.297726-1-nik.borisov@suse.com [1]
> Reviewed-by: Nikolay Borisov <nik.borisov@...e.com>
> Fixes: 9aa6ea69852c ("x86/tdx: Make pages shared in ioremap()")
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
> arch/x86/Kconfig | 2 ++
> arch/x86/include/asm/x86_init.h | 2 ++
> arch/x86/kernel/x86_init.c | 6 ++++++
> arch/x86/mm/init.c | 23 +++++++++++++++++------
> 4 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 4b9f378e05f6..12a1b5acd55b 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -891,6 +891,7 @@ config INTEL_TDX_GUEST
> depends on X86_X2APIC
> depends on EFI_STUB
> depends on PARAVIRT
> + depends on STRICT_DEVMEM
> select ARCH_HAS_CC_PLATFORM
> select X86_MEM_ENCRYPT
> select X86_MCE
> @@ -1510,6 +1511,7 @@ config AMD_MEM_ENCRYPT
> bool "AMD Secure Memory Encryption (SME) support"
> depends on X86_64 && CPU_SUP_AMD
> depends on EFI_STUB
> + depends on STRICT_DEVMEM
> select DMA_COHERENT_POOL
> select ARCH_USE_MEMREMAP_PROT
> select INSTRUCTION_DECODER
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 213cf5379a5a..0ae436b34b88 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -305,6 +305,7 @@ struct x86_hyper_runtime {
> * semantics.
> * @realmode_reserve: reserve memory for realmode trampoline
> * @realmode_init: initialize realmode trampoline
> + * @devmem_is_allowed restrict /dev/mem and PCI sysfs resource access
> * @hyper: x86 hypervisor specific runtime callbacks
> */
> struct x86_platform_ops {
> @@ -323,6 +324,7 @@ struct x86_platform_ops {
> void (*set_legacy_features)(void);
> void (*realmode_reserve)(void);
> void (*realmode_init)(void);
> + bool (*devmem_is_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..346301375bd4 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -143,6 +143,11 @@ 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 platform_devmem_is_allowed(unsigned long pfn)
> +{
> + return !cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
> +}
> +
> struct x86_platform_ops x86_platform __ro_after_init = {
> .calibrate_cpu = native_calibrate_cpu_early,
> .calibrate_tsc = native_calibrate_tsc,
> @@ -156,6 +161,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,
> + .devmem_is_allowed = platform_devmem_is_allowed,
> .hyper.pin_vcpu = x86_op_int_noop,
> .hyper.is_private_mmio = is_private_mmio_noop,
>
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index bfa444a7dbb0..df5435c8dbea 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -861,18 +861,23 @@ void __init poking_init(void)
> * area traditionally contains BIOS code and data regions used by X, dosemu,
> * and similar apps. Since they map the entire memory range, the whole range
> * must be allowed (for mapping), but any areas that would otherwise be
> - * disallowed are flagged as being "zero filled" instead of rejected.
> + * disallowed are flagged as being "zero filled" instead of rejected, for
> + * read()/write().
> + *
> * Access has to be given to non-kernel-ram areas as well, these contain the
> * PCI mmio resources as well as potential bios/acpi data regions.
> */
> int devmem_is_allowed(unsigned long pagenr)
> {
> + bool platform_allowed = x86_platform.devmem_is_allowed(pagenr);
> +
If we are going to do this, I don't see the point of having an
x86_platform_op. It may be better to simply gate this on
cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) directly here.
Thanks,
Naveen
> if (region_intersects(PFN_PHYS(pagenr), PAGE_SIZE,
> IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)
> != REGION_DISJOINT) {
> /*
> - * For disallowed memory regions in the low 1MB range,
> - * request that the page be shown as all zeros.
> + * For disallowed memory regions in the low 1MB range, request
> + * that the page be shown as all zeros for read()/write(), fail
> + * mmap()
> */
> if (pagenr < 256)
> return 2;
> @@ -885,14 +890,20 @@ int devmem_is_allowed(unsigned long pagenr)
> * restricted resource under CONFIG_STRICT_DEVMEM.
> */
> if (iomem_is_exclusive(pagenr << PAGE_SHIFT)) {
> - /* Low 1MB bypasses iomem restrictions. */
> - if (pagenr < 256)
> + /*
> + * Low 1MB bypasses iomem restrictions unless the platform says
> + * the physical address is not suitable for direct access.
> + */
> + if (pagenr < 256) {
> + if (!platform_allowed)
> + return 2;
> return 1;
> + }
>
> return 0;
> }
>
> - return 1;
> + return platform_allowed;
> }
>
> void free_init_pages(const char *what, unsigned long begin, unsigned long end)
>
Powered by blists - more mailing lists