Re: [PATCH v6 6/7] mm/memory-failure: efi: reserve the inherited poisoned-memory units

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] efi: linux_efi_poisoned_memory ABI depends on architecture word size
- [High] efi: missing PHYS_ADDR_MAX check causes physical address truncation
--

commit 458406c8bb39d60c5c6472cbc3795bf8b4bdc8e3
Author: Breno Leitao <leitao@debian.org>

mm/memory-failure: efi: reserve the inherited poisoned-memory units

This commit parses a new EFI table to inherit hardware-poisoned memory frames 
from previous kernels during kexec. It prevents the memory allocator from using 
these corrupt physical pages.

> diff --git a/drivers/firmware/efi/poison.c b/drivers/firmware/efi/poison.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/firmware/efi/poison.c
> @@ -0,0 +1,174 @@
[ ... ]
> +static bool __init
> +efi_poison_geometry_valid(const struct linux_efi_poisoned_memory *pm)
> +{
> +	const u64 max_end = PHYS_ADDR_MAX - PAGE_SIZE;
> +	u64 nbits, span, top, end, len;
> +
> +	/* 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 architecture-dependent validation cause valid tables from mismatched
architectures to be rejected during kexec? 

If a 32-bit kernel populates the linux_efi_poisoned_memory table with a size
that is a multiple of 4, a 64-bit kexec kernel will evaluate
IS_ALIGNED(pm->size, sizeof(unsigned long)) where sizeof(unsigned long) is 8.
This check would fail, and the table would be erroneously discarded.

Additionally, if the table structure uses unsigned long for the bitmap array,
reading a 32-bit written array as 64-bit words can shift bit indices on
big-endian systems. Could fixed-width types like u64 be used instead to
ensure a stable cross-kernel ABI?

> +
> +	/* And a footprint that can be page aligned without wrapping either. */
> +	len = sizeof(*pm) + pm->size;
> +	if (check_add_overflow(efi.poisoned_memory, len, &end) ||
> +	    end > max_end)
> +		return false;
> +
> +	if (pm->unit_size < PAGE_SIZE || !is_power_of_2(pm->unit_size))
> +		return false;
> +
> +	/* Every unit has to lie inside the address space. */
> +	if (check_mul_overflow(nbits, (u64)pm->unit_size, &span) ||
> +	    check_add_overflow(pm->phys_base, span, &top))
> +		return false;

[Severity: High]
Should this validation also check if top exceeds PHYS_ADDR_MAX?

While this calculates the top address in 64-bit space, without an explicit
bounds check, a table specifying a phys_base above 4GB could pass validation
on a 32-bit architecture without PAE.

> +
> +	return IS_ALIGNED(pm->phys_base, pm->unit_size);
> +}
[ ... ]
> +static void __init
> +efi_poison_reserve_units(phys_addr_t table,
> +			 const struct linux_efi_poisoned_memory *hdr)
> +{
[ ... ]
> +		for_each_set_bit(bit, map, nbits) {
> +			u64 unit = off * BITS_PER_BYTE + bit;
> +			phys_addr_t base = hdr->phys_base + unit * unit_size;

[Severity: High]
Can this lead to physical address truncation on 32-bit systems?

Because hdr->phys_base + unit * unit_size evaluates as a 64-bit integer,
assigning it to phys_addr_t (which is 32 bits on 32-bit systems without PAE)
will silently truncate the upper 32 bits. This could cause valid low memory
to be erroneously marked as poisoned and reserved, potentially leading to
boot failures.

> +			bool overlap;
> +
> +			overlap = memblock_is_region_reserved(base, unit_size);
> +			if (memblock_reserve_hwpoison(base, unit_size)) {
> +				nr_skipped++;
> +			} else {
> +				nr_units++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-hwpoison-kho-v6-0-f304255147da@debian.org?part=6