Message-ID: <aB3RqS85p6DiHKHm@MiWiFi-R3L-srv>
Date: Fri, 9 May 2025 17:58:01 +0800
From: Baoquan He <bhe@...hat.com>
To: Coiby Xu <coxu@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	fuqiang wang <fuqiang.wang@...ystack.cn>,
	Vivek Goyal <vgoyal@...hat.com>, Dave Young <dyoung@...hat.com>,
	kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH v4] x86/kexec: fix potential cmem->ranges out of bounds

On 05/09/25 at 12:04pm, Coiby Xu wrote:
> On Wed, May 07, 2025 at 10:59:59PM -0700, Andrew Morton wrote:
> > On Thu, 8 May 2025 12:25:15 +0800 Coiby Xu <coxu@...hat.com> wrote:
> > 
> > > >
> > > >Acked-by: Baoquan He <bhe@...hat.com>
> > > 
> > > Hi Andrew,
> > > 
> > > It seems this patch was missed.
> > 
> > January 2024.  Yes, it's fair to assume that it was missed ;)
> > 
> > > Will you pick it up?
> > 
> > Sure.
> 
> Thanks for quickly processing this patch! Sorry I didn't reply yesterday
> as I was trying to reproduce the UBSAN warning and truly understand the
> it.
> 
> > 
> > > Without this patch,
> > > kdump kernel will fail to be loaded by the kexec_file_load,
> 
> As already pointed out by Baoquan, a manual test shows kexec_file_load
> actually works despite the UBSAN warning. Sorry I misinterpreted the
> UBSAN warning and the automated test result failure (somehow sysrq
> wasn't be triggered and vmcore wasn't saved either).
> 
> 
> > > 
> > >   [  139.736948] UBSAN: array-index-out-of-bounds in arch/x86/kernel/crash.c:350:25
> > >   [  139.742360] index 0 is out of range for type 'range [*]'
> [...]
> > > 
> > 
> > Do we know why this has appeared at such a late date?  The reporter
> > must be doing something rare.
> 
> The UBSAN warning happens because flexible array members annotated with
> __counted_by are accessed without assigning an array element count i.e.
> crash_mem->ranges[0] is accessed without setting max_nr_ranges after
> vzalloc,
> 
>     // include/linux/crash_core.h
>     struct crash_mem {
>     	unsigned int max_nr_ranges;
>     	unsigned int nr_ranges;
>     	struct range ranges[] __counted_by(max_nr_ranges);
>     };
> 
> The bad commit was introduced in 2021 but only recent gcc-15 supports
> __counted_by. That's why we don't see this UBSAN warning until this
> year. And although this UBSAN warning is scary enough, fortunately it
> doesn't cause a real problem.
> 
> > 
> > Baoquan, please re-review this?
> > 
> > A -stable backport is clearly required.  A Fixes: would be nice, but I
> > assume this goes back a long time so it isn't worth spending a lot of
> > time working out when this was introduced.
> 
> So I believe the correct fix should be as follows,

Thanks for testing and investigation into these. Could you arrange this
into formal patches based on your testing and analysis? 

It would be great if you can include Fuqiang's patch since it has
conflict with your LUKS patch. This can facilitate patch merging for
Andrew. Thanks in advance.

> 
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -301,6 +301,7 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>         cmem = vzalloc(struct_size(cmem, ranges, 1));
>         if (!cmem)
>                 return -ENOMEM;
> +       cmem->max_nr_ranges = 1;
>         memset(&cmd, 0, sizeof(struct crash_memmap_data));
>         cmd.params = params;
> 
> 
> And a Fixes tag should be dedicated to commit
> 5849cdf8c120 ("x86/crash: Fix crash_setup_memmap_entries() out-of-bounds access")
> which forgot to set cmem->max_nr_ranges=1.
> 
> > 
> > The patch needed a bit of work to apply to current code.  I did the
> > below.  It compiles.
> > 
> > --- a/arch/x86/kernel/crash.c~x86-kexec-fix-potential-cmem-ranges-out-of-bounds
> > +++ a/arch/x86/kernel/crash.c
> > @@ -165,8 +165,18 @@ static struct crash_mem *fill_up_crash_e
> > 	/*
> > 	 * 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.
> > 	 */
> > -	nr_ranges += 2;
> > +	nr_ranges += 3;
> > 	cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
> > 	if (!cmem)
> > 		return NULL;
> > @@ -317,9 +327,16 @@ int crash_setup_memmap_entries(struct ki
> > 	 * split. So use two slots here.
> > 	 */
> > 	nr_ranges = 2;
> > -	cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
> > +	/*
> > +	 * 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.
> > +	 */
> > +	cmem = vzalloc(struct_size(cmem, ranges, 2));
> > 	if (!cmem)
> > 		return -ENOMEM;
> > +	cmem->max_nr_ranges = 2;
> 
> Thanks for coming up with the above patch! I think the goal of this
> patch is addressing a different issue but it also fixes the UBSAN
> warning because cmem->max_nr_ranges is now set.
> 
> > 
> > 	cmem->max_nr_ranges = nr_ranges;
> > 	cmem->nr_ranges = 0;
> > _
> 
> 
> -- 
> Best regards,
> Coiby
> 

Powered by blists - more mailing lists