| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL_JsqK1Pb9nAeL84EP2U3MQgpBsm+E_0QXmzbigWXnS245WPQ@mail.gmail.com>
Date: Thu, 4 Feb 2021 13:26:24 -0600
From: Rob Herring <robh@...nel.org>
To: Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
Cc: Mimi Zohar <zohar@...ux.ibm.com>,
Thiago Jung Bauermann <bauerman@...ux.ibm.com>,
"AKASHI, Takahiro" <takahiro.akashi@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Will Deacon <will@...nel.org>, Joe Perches <joe@...ches.com>,
Catalin Marinas <catalin.marinas@....com>,
Michael Ellerman <mpe@...erman.id.au>,
James Morse <james.morse@....com>,
Sasha Levin <sashal@...nel.org>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Frank Rowand <frowand.list@...il.com>,
vincenzo.frascino@....com, Mark Rutland <mark.rutland@....com>,
dmitry.kasatkin@...il.com, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>,
Pavel Tatashin <pasha.tatashin@...een.com>,
Allison Randal <allison@...utok.net>,
Masahiro Yamada <masahiroy@...nel.org>,
Bhupesh Sharma <bhsharma@...hat.com>,
Matthias Brugger <mbrugger@...e.com>,
Hsin-Yi Wang <hsinyi@...omium.org>, tao.li@...o.com,
Christophe Leroy <christophe.leroy@....fr>,
Prakhar Srivastava <prsriva@...ux.microsoft.com>,
balajib@...ux.microsoft.com, linux-integrity@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
devicetree@...r.kernel.org,
linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH v16 11/12] powerpc: Use OF alloc and free for FDT
On Thu, Feb 4, 2021 at 10:42 AM Lakshmi Ramasubramanian
<nramas@...ux.microsoft.com> wrote:
>
> of_alloc_and_init_fdt() and of_free_fdt() have been defined in
> drivers/of/kexec.c to allocate and free memory for FDT.
>
> Use of_alloc_and_init_fdt() and of_free_fdt() to allocate and
> initialize the FDT, and to free the FDT respectively.
>
> powerpc sets the FDT address in image_loader_data field in
> "struct kimage" and the memory is freed in
> kimage_file_post_load_cleanup(). This cleanup function uses kfree()
> to free the memory. But since of_alloc_and_init_fdt() uses kvmalloc()
> for allocation, the buffer needs to be freed using kvfree().
You could just change the kexec core to call kvfree() instead.
> Define "fdt" field in "struct kimage_arch" for powerpc to store
> the address of FDT, and free the memory in powerpc specific
> arch_kimage_file_post_load_cleanup().
However, given all the other buffers have an explicit field in kimage
or kimage_arch, changing powerpc is to match arm64 is better IMO.
> Signed-off-by: Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
> Suggested-by: Rob Herring <robh@...nel.org>
> Suggested-by: Thiago Jung Bauermann <bauerman@...ux.ibm.com>
> ---
> arch/powerpc/include/asm/kexec.h | 2 ++
> arch/powerpc/kexec/elf_64.c | 26 ++++++++++++++++----------
> arch/powerpc/kexec/file_load_64.c | 3 +++
> 3 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
> index 2c0be93d239a..d7d13cac4d31 100644
> --- a/arch/powerpc/include/asm/kexec.h
> +++ b/arch/powerpc/include/asm/kexec.h
> @@ -111,6 +111,8 @@ struct kimage_arch {
> unsigned long elf_headers_mem;
> unsigned long elf_headers_sz;
> void *elf_headers;
> +
> + void *fdt;
> };
>
> char *setup_kdump_cmdline(struct kimage *image, char *cmdline,
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index d0e459bb2f05..51d2d8eb6c1b 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -19,6 +19,7 @@
> #include <linux/kexec.h>
> #include <linux/libfdt.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> #include <linux/of_fdt.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -32,7 +33,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> unsigned int fdt_size;
> unsigned long kernel_load_addr;
> unsigned long initrd_load_addr = 0, fdt_load_addr;
> - void *fdt;
> + void *fdt = NULL;
> const void *slave_code;
> struct elfhdr ehdr;
> char *modified_cmdline = NULL;
> @@ -103,18 +104,12 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> }
>
> fdt_size = fdt_totalsize(initial_boot_params) * 2;
> - fdt = kmalloc(fdt_size, GFP_KERNEL);
> + fdt = of_alloc_and_init_fdt(fdt_size);
> if (!fdt) {
> pr_err("Not enough memory for the device tree.\n");
> ret = -ENOMEM;
> goto out;
> }
> - ret = fdt_open_into(initial_boot_params, fdt, fdt_size);
> - if (ret < 0) {
> - pr_err("Error setting up the new device tree.\n");
> - ret = -EINVAL;
> - goto out;
> - }
>
> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
The first thing this function does is call setup_new_fdt() which first
calls of_kexec_setup_new_fdt(). (Note, I really don't understand the
PPC code split. It looks like there's a 32-bit and 64-bit split, but
32-bit looks broken to me. Nothing ever calls setup_new_fdt() except
setup_new_fdt_ppc64()). The arm64 version is calling
of_alloc_and_init_fdt() and then of_kexec_setup_new_fdt() directly.
So we can just make of_alloc_and_init_fdt() also call
of_kexec_setup_new_fdt() (really, just tweak of_kexec_setup_new_fdt do
the alloc and copy). I don't think the architecture needs to pick the
size either. It's doubtful that either one is that sensitive to the
amount of extra space.
> initrd_len, cmdline);
> @@ -131,6 +126,10 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> ret = kexec_add_buffer(&kbuf);
> if (ret)
> goto out;
> +
> + /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> + image->arch.fdt = fdt;
> +
> fdt_load_addr = kbuf.mem;
>
> pr_debug("Loaded device tree at 0x%lx\n", fdt_load_addr);
> @@ -145,8 +144,15 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> kfree(modified_cmdline);
> kexec_free_elf_info(&elf_info);
>
> - /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> - return ret ? ERR_PTR(ret) : fdt;
> + /*
> + * Once FDT buffer has been successfully passed to kexec_add_buffer(),
> + * the FDT buffer address is saved in image->arch.fdt. In that case,
> + * the memory cannot be freed here in case of any other error.
> + */
> + if (ret && !image->arch.fdt)
> + of_free_fdt(fdt);
Just call kvfree() directly.
> +
> + return ret ? ERR_PTR(ret) : NULL;
> }
>
> const struct kexec_file_ops kexec_elf64_ops = {
> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
> index 3cab318aa3b9..d9d5b5569a6d 100644
> --- a/arch/powerpc/kexec/file_load_64.c
> +++ b/arch/powerpc/kexec/file_load_64.c
> @@ -1113,5 +1113,8 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image)
> image->arch.elf_headers = NULL;
> image->arch.elf_headers_sz = 0;
>
> + of_free_fdt(image->arch.fdt);
> + image->arch.fdt = NULL;
> +
> return kexec_image_post_load_cleanup_default(image);
> }
> --
> 2.30.0
>
Powered by blists - more mailing lists