Re: [PATCH v4 3/5] mm/memory-failure: efi: record hardware-poisoned frames into the poisoned-memory table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing overflow check when computing the end address of the EFI poisoned memory table can lead to an unbounded memblock reservation and memory corruption.
--

commit 7216d7e029930900ac5741a5f5bbfe7a2107b408
Author: Breno Leitao <leitao@debian.org>

mm/memory-failure: efi: record hardware-poisoned frames into the poisoned-memory table

This commit hooks action_result() to efi_hwpoison_record_pfn() to record hard
offlined frames in the LINUX_EFI_POISONED_MEMORY table, ensuring they are
passed down to the next kernel via kexec. It also adds early initialization
code to vet the table inherited from previous boots.

> diff --git a/drivers/firmware/efi/poison.c b/drivers/firmware/efi/poison.c
> new file mode 100644
> index 0000000000000..c18edf111c710
> --- /dev/null
> +++ b/drivers/firmware/efi/poison.c
[ ... ]
> +static bool __init
> +efi_poison_geometry_valid(const struct linux_efi_poisoned_memory *pm)
> +{
> +	u64 nbits;
> +
> +	/* Whole words, and a bit count that can be taken without wrapping. */
> +	if (!pm->size || !IS_ALIGNED(pm->size, sizeof(unsigned long)) ||
> +	    check_mul_overflow(pm->size, (u64)BITS_PER_BYTE, &nbits))
> +		return false;

[Severity: High]
Does this validation fully protect against an excessively large pm->size?

While check_mul_overflow() validates the size for bit conversion, it appears
it does not verify that the table's footprint fits within the physical address
space without overflowing when combined with efi.poisoned_memory.

> +
> +	if (pm->unit_size < PAGE_SIZE || !is_power_of_2(pm->unit_size))
> +		return false;
> +
> +	return IS_ALIGNED(pm->phys_base, pm->unit_size);
> +}
[ ... ]
> +void __init efi_poisoned_memory_reserve(void)
> +{
> +	struct linux_efi_poisoned_memory *pm;
> +	phys_addr_t start, end;
> +
> +	if (efi.poisoned_memory == EFI_INVALID_TABLE_ADDR)
> +		return;
> +
> +	pm = early_memremap(efi.poisoned_memory, sizeof(*pm));
> +	if (!pm) {
> +		pr_warn("Could not map poisoned-memory table\n");
> +		efi.poisoned_memory = EFI_INVALID_TABLE_ADDR;
> +		return;
> +	}
> +
> +	if (!efi_poison_table_valid(pm)) {
> +		efi.poisoned_memory = EFI_INVALID_TABLE_ADDR;
> +		early_memunmap(pm, sizeof(*pm));
> +		return;
> +	}
> +
> +	start = PAGE_ALIGN_DOWN(efi.poisoned_memory);
> +	end = PAGE_ALIGN(efi.poisoned_memory + sizeof(*pm) + pm->size);

[Severity: High]
Can this computation wrap around if pm->size is maliciously large?

Because pm->size is only constrained to be less than U64_MAX / 8 in
efi_poison_geometry_valid(), an unvalidated addition here could overflow,
resulting in end < start. If that happens, an enormous end - start would
be passed to memblock_add() and memblock_reserve().

This could result in memblock capping the size to PHYS_ADDR_MAX - start,
blindly marking the rest of the physical address space (including MMIO) as
RAM. This forces direct-map cacheable page table entries for MMIO regions,
leading to system crashes or memory layout corruption.

> +	early_memunmap(pm, sizeof(*pm));
> +
> +	memblock_add(start, end - start);
> +	memblock_reserve(start, end - start);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-hwpoison-kho-v4-0-359313564495@debian.org?part=3