Messages in this thread | | | Date | Tue, 19 Dec 2023 16:55:16 +0800 | Subject | Re: [PATCH] kexec: avoid out of bounds in crash_exclude_mem_range() | From | fuqiang wang <> |
| |
在 2023/12/19 13:29, Yuntao Wang 写道: > On Tue, 19 Dec 2023 11:50:32 +0800, fuqiang wang <fuqiang.wang@easystack.cn> 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@gmail.com/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!
Hi Yuntao,
Thanks for your understanding and encourage. :)
>> 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.
Thank you very much for your patient comment. This change does indeed improve readability. But as a combination of these two, how do you feel about moving crash_setup_memmap_entries() behind vzalloc(). >> 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.
The image->elf_load_addr is determined by arch_kexec_locate_mem_hole(), this function can ensure that the value is within the range of [crashk_res.start, crashk_res.end), but it seems that it cannot guarantee that its value will always be equal to crashk_res.start. Perhaps I have some omissions, please point them out.
Thanks fuqiang >> Thanks >> fuqiang
| |