Re: [PATCH] kexec: avoid out of bounds in crash_exclude_mem_range()

[Yuntao Wang]

On Tue, 19 Dec 2023 11:50:32 +0800, fuqiang wang <fuqiang.wang@xxxxxxxxxxxx> wrote:
> 在 2023/12/19 10:47, Yuntao Wang 写道:
> 
> > Hi fuqiang,
> >
> > Yesterday, I posted two patches that happen to address the bugs you an Baoquan
> > are currently discussing here, I wasn't aware that you both were also working
> > on fixing these issues.
> >
> > Baoquan suggested I talk to you about it.
> >
> > If you're interested, you can take a look at my patches and review them to see
> > if there are any issues. If everything is fine, and if you're willing, you can
> > also add a 'Reviewed-by' tag there.
> >
> > The following link is for the two patches I posted yesterday:
> >
> > https://lore.kernel.org/lkml/20231218081915.24120-3-ytcoode@xxxxxxxxx/t/#u
> >
> > Sincerely,
> > Yuntao
> 
> Hi Yuntao,
> 
> I'm glad you've also noticed this issue. But I'm sorry, I want to solve this
> problem myself because this is my first time posting a patch in the community,
> and I cherish this opportunity very much.

I can truly understand your feelings because I still remember how thrilled I
was when my first patch got merged. So keep it up!

> 
> I have carefully reviewed your patch. There is some changes where my views differ
> from yours:
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index c92d88680dbf..3be46f4b441e 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -282,10 +282,6 @@ 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));
> -    if (!cmem)
> -        return -ENOMEM;
> -
>       memset(&cmd, 0, sizeof(struct crash_memmap_data));
>       cmd.params = params;
> 
> @@ -321,6 +317,11 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>       }
> 
>       /* Exclude some ranges from crashk_res and add rest to memmap */
> +    cmem = vzalloc(struct_size(cmem, ranges, 1));
> +    if (!cmem)
> +        return -ENOMEM;
> +    cmem->max_nr_ranges = 1;
> +
>       ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
>       if (ret)
>           goto out;
> 
> 1. I don't feel very good that you have moved vzalloc() to in front of
> memmap_exclude_ranges. Because if memory allocation fails, there is no need to
> do anything else afterwards.

I moved it here because only memmap_exclude_ranges() and the code below it use cmem.

I think it is a good practice to put related code together, which also improves
code readability.

> 
> 2. The cmem->max_nr_ranges should be set to 2. Because in
> memmap_exclude_ranges, a cmem->ranges[] will be filled in and if a split occurs
> later, another one will be added.

With the current code, image->elf_load_addr should be equal to crashk_res.start,
so split will not occur in crash_exclude_mem_range(). Therefore, setting
cmem->max_nr_ranges to 1 is safe.

> 
> Thanks
> fuqiang