Re: [PATCH 2/3] of: reserved_mem: reject statically placed regions overlapping existing reservations

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Rejecting partially overlapping reserved memory regions without calling memblock_reserve leaves non-overlapping parts exposed to the buddy allocator.
- [Low] The overlap error message prints an exclusive end address using a format conventionally used for inclusive ranges, and can wrap around to 0 on integer overflow.

Pre-existing issues:
- [High] The patch fails to reject overlaps for 'no-map' regions, leaving the memory corruption vulnerability it intended to fix fully exploitable in that case.
--

--- Patch [2]: [PATCH 2/3] of: reserved_mem: reject statically placed regions overlapping existing reservations ---
commit 5a1dc7b658795d38b565b07f22ac870257f66deb
Author: Wandun Chen <[email protected]>
Subject: of: reserved_mem: reject statically placed regions overlapping existing reservations

This patch rejects statically placed reserved memory regions that overlap
with existing reservations. It prevents the overlapping memory from being
used concurrently or returned to the buddy allocator if a region's driver
fails to initialize.

Link: https://lore.kernel.org/all/[email protected]/ [1]

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index f20747725de42..67cb60616fc69 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -146,6 +146,15 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  
>  		return memblock_mark_nomap(base, size);
>  	}

[Severity: High]
This is a pre-existing issue, but in early_init_dt_reserve_memory() does the
overlap check for nomap regions fail to detect overlaps between two nomap
regions?

Because memblock_mark_nomap() does not add the region to memblock.reserved,
a second overlapping nomap region bypasses the check. If the second driver
later fails and clears the flag, could memblock_free_all() release the memory
to the buddy allocator while the first driver is still using it?

> +
> +	if (memblock_is_region_reserved(base, size)) {
> +		phys_addr_t end = base + size;

[Severity: Low]
Does this calculate an exclusive end address instead of an inclusive one?

If base + size reaches the physical address limit, could this overflow to 0
and print a misleading log message?

> +
> +		pr_err("Reserved memory: [%pa-%pa] overlaps an existing reservation, ignoring\n",
> +		       &base, &end);
> +		return -EBUSY;

[Severity: High]
If a statically placed region only partially overlaps an existing
reservation, does returning -EBUSY here without calling memblock_reserve()
leave the non-overlapping parts exposed to the buddy allocator?

To safely protect the non-overlapping physical pages from being used for
general kernel use, should the code still call memblock_reserve() before
returning -EBUSY?

> +	}
> +
>  	return memblock_reserve(base, size);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2