Re: [PATCH v5 3/3] arm64,ppc64le/kdump: pass dm-crypt keys to kdump kernel

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 25/02/26 11:33, Coiby Xu wrote:
> CONFIG_CRASH_DM_CRYPT has been introduced to support LUKS-encrypted
> device dump target by addressing two challenges [1],
>   - Kdump kernel may not be able to decrypt the LUKS partition. For some
>     machines, a system administrator may not have a chance to enter the
>     password to decrypt the device in kdump initramfs after the 1st kernel
>     crashes
>
>   - LUKS2 by default use the memory-hard Argon2 key derivation function
>     which is quite memory-consuming compared to the limited memory reserved
>     for kdump.
>
> To also enable this feature for ARM64 and PowerPC, the missing piece is
> to let the kdump kernel know where to find the dm-crypt keys which are
> randomly stored in memory reserved for kdump. Introduce a new device
> tree property dmcryptkeys [2] as similar to elfcorehdr to pass the
> memory address of the stored info of dm-crypt keys to the kdump kernel.
> Since this property is only needed by the kdump kernel, it won't be
> exposed to user space.
>
> [1] https://lore.kernel.org/all/20250502011246.99238-1-coxu@redhat.com/
> [2] https://github.com/devicetree-org/dt-schema/pull/181
>
> Cc: Arnaud Lefebvre <arnaud.lefebvre@clever-cloud.com>
> Cc: Baoquan he <bhe@redhat.com>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Kairui Song <ryncsn@gmail.com>
> Cc: Pingfan Liu <kernelfans@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Thomas Staudt <tstaudt@de.ibm.com>
> Cc: Sourabh Jain <sourabhjain@linux.ibm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Christophe Leroy (CS GROUP) <chleroy@kernel.org>
> Signed-off-by: Coiby Xu <coxu@redhat.com>
> ---
>   arch/arm64/kernel/machine_kexec_file.c |  4 ++++
>   arch/powerpc/kexec/elf_64.c            |  4 ++++
>   drivers/of/fdt.c                       | 21 +++++++++++++++++++++
>   drivers/of/kexec.c                     | 19 +++++++++++++++++++
>   4 files changed, 48 insertions(+)
>
> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
> index fba260ad87a9..e31fabed378a 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -134,6 +134,10 @@ int load_other_segments(struct kimage *image,
>   
>   		kexec_dprintk("Loaded elf core header at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
>   			      image->elf_load_addr, kbuf.bufsz, kbuf.memsz);
> +
> +		ret = crash_load_dm_crypt_keys(image);
> +		if (ret)
> +			goto out_err;
>   	}
>   #endif
>   
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 5d6d616404cf..ea50a072debf 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -79,6 +79,10 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>   			goto out;
>   		}
>   
> +		ret = crash_load_dm_crypt_keys(image);
> +		if (ret)
> +			goto out;
> +
>   		/* Setup cmdline for kdump kernel case */
>   		modified_cmdline = setup_kdump_cmdline(image, cmdline,
>   						       cmdline_len);
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 331646d667b9..2967e4aff807 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -866,6 +866,26 @@ static void __init early_init_dt_check_for_elfcorehdr(unsigned long node)
>   		 elfcorehdr_addr, elfcorehdr_size);
>   }
>   
> +static void __init early_init_dt_check_for_dmcryptkeys(unsigned long node)
> +{
> +	const char *prop_name = "linux,dmcryptkeys";
> +	const __be32 *prop;
> +
> +	if (!IS_ENABLED(CONFIG_CRASH_DM_CRYPT))
> +		return;
> +
> +	pr_debug("Looking for dmcryptkeys property... ");
> +
> +	prop = of_get_flat_dt_prop(node, prop_name, NULL);
> +	if (!prop)
> +		return;
> +
> +	dm_crypt_keys_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +
> +	/* Property only accessible to crash dump kernel */
> +	fdt_delprop(initial_boot_params, node, prop_name);
> +}
> +
>   static unsigned long chosen_node_offset = -FDT_ERR_NOTFOUND;
>   
>   /*
> @@ -1097,6 +1117,7 @@ int __init early_init_dt_scan_chosen(char *cmdline)
>   
>   	early_init_dt_check_for_initrd(node);
>   	early_init_dt_check_for_elfcorehdr(node);
> +	early_init_dt_check_for_dmcryptkeys(node);
>   
>   	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
>   	if (rng_seed && l > 0) {
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index c4cf3552c018..fbd253f0d3c5 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -423,6 +423,25 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>   		if (ret)
>   			goto out;
>   
> +		if (image->dm_crypt_keys_addr != 0) {
> +			ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
> +						       "linux,dmcryptkeys",
> +						       image->dm_crypt_keys_addr,
> +						       image->dm_crypt_keys_sz);
> +
> +			if (ret)
> +				goto out;
> +
> +			/*
> +			 * Avoid dmcryptkeys from being stomped on in kdump kernel by
> +			 * setting up memory reserve map.
> +			 */
> +			ret = fdt_add_mem_rsv(fdt, image->dm_crypt_keys_addr,
> +					      image->dm_crypt_keys_sz);
> +			if (ret)
> +				goto out;
> +		}
> +
>   #ifdef CONFIG_CRASH_DUMP
>   		/* add linux,usable-memory-range */
>   		ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,

The above changes look good to me.

Feel free to add:
Reviewed-by: Sourabh Jain <sourabhjain@linux.ibm.com>

But while reading crash_load_dm_crypt_keys() I noticed a possibility of a
double free at the address pointed by `keys_header`:

In crash_load_dm_crypt_keys()/crash_dump_dm_crypt.c
     snip...

     kbuf.buffer = keys_header;

     snip....

     r = kexec_add_buffer(&kbuf);
     if (r) {
         pr_err("Failed to call kexec_add_buffer, ret=%d\n", r);
         kvfree((void *)kbuf.buffer);                           <--- 
First Free
         return r;
     }

Since `keys_header` is not reset, the next call to build_keys_header()
will cause a double free at `keys_header`.

static int build_keys_header(void)
{

     snip...

     if (keys_header != NULL)
         kvfree(keys_header);

     snip...
}

What do you think?

- Sourabh Jain