Re: [PATCH v2 3/3] powerpc/kexec_file: Prevent kexec range truncation

On 29/07/26 06:59, Jinjie Ruan wrote:
> Sashiko AI review pointed out the following issue.
>
> The __merge_memory_ranges() function incorrectly handles overlapping
> memory ranges when merging them. Although sort_memory_ranges() sorts all
> ranges by their start address in ascending order beforehand, the merge
> logic remains defective in two ways:
>
> 1. It compares the current range's start against the previous element (i-1)
>     instead of the running target index (idx)
>
> 2. It unconditionally overwrites 'ranges[idx].end' with 'ranges[i].end'.
>
> This logic flaw leads to critical memory truncation when a larger memory
> range completely subsumes subsequent smaller ranges.
>
> For example, consider a sorted input array with three ranges:
>    Range A (idx=0): [0x1000 - 0x9000]
>    Range B (i=1):   [0x2000 - 0x5000] (completely inside Range A)
>    Range C (i=2):   [0x6000 - 0x8000] (completely inside Range A)
>
> 1. When i=1 (Range B):
>     ranges[1].start (0x2000) <= ranges[0].end + 1 (0x9001) is TRUE.
>     The code executes: ranges[0].end = ranges[1].end, which erroneously
>     shrinks Range A's end from 0x9000 down to 0x5000.
>
> 2. When i=2 (Range C):
>     ranges[2].start (0x6000) <= ranges[1].end + 1 (0x5001) is FALSE.
>     The code falls into the else block, creating a broken new range.
>
> As a result, valid memory fragments [0x5001 - 0x5fff] and [0x8001 - 0x9000]
> are completely lost from the kexec exclude lists, potentially allowing
> the crash kernel to overwrite active memory, causing data corruption
> or crashes.
>
> Fix this by ensuring the start of the current range is compared against the
> end of the active merged range (idx), and use max() to safely prevent the
> outer boundary from being truncated.
>
> Cc: Sourabh Jain <[email protected]>
> Cc: Hari Bathini <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: [email protected]
> Fixes: 180adfc532a8 ("powerpc/kexec_file: Add helper functions for getting memory ranges")
> Signed-off-by: Jinjie Ruan <[email protected]>
> ---
>   arch/powerpc/kexec/ranges.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/kexec/ranges.c b/arch/powerpc/kexec/ranges.c
> index e5fea23b191b..539061d14a77 100644
> --- a/arch/powerpc/kexec/ranges.c
> +++ b/arch/powerpc/kexec/ranges.c
> @@ -21,6 +21,7 @@
>   #include <linux/of.h>
>   #include <linux/slab.h>
>   #include <linux/memblock.h>
> +#include <linux/minmax.h>
>   #include <linux/crash_core.h>
>   #include <asm/sections.h>
>   #include <asm/kexec_ranges.h>
> @@ -105,19 +106,16 @@ static void __merge_memory_ranges(struct crash_mem *mem_rngs)
>   	struct range *ranges;
>   	int i, idx;
>   
> -	if (!mem_rngs)
> +	if (!mem_rngs || mem_rngs->nr_ranges <= 1)
>   		return;

Although the below loop handles this but it is good to return early when 
there is
only range.

>   
>   	idx = 0;
> -	ranges = &(mem_rngs->ranges[0]);
> +	ranges = mem_rngs->ranges;
>   	for (i = 1; i < mem_rngs->nr_ranges; i++) {
> -		if (ranges[i].start <= (ranges[i-1].end + 1))
> -			ranges[idx].end = ranges[i].end;
> +		if (ranges[i].start <= (ranges[idx].end + 1))
> +			ranges[idx].end = max(ranges[idx].end, ranges[i].end);

Yeah this changes is needed.

>   		else {
>   			idx++;
> -			if (i == idx)
> -				continue;

Do we really need to remove the above condition?

Isn't this condition is helpful till we find an overlap?

- Sourabh Jain

> -
>   			ranges[idx] = ranges[i];
>   		}
>   	}