Message-ID: <ZXrIDNvvlpZyiLYK@MiWiFi-R3L-srv>
Date:   Thu, 14 Dec 2023 17:17:00 +0800
From:   Baoquan He <bhe@...hat.com>
To:     fuqiang wang <fuqiang.wang@...ystack.cn>
Cc:     Vivek Goyal <vgoyal@...hat.com>, Dave Young <dyoung@...hat.com>,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kexec: avoid out of bounds in crash_exclude_mem_range()

On 12/13/23 at 09:10pm, fuqiang wang wrote:
> 在 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.

OK, this is an special case.

> > @@ -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.

Yeah, the init need be added.

> 
> 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?

I don't see the difference between your patch and the current code.
Please see my comment in your earlier example.

Powered by blists - more mailing lists