Re: [PATCH v2 3/3] powerpc/kexec_file: Prevent kexec range truncation
From: Sourabh Jain
Date: Wed Jul 29 2026 - 04:27:21 EST
On 29/07/26 12:36, Jinjie Ruan wrote:
在 2026/7/29 14:44, Sourabh Jain 写道:
Yes, with the continue we can avoid the self-assignment.
On 29/07/26 11:56, Jinjie Ruan wrote:
在 2026/7/29 12:43, Sourabh Jain 写道:I agree that keeping or removing it doesn't change the functionality.
On 29/07/26 06:59, Jinjie Ruan wrote:Hi Sourabh,
Sashiko AI review pointed out the following issue.Although the below loop handles this but it is good to return early when
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 <sourabhjain@xxxxxxxxxxxxx>
Cc: Hari Bathini <hbathini@xxxxxxxxxxxxx>
Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Fixes: 180adfc532a8 ("powerpc/kexec_file: Add helper functions for
getting memory ranges")
Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
---
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;
there is
only range.
idx = 0;Yeah this changes is needed.
- 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);
else {Do we really need to remove the above condition?
idx++;
- if (i == idx)
- continue;
Isn't this condition is helpful till we find an overlap?
I believe there is no functional change here. The else branch itself
already indicates that there will be no overlap this time, so we can
safely use ranges[i] as the next memory region.
My point is that it helps avoid copying a range onto itself. For example:
Range A (idx = 0): [0x1000 - 0x2000]
Range B (idx = 1): [0x3000 - 0x5000]
Range C (idx = 2): [0x6000 - 0x8000]
For all three ranges, idx = i = x and kernel will do:
ranges[x] = ranges[x];
This self-assignment can be avoided if we keep the condition instead of
removing it. Isn't it?
If this review is fine, can it be updated and submitted directly, or
does it need to be updated in the new version?
A new version will be helpful. And feel free to add:
Reviewed-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
- Sourabh Jain
Whether we remove it or keep it is fine.
- Sourabh Jain
-
ranges[idx] = ranges[i];
}
}