Received-SPF: pass (google.com: domain of linux-kernel+bounces-5229-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <273284e8-7680-4f5f-8065-c5d780987e59@easystack.cn>
Date: Tue, 19 Dec 2023 20:54:02 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] kexec: avoid out of bounds in crash_exclude_mem_range()
Content-Language: en-US
To: Yuntao Wang <ytcoode@gmail.com>
Cc: bhe@redhat.com, dyoung@redhat.com, kexec@lists.infradead.org,
 linux-kernel@vger.kernel.org, vgoyal@redhat.com
References: <3765549d-892e-4102-9b56-9add1d0a8089@easystack.cn>
 <20231219103928.98465-1-ytcoode@gmail.com>
From: fuqiang wang <fuqiang.wang@easystack.cn>
In-Reply-To: <20231219103928.98465-1-ytcoode@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

在 2023/12/19 18:39, Yuntao Wang 写道:

> On Tue, 19 Dec 2023 16:55:16 +0800, fuqiang wang <fuqiang.wang@easystack.cn> wrote:
>
>> 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().
> I don't quite understand what you're trying to express.
Hi Yuntao,

I make the following changes based on your patch. This change can increase code
readability on one hand, On the other hand, if these functions return errors,
the rest process of crash_setup_memmap_entries() can be skipped.

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index c92d88680dbf..67a974c041b9 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -285,6 +285,12 @@ 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;
+
+       /* 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 +326,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;
-
         for (i = 0; i < cmem->nr_ranges; i++) {
                 ei.size = cmem->ranges[i].end - cmem->ranges[i].start + 1;
>> 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.
> Because elfcorehdr is the first one and only one that allocates memory from the
> starting address of crashk_res, and the starting address of crashk_res meets
> the alignment requirement of elfcorehdr.
>
> elfcorehdr requires 4k alignment, and the starting address of crashk_res is
> 16M aligned.
>
> Therefore, image->elf_load_addr should be equal to crashk_res.start.
Yes! you read the code very carefully and I didn't notice that! However, the
location of elfheader in crashk_res.start is highly dependent on elfheader in
crashk_res memory allocation order and position. At present, x86 first allocate
the memory of elfheader. However, ppc64 doesn't seem to be like this (It first
executes load_backup_segment()). Although arm64 allocates elfheader first, it
sets kbuf.top_down to true in load_other_segments(). This will cause the
elfheader to be allocated near crashk_res.end. I debugged using crash on the
arm64 machine and the result is(Although the kernel version of the testing
machine may be a bit low, the process of allocating elfheaders is consistent
with upstream):

     crash> p crashk_res.start
     $6 = 1375731712
     crash> p crashk_res.end
     $7 = 2147483647
     crash> p kexec_crash_image.arch.elf_headers_mem
     $9 = 2147352576

So I think it's best to set cmem->max_nr_ranges to 2 for easy maintenance in
the future. What do you think about ?