Re: [PATCH] x86/unaccepted_memory: Do not accept ranges that are no longer E820 RAM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] x86/unaccepted_memory: arch_accept_memory() desynchronizes unaccepted bitmap
- [High] x86/unaccepted_memory: unaligned E820 bounds passed to __arch_accept_memory()
--

commit f56630bdf157fa53e09d29e75f1fc454adb6cd1e
Author: Pankaj Gupta <pankaj.gupta@amd.com>

x86/unaccepted_memory: Do not accept ranges that are no longer E820 RAM

This commit updates arch_accept_memory() on x86 to iterate over e820_table
and only accept subranges that are marked as E820_TYPE_RAM. This avoids
attempting to accept memory holes removed via the memmap= command-line
parameter.

> diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
> index f5937e9866ac1..744f1a993352c 100644
> --- a/arch/x86/include/asm/unaccepted_memory.h
> +++ b/arch/x86/include/asm/unaccepted_memory.h
> @@ -2,10 +2,11 @@
>  #define _ASM_X86_UNACCEPTED_MEMORY_H
>  
>  #include <linux/efi.h>
> +#include <asm/e820/api.h>
>  #include <asm/tdx.h>
>  #include <asm/sev.h>
>  
> -static inline void arch_accept_memory(phys_addr_t start, phys_addr_t end)
> +static inline void __arch_accept_memory(phys_addr_t start, phys_addr_t end)
>  {
>  	/* Platform-specific memory-acceptance call goes here */
>  	if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
> @@ -18,6 +19,26 @@ static inline void arch_accept_memory(phys_addr_t start, phys_addr_t end)
>  	}
>  }
>  
> +static inline void arch_accept_memory(phys_addr_t start, phys_addr_t end)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < e820_table->nr_entries; i++) {
> +		struct e820_entry *entry = &e820_table->entries[i];
> +		phys_addr_t s, e;
> +
> +		if (entry->type != E820_TYPE_RAM)
> +			continue;

[Severity: High]
Does skipping non-E820_TYPE_RAM ranges here desynchronize unaccepted->bitmap
from the actual hardware page acceptance state across kdump and kexec?

In drivers/firmware/efi/unaccepted_memory.c:accept_memory(), unit_size-aligned
range ends are extended by an extra unit_size to act as a guard page for
load_unaligned_zeropad(), and the bits in unaccepted->bitmap are cleared
unconditionally after arch_accept_memory() returns:

drivers/firmware/efi/unaccepted_memory.c:accept_memory() {
	...
	/*
	 * load_unaligned_zeropad() can lead to unwanted loads across page
	 * boundaries. The unwanted loads are typically harmless. Map an
	 * extra unit if the end of the requested range is aligned to
	 * unit_size to avoid page faults on unaccepted memory.
	 */
	if (!(end % unit_size))
		end += unit_size;
	...
		arch_accept_memory(phys_start, phys_end);

		spin_lock(&unaccepted_memory_lock);
		bitmap_clear(unaccepted->bitmap, range_start, len);
	...
}

When arch_accept_memory() skips a range that is not E820_TYPE_RAM in
e820_table, accept_memory() still clears its bit in unaccepted->bitmap even
though the pages were never accepted in hardware.

In a kdump kernel, e820_table only contains the crashkernel reservation as
E820_TYPE_RAM while the panicked kernel's remaining RAM is omitted from
e820_table. When the crash kernel accepts memory up to the 2 MiB-aligned end
of crashk_res, accept_memory() extends end by unit_size into the panicked
kernel's adjacent unaccepted RAM. arch_accept_memory() skips that range, but
accept_memory() clears its bit in unaccepted->bitmap. Later, when /proc/vmcore
is read, unaccepted_memory_vmcore_pfn_is_ram() checks
!pfn_is_unaccepted_memory(pfn), sees the cleared bit, and reads the
unaccepted page via read_from_oldmem(), crashing the kdump kernel.

A similar mismatch occurs with kexec: e820__memory_setup() populates
e820_table_kexec before parse_early_param() applies memmap= or mem= options
to e820_table, andarch/x86/kernel/kexec-bzimage64.c:setup_e820_entries()
passes e820_table_kexec to the next kernel while sharing the physical
efi.unaccepted table. Both kernel/kexec_core.c:sanity_check_segment_list()
(which calls accept_memory() on kexec segment destinations placed in
e820_table_kexec RAM) and the unit_size extension at the boundary before a
memmap= hole will clear bits in unaccepted->bitmap without accepting the
memory in hardware, causing the kexec load or the next kernel to fault on
those pages.

> +
> +		s = max_t(phys_addr_t, start, entry->addr);
> +		e = min_t(phys_addr_t, end, entry->addr + entry->size);
> +		if (s >= e)
> +			continue;
> +
> +		__arch_accept_memory(s, e);

[Severity: High]
What happens if entry->addr or entry->addr + entry->size is not aligned to
PAGE_SIZE?

Entries in e820_table can have byte-granular boundaries from firmware or
command-line options such as memmap= and mem=. When memblock is set up in
arch/x86/kernel/e820.c:e820__memblock_setup(), partial pages are trimmed via
memblock_trim_memory(PAGE_SIZE) and left unmapped in the direct map, whereas
e820_table retains the unaligned addresses.

Clamping s and e directly to entry->addr and entry->addr + entry->size can
therefore pass non-page-aligned boundaries to __arch_accept_memory(s, e):

1. On TDX, tdx_accept_memory() -> try_accept_one() requires start to be
   aligned to at least PG_LEVEL_4K and len to be at least PAGE_SIZE:

   arch/x86/coco/tdx/tdx-shared.c:try_accept_one() {
   	...
   	if (!IS_ALIGNED(start, accept_size))
   		return 0;

   	if (len < accept_size)
   		return 0;
   	...
   }

   An unaligned s or a trailing sub-page remainder causes tdx_accept_memory()
   to return false and __arch_accept_memory() to call
   panic("TDX: Failed to accept memory\n").

2. On SEV-SNP, snp_accept_memory() passes vaddr = (unsigned long)__va(start)
   and npages = (end - start) >> PAGE_SHIFT to set_pages_state(), which
   masks vaddr with PAGE_MASK. If s is not page-aligned, vaddr & PAGE_MASK
   rounds down into the unmapped partial page trimmed by
   memblock_trim_memory(PAGE_SIZE) and faults in pvalidate_pages(), while
   (e - s) >> PAGE_SHIFT can truncate npages and leave the last full page
   unaccepted.

Should s be rounded up with PAGE_ALIGN(s) and e rounded down with
PAGE_ALIGN_DOWN(e) before checking s >= e?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102113127.161823-1-pankaj.gupta@amd.com?part=1