[PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds

[PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds

[fuqiang wang]

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, 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
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.
 	 */
-	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.
+	 */
+	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

Re: [PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds

[Baoquan He]

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.

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
> 

Re: [PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds

[Baoquan He]

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

Re: [PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds

[fuqiang wang]

在 2023/12/24 12:46, Baoquan He 写道:

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

Hi Baoquan

Thank you very much for your suggestions in the patch log and code comments. I
have learned a lot and I will gradually improve.

I found the following patch in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=666ef13f2870c43ba8a402ec8a3cedf6eb6c6f5a

I'm sorry, It's my mistake. Do you think it is still necessary to merge this
patch based on that ?

Thanks a lot again
fuqiang

Re: [PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds

[Baoquan He]

On 12/25/23 at 09:44pm, fuqiang wang wrote:
> 在 2023/12/24 12:46, Baoquan He 写道:
> 
> > 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.
> > 
> 
> Hi Baoquan
> 
> Thank you very much for your suggestions in the patch log and code comments. I
> have learned a lot and I will gradually improve.
> 
> I found the following patch in linux-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=666ef13f2870c43ba8a402ec8a3cedf6eb6c6f5a
> 
> I'm sorry, It's my mistake. Do you think it is still necessary to merge this
> patch based on that ?

That patch need be withdrew because that is not expected according to
our discussion.

Hi Andrew,

Could you withdraw the patch fuqiang mentioned?

x86/crash: fix potential cmem->ranges array overflow
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=666ef13f2870c43ba8a402ec8a3cedf6eb6c6f5a

Thanks