Re: [PATCH v2 1/2] x86/kexec: Fix potential out of bounds in crash_setup_memmap_entries()

[fuqiang wang]

在 2023/12/21 21:14, Baoquan He 写道:

> On 12/20/23 at 01:57pm, fuqiang wang wrote:
> > In memmap_exclude_ranges(), there will exclude elfheader from
> > 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
> > elfheader in crashk_res. To avoid potential out of bounds in future, Set
> > the array size to 2.
> If so, I would suggest to add extra slot for low 1M too in
> fill_up_crash_elf_data() lest the low 1M could be changed in the future,
> e.g [start, 1M].


Hi Baoquan

This seems to be better for future maintenance. Thank you for your suggestion.

> > But similar issue will not exist in fill_up_crash_elf_data(). Because
> > the range to be excluded is [0, 1M], start (0) is special and will not
> > appear in the middle of existing cmem->ranges[]. I added a comment to
> > explain it.
> >
> > Signed-off-by: fuqiang wang <fuqiang.wang@xxxxxxxxxxxx>
> > ---
> >   arch/x86/kernel/crash.c | 20 ++++++++++++++------
> >   1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index c92d88680dbf..1c15d0884c90 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -149,6 +149,13 @@ 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.
> >   	 */
> >   	nr_ranges += 2;
> >   	cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
> > @@ -282,9 +289,15 @@ 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;
> > +	cmem->max_nr_ranges = 2;
> > +
> > +	/* Exclude some ranges from crashk_res and add rest to memmap */
> > +	ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
> > +	if (ret)
> > +		goto out;
> >   
> >   	memset(&cmd, 0, sizeof(struct crash_memmap_data));
> >   	cmd.params = params;
> > @@ -320,11 +333,6 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
> >   		add_e820_entry(params, &ei);
> >   	}
> >   
> > -	/* Exclude some ranges from crashk_res and add rest to memmap */
> > -	ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
> > -	if (ret)
> > -		goto out;
> And you didn't mention moving above code block up in log. I would
> suggest keeping it as is because it looks more reasonable to be adjacent
> to the following cmem->ranges[] handling.

Yes, baoquan, keeping it as it is may be more coherent.I will post a new patch later.

Thanks
fuqiang

> > -
> >   	for (i = 0; i < cmem->nr_ranges; i++) {
> >   		ei.size = cmem->ranges[i].end - cmem->ranges[i].start + 1;
> >   
> > --
> > 2.42.0