| Message ID | 20230803175916.3174453-5-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RISC-V: ACPI: Add external interrupt controller support | expand |
On Thu, Aug 03, 2023 at 11:28:59PM +0530, Sunil V L wrote: > Enhance the acpi_os_ioremap() to support opregions in MMIO > space. Also, have strict checks using EFI memory map > to allow remapping the RAM similar to arm64. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Alexandre Ghiti <alexghiti@rivosinc.com> You may use --cc to the command line when forming patches. Also we usually consider Cc: as a part of the tag block, meaning no blank line should be here. > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> ... > #include <linux/io.h> > #include <linux/pci.h> > #include <linux/efi.h> > +#include <linux/memblock.h> Can you squeeze it to have some order, like to be after io.h (taking into account given context)? ... > + if (memblock_is_map_memory(phys) || > + !memblock_is_region_memory(phys, size)) { > + pr_warn(FW_BUG "requested region covers kernel memory @ %p\n", > + &phys); How %p can be useful here (it's mangled), but also wouldn't this give a hint to an attacker about the kernel memory location and diminish the KASLR protection? (IIRC after boot we always have the same salt for the mangling the pointers when printing, so at least theoretically it might be possible to bruteforce the printing algo to give a clue about the kernel address.) > + return NULL; > + }
On Fri, Aug 04, 2023 at 08:47:53AM +0300, Andy Shevchenko wrote: > On Thu, Aug 03, 2023 at 11:28:59PM +0530, Sunil V L wrote: > > Enhance the acpi_os_ioremap() to support opregions in MMIO > > space. Also, have strict checks using EFI memory map > > to allow remapping the RAM similar to arm64. > > > > Cc: Ard Biesheuvel <ardb@kernel.org> > > Cc: Alexandre Ghiti <alexghiti@rivosinc.com> > > You may use --cc to the command line when forming patches. > > Also we usually consider Cc: as a part of the tag block, meaning no blank line > should be here. > Thanks!, Andy. I specifically wanted Ard and Alex to look at this patch and hence added Cc. I was not aware that no blank line should be there. Thanks for letting me know. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > ... > > > #include <linux/io.h> > > #include <linux/pci.h> > > #include <linux/efi.h> > > +#include <linux/memblock.h> > > Can you squeeze it to have some order, like to be after io.h (taking into > account given context)? > Yeah, sure. Let me update in next version. > ... > > > + if (memblock_is_map_memory(phys) || > > + !memblock_is_region_memory(phys, size)) { > > + pr_warn(FW_BUG "requested region covers kernel memory @ %p\n", > > + &phys); > > How %p can be useful here (it's mangled), but also wouldn't this give a hint to > an attacker about the kernel memory location and diminish the KASLR protection? > (IIRC after boot we always have the same salt for the mangling the pointers when > printing, so at least theoretically it might be possible to bruteforce the > printing algo to give a clue about the kernel address.) > Okay. This is copied from arm64 version. But I think this is a good point. Let me just print the warning message without address in the next version. Thanks! Sunil
On Thu, Aug 03, 2023 at 11:28:59PM +0530, Sunil V L wrote: > Enhance the acpi_os_ioremap() to support opregions in MMIO > space. Also, have strict checks using EFI memory map > to allow remapping the RAM similar to arm64. > > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Alexandre Ghiti <alexghiti@rivosinc.com> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > --- > arch/riscv/Kconfig | 1 + > arch/riscv/kernel/acpi.c | 86 +++++++++++++++++++++++++++++++++++++++- > 2 files changed, 86 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 318f62a0a187..e19f32c12a68 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -38,6 +38,7 @@ config RISCV > select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST > select ARCH_HAS_UBSAN_SANITIZE_ALL > select ARCH_HAS_VDSO_DATA > + select ARCH_KEEP_MEMBLOCK > select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX > select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT > select ARCH_STACKWALK > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > index 56cb2c986c48..aa4433bca6d9 100644 > --- a/arch/riscv/kernel/acpi.c > +++ b/arch/riscv/kernel/acpi.c > @@ -17,6 +17,7 @@ > #include <linux/io.h> > #include <linux/pci.h> > #include <linux/efi.h> > +#include <linux/memblock.h> > > int acpi_noirq = 1; /* skip ACPI IRQ initialization */ > int acpi_disabled = 1; > @@ -217,7 +218,90 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size) > > void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) > { > - return (void __iomem *)memremap(phys, size, MEMREMAP_WB); > + efi_memory_desc_t *md, *region = NULL; > + pgprot_t prot; > + > + if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP))) > + return NULL; > + > + for_each_efi_memory_desc(md) { > + u64 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT); > + > + if (phys < md->phys_addr || phys >= end) > + continue; > + > + if (phys + size > end) { > + pr_warn(FW_BUG "requested region covers multiple EFI memory regions\n"); > + return NULL; > + } > + region = md; > + break; > + } > + > + /* > + * It is fine for AML to remap regions that are not represented in the > + * EFI memory map at all, as it only describes normal memory, and MMIO > + * regions that require a virtual mapping to make them accessible to > + * the EFI runtime services. > + */ > + prot = PAGE_KERNEL_IO; > + if (region) { > + switch (region->type) { > + case EFI_LOADER_CODE: > + case EFI_LOADER_DATA: > + case EFI_BOOT_SERVICES_CODE: > + case EFI_BOOT_SERVICES_DATA: > + case EFI_CONVENTIONAL_MEMORY: > + case EFI_PERSISTENT_MEMORY: > + if (memblock_is_map_memory(phys) || > + !memblock_is_region_memory(phys, size)) { > + pr_warn(FW_BUG "requested region covers kernel memory @ %p\n", > + &phys); > + return NULL; > + } > + > + /* > + * Mapping kernel memory is permitted if the region in > + * question is covered by a single memblock with the > + * NOMAP attribute set: this enables the use of ACPI > + * table overrides passed via initramfs. > + * This particular use case only requires read access. > + */ > + fallthrough; > + > + case EFI_RUNTIME_SERVICES_CODE: > + /* > + * This would be unusual, but not problematic per se, > + * as long as we take care not to create a writable > + * mapping for executable code. > + */ > + prot = PAGE_KERNEL_RO; > + break; > + > + case EFI_ACPI_RECLAIM_MEMORY: > + /* > + * ACPI reclaim memory is used to pass firmware tables > + * and other data that is intended for consumption by > + * the OS only, which may decide it wants to reclaim > + * that memory and use it for something else. We never > + * do that, but we usually add it to the linear map > + * anyway, in which case we should use the existing > + * mapping. > + */ > + if (memblock_is_map_memory(phys)) > + return (void __iomem *)__va(phys); > + fallthrough; > + > + default: > + if (region->attribute & EFI_MEMORY_WB) > + prot = PAGE_KERNEL; > + else if ((region->attribute & EFI_MEMORY_WC) || > + (region->attribute & EFI_MEMORY_WT)) > + prot = pgprot_writecombine(PAGE_KERNEL); > + } > + } > + > + return ioremap_prot(phys, size, pgprot_val(prot)); > } > > #ifdef CONFIG_PCI > -- > 2.39.2 > Such a faithful port of arm64's function that it begs the question as to whether or not we should refactor and share. Anyway, other than Andy's comments, LGTM Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks, drew
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 318f62a0a187..e19f32c12a68 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -38,6 +38,7 @@ config RISCV select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_HAS_UBSAN_SANITIZE_ALL select ARCH_HAS_VDSO_DATA + select ARCH_KEEP_MEMBLOCK select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT select ARCH_STACKWALK diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c index 56cb2c986c48..aa4433bca6d9 100644 --- a/arch/riscv/kernel/acpi.c +++ b/arch/riscv/kernel/acpi.c @@ -17,6 +17,7 @@ #include <linux/io.h> #include <linux/pci.h> #include <linux/efi.h> +#include <linux/memblock.h> int acpi_noirq = 1; /* skip ACPI IRQ initialization */ int acpi_disabled = 1; @@ -217,7 +218,90 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size) void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size) { - return (void __iomem *)memremap(phys, size, MEMREMAP_WB); + efi_memory_desc_t *md, *region = NULL; + pgprot_t prot; + + if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP))) + return NULL; + + for_each_efi_memory_desc(md) { + u64 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT); + + if (phys < md->phys_addr || phys >= end) + continue; + + if (phys + size > end) { + pr_warn(FW_BUG "requested region covers multiple EFI memory regions\n"); + return NULL; + } + region = md; + break; + } + + /* + * It is fine for AML to remap regions that are not represented in the + * EFI memory map at all, as it only describes normal memory, and MMIO + * regions that require a virtual mapping to make them accessible to + * the EFI runtime services. + */ + prot = PAGE_KERNEL_IO; + if (region) { + switch (region->type) { + case EFI_LOADER_CODE: + case EFI_LOADER_DATA: + case EFI_BOOT_SERVICES_CODE: + case EFI_BOOT_SERVICES_DATA: + case EFI_CONVENTIONAL_MEMORY: + case EFI_PERSISTENT_MEMORY: + if (memblock_is_map_memory(phys) || + !memblock_is_region_memory(phys, size)) { + pr_warn(FW_BUG "requested region covers kernel memory @ %p\n", + &phys); + return NULL; + } + + /* + * Mapping kernel memory is permitted if the region in + * question is covered by a single memblock with the + * NOMAP attribute set: this enables the use of ACPI + * table overrides passed via initramfs. + * This particular use case only requires read access. + */ + fallthrough; + + case EFI_RUNTIME_SERVICES_CODE: + /* + * This would be unusual, but not problematic per se, + * as long as we take care not to create a writable + * mapping for executable code. + */ + prot = PAGE_KERNEL_RO; + break; + + case EFI_ACPI_RECLAIM_MEMORY: + /* + * ACPI reclaim memory is used to pass firmware tables + * and other data that is intended for consumption by + * the OS only, which may decide it wants to reclaim + * that memory and use it for something else. We never + * do that, but we usually add it to the linear map + * anyway, in which case we should use the existing + * mapping. + */ + if (memblock_is_map_memory(phys)) + return (void __iomem *)__va(phys); + fallthrough; + + default: + if (region->attribute & EFI_MEMORY_WB) + prot = PAGE_KERNEL; + else if ((region->attribute & EFI_MEMORY_WC) || + (region->attribute & EFI_MEMORY_WT)) + prot = pgprot_writecombine(PAGE_KERNEL); + } + } + + return ioremap_prot(phys, size, pgprot_val(prot)); } #ifdef CONFIG_PCI
Enhance the acpi_os_ioremap() to support opregions in MMIO space. Also, have strict checks using EFI memory map to allow remapping the RAM similar to arm64. Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Alexandre Ghiti <alexghiti@rivosinc.com> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> --- arch/riscv/Kconfig | 1 + arch/riscv/kernel/acpi.c | 86 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 1 deletion(-)