Received-SPF: pass (google.com: domain of linux-kernel+bounces-10633-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 24 Dec 2023 12:46:00 +0800
From: Baoquan He <bhe@redhat.com>
To: fuqiang wang <fuqiang.wang@easystack.cn>
Cc: Vivek Goyal <vgoyal@redhat.com>, Dave Young <dyoung@redhat.com>,
	kexec@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds
Message-ID: <ZYe3iN816iiKDwIu@MiWiFi-R3L-srv>
References: <20231222121855.148215-1-fuqiang.wang@easystack.cn>
 <ZYWPHSmwK8iG6xUr@MiWiFi-R3L-srv>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZYWPHSmwK8iG6xUr@MiWiFi-R3L-srv>

On 12/22/23 at 09:29pm, Baoquan He wrote:
> On 12/22/23 at 08:18pm, fuqiang wang wrote:
> > In memmap_exclude_ranges(), there will exclude elfheader from
>                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       elfheader will be excluded from crashk_res. OR
>       it will exclude elfheader from crashk_res.
> 
> > crashk_res. In the current x86 architecture code, the elfheader is
> > always allocated at crashk_res.start. It seems that there won't be a
> > split a new range. But it depends on the allocation position of
>  ~~~~~~~~~~~~~~~~~~
>   It seems that there won't be a new split range.
> > elfheader in crashk_res. To avoid potential out of bounds in future, add
> > a extra slot.
> > 
> > The similar issue also exists in fill_up_crash_elf_data(). The range to
> > be excluded is [0, 1M], start (0) is special and will not appear in the
> > middle of existing cmem->ranges[]. But in order to lest the low 1M could
>                                          ~~~~~~~~~~~~~~~~
>                                           in case
> > be changed in the future, add a extra slot too.
> > 
> > Previously discussed link:
> > [1] https://lore.kernel.org/kexec/ZXk2oBf%2FT1Ul6o0c@MiWiFi-R3L-srv/
> > [2] https://lore.kernel.org/kexec/273284e8-7680-4f5f-8065-c5d780987e59@easystack.cn/
> > [3] https://lore.kernel.org/kexec/ZYQ6O%2F57sHAPxTHm@MiWiFi-R3L-srv/
> > 
> > Signed-off-by: fuqiang wang <fuqiang.wang@easystack.cn>
> > ---
> >  arch/x86/kernel/crash.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index c92d88680dbf..97d33a6fc4fb 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -149,8 +149,18 @@ static struct crash_mem *fill_up_crash_elf_data(void)
> >  	/*
> >  	 * Exclusion of crash region and/or crashk_low_res may cause
> >  	 * another range split. So add extra two slots here.
> > +	 *
> > +	 * Exclusion of low 1M may not cause another range split, because the
> > +	 * range of exclude is [0, 1M] 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, the start of [0, 1M] cannot meet this
> > +	 * condition.
> > +	 *
> > +	 * But in order to lest the low 1M could be changed in the future,
> > +	 * (e.g. [stare, 1M]), add a extra slot.


Rethink about this, seems above code comment is fine to be kept, and the 
same feeling about the elfheader region split from crashk_res. So, other
than the patch log concerns, this patch looks good to me. Let's see if
other people has concern about the newly added comments.

> 
> Sometime, too much is as bad as too little. I feel below words are
> enough to state three regions are gonna be excluded, and may cause
> another split (may not cause). The code comment plus commit log can help
> people know why they are needed.
> 
>   	 * Exclusion of low1M, crashk_res and/or crashk_low_res may cause
>   	 * another range split. So add extra three slots here.
> 
> >  	 */
> > -	nr_ranges += 2;
> > +	nr_ranges += 3;
> >  	cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
> >  	if (!cmem)
> >  		return NULL;
> > @@ -282,9 +292,16 @@ 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));
> > +	/*
> > +	 * In the current x86 architecture code, the elfheader is always
> > +	 * allocated at crashk_res.start. But it depends on the allocation
> > +	 * position of elfheader in crashk_res. To avoid potential out of
> > +	 * bounds in future, add a extra slot.
> > +	 */
> 
> Ditto.
> 
>  +	/*
>  +	 * Elfheader gonna be excluded from crashk_res, to avoid potential
>  +	 * out of bounds, add one extra slot.
>  +	 */
> 
> > +	cmem = vzalloc(struct_size(cmem, ranges, 2));
> >  	if (!cmem)
> >  		return -ENOMEM;
> > +	cmem->max_nr_ranges = 2;
> >  
> >  	memset(&cmd, 0, sizeof(struct crash_memmap_data));
> >  	cmd.params = params;
> > -- 
> > 2.42.0
> > 
>