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

On 29/07/26 12:36, Jinjie Ruan wrote:
>
> 在 2026/7/29 14:44, Sourabh Jain 写道:
>>
>> On 29/07/26 11:56, Jinjie Ruan wrote:
>>> 在 2026/7/29 12:43, Sourabh Jain 写道:
>>>> 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?
>>> Hi Sourabh,
>>>
>>> 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.
>> I agree that keeping or removing it doesn't change the functionality.
>>
>> 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?
> Yes, with the continue we can avoid the self-assignment.
>
> 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 <[email protected]>

>
>> - Sourabh Jain
>>
>>
>>> Whether we remove it or keep it is fine.
>>>
>>>> - Sourabh Jain
>>>>
>>>>> -
>>>>>                 ranges[idx] = ranges[i];
>>>>>             }
>>>>>         }