Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <92a1bdff-e988-48ff-8e78-2998834a3e02@easystack.cn>
Date:   Wed, 13 Dec 2023 21:10:01 +0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] kexec: avoid out of bounds in crash_exclude_mem_range()
Content-Language: en-US
To:     Baoquan He <bhe@redhat.com>
Cc:     Vivek Goyal <vgoyal@redhat.com>, Dave Young <dyoung@redhat.com>,
        kexec@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20231127025641.62210-1-fuqiang.wang@easystack.cn>
 <ZWg9aZYoo0v+tCQ8@MiWiFi-R3L-srv>
 <e588c619-9b97-4627-bce1-b595c757e5c4@easystack.cn>
 <ZXk2oBf/T1Ul6o0c@MiWiFi-R3L-srv>
From:   fuqiang wang <fuqiang.wang@easystack.cn>
In-Reply-To: <ZXk2oBf/T1Ul6o0c@MiWiFi-R3L-srv>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

在 2023/12/13 12:44, Baoquan He 写道:

> On 11/30/23 at 09:20pm, fuqiang wang wrote:
>> On 2023/11/30 15:44, Baoquan He wrote:
>>> On 11/27/23 at 10:56am, fuqiang wang wrote:
>>>> When the split happened, judge whether mem->nr_ranges is equal to
>>>> mem->max_nr_ranges. If it is true, return -ENOMEM.
>>>>
>>>> The advantage of doing this is that it can avoid array bounds caused by
>>>> some bugs. E.g., Before commit 4831be702b95 ("arm64/kexec: Fix missing
>>>> extra range for crashkres_low."), reserve both high and low memories for
>>>> the crashkernel may cause out of bounds.
>>>>
>>>> On the other hand, move this code before the split to ensure that the
>>>> array will not be changed when return error.
>>> If out of array boundary is caused, means the laoding failed, whether
>>> the out of boundary happened or not. I don't see how this code change
>>> makes sense. Do I miss anything?
>>>
>>> Thanks
>>> Baoquan
>>>
>> Hi baoquan,
>>
>> In some configurations, out of bounds may not cause crash_exclude_mem_range()
>> returns error, then the load will succeed.
>>
>> E.g.
>> There is a cmem before execute crash_exclude_mem_range():
>>
>>    cmem = {
>>      max_nr_ranges = 3
>>      nr_ranges = 2
>>      ranges = {
>>         {start = 1,      end = 1000}
>>         {start = 1001,    end = 2000}
>>      }
>>    }
>>
>> After executing twice crash_exclude_mem_range() with the start/end params
>> 100/200, 300/400 respectively, the cmem will be:
>>
>>    cmem = {
>>      max_nr_ranges = 3
>>      nr_ranges = 4                    <== nr_ranges > max_nr_ranges
>>      ranges = {
>>        {start = 1,       end = 99  }
>>        {start = 201,     end = 299 }
>>        {start = 401,     end = 1000}
>>        {start = 1001,    end = 2000}  <== OUT OF BOUNDS
>>      }
>>    }
>>
>> When an out of bounds occurs during the second execution, the function will not
>> return error.
>>
>> Additionally, when the function returns error, means the load failed. It seems
>> meaningless to keep the original data unchanged. But in my opinion, this will
>> make this function more rigorous and more versatile. (However, I am not sure if
>> it is self-defeating and I hope to receive more suggestions).
> Sorry for late reply.
>
> I checked the code again, there seems to be cases out of bounds occur
> very possiblly. We may need to enlarge the cmem array to avoid the risk.
>
> In below draft code, we need add another slot to exclude the low 1M area
> when preparing elfcorehdr. And to exclude the elf header region from
> crash kernel region, we need create the cmem with 2 slots.
>
> With these change, we can absolutely avoid out of bounds occurence.
> What do you think?
>
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 1715e5f06a59..21facabcf699 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -147,10 +147,10 @@ static struct crash_mem *fill_up_crash_elf_data(void)
>   		return NULL;
>   
>   	/*
> -	 * Exclusion of crash region and/or crashk_low_res may cause
> -	 * another range split. So add extra two slots here.
> +	 * Exclusion of low 1M, crash region and/or crashk_low_res may
> +	 * cause another range split. So add extra two slots here.
>   	 */
> -	nr_ranges += 2;
> +	nr_ranges += 3;
>   	cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
>   	if (!cmem)
>   		return NULL;
Hi baoquan,

Exclusion of low 1M may not cause new region. Because when calling
crash_exclude_mem_range(), the start parameter is 0 and the condition for
splitting a new region is that the start, end parameters are both in a certain
existing region in cmem and cannot be equal to existing region's start or end.
Obviously, start (0) cannot meet this condition.
> @@ -282,7 +282,7 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>   	struct crash_memmap_data cmd;
>   	struct crash_mem *cmem;
>   
> -	cmem = vzalloc(struct_size(cmem, ranges, 1));
> +	cmem = vzalloc(struct_size(cmem, ranges, 2));
>   	if (!cmem)
>   		return -ENOMEM;
>   
>
Yes, you are right. Exclude the elf header region from crash kernel region may
cause split a new region. And there seems to be another issue with this code
path: Before calling crash_exclude_mem_range(), cmem->max_nr_ranges was not
initialized.

In my opinion, these change can absolutely avoid out of bounds occurence. But
when we forget to modify max_nr_ranges due to a mistakes in the future, is it
better to report it by returning an error through crash_exclude_mem_range().
What do you think about it?

Thanks
fuqiang