[PATCH v6 0/7] Support kdump with LUKS encryption by reusing LUKS volume keys

[PATCH v6 0/7] Support kdump with LUKS encryption by reusing LUKS volume keys

[Coiby Xu]

LUKS is the standard for Linux disk encryption, widely adopted by users,
and in some cases, such as Confidential VMs, it is a requirement. With 
kdump enabled, when the first kernel crashes, the system can boot into
the kdump/crash kernel to dump the memory image (i.e., /proc/vmcore) 
to a specified target. However, there are two challenges when dumping
vmcore to a LUKS-encrypted device:

 - 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; For cloud confidential VMs, depending on the policy the
   kdump kernel may not be able to unseal the keys with TPM and the
   console virtual keyboard is untrusted.

 - LUKS2 by default use the memory-hard Argon2 key derivation function
   which is quite memory-consuming compared to the limited memory reserved
   for kdump. Take Fedora example, by default, only 256M is reserved for
   systems having memory between 4G-64G. With LUKS enabled, ~1300M needs
   to be reserved for kdump. Note if the memory reserved for kdump can't
   be used by 1st kernel i.e. an user sees ~1300M memory missing in the
   1st kernel.

Besides users (at least for Fedora) usually expect kdump to work out of
the box i.e. no manual password input or custom crashkernel value is
needed. And it doesn't make sense to derivate the keys again in kdump
kernel which seems to be redundant work.

This patch set addresses the above issues by making the LUKS volume keys
persistent for kdump kernel with the help of cryptsetup's new APIs
(--link-vk-to-keyring/--volume-key-keyring). Here is the life cycle of
the kdump copies of LUKS volume keys,

 1. After the 1st kernel loads the initramfs during boot, systemd
    use an user-input passphrase to de-crypt the LUKS volume keys
    or TPM-sealed key and then save the volume keys to specified keyring
    (using the --link-vk-to-keyring API) and the key will expire within
    specified time.

 2. A user space tool (kdump initramfs loader like kdump-utils) create
    key items inside /sys/kernel/config/crash_dm_crypt_keys to inform
    the 1st kernel which keys are needed.

 3. When the kdump initramfs is loaded by the kexec_file_load
    syscall, the 1st kernel will iterate created key items, save the
    keys to kdump reserved memory.

 4. When the 1st kernel crashes and the kdump initramfs is booted, the
    kdump initramfs asks the kdump kernel to create a user key using the
    key stored in kdump reserved memory by writing yes to
    /sys/kernel/crash_dm_crypt_keys/restore. Then the LUKS encrypted
    device is unlocked with libcryptsetup's --volume-key-keyring API.

 5. The system gets rebooted to the 1st kernel after dumping vmcore to
    the LUKS encrypted device is finished

After libcryptsetup saving the LUKS volume keys to specified keyring,
whoever takes this should be responsible for the safety of these copies
of keys. The keys will be saved in the memory area exclusively reserved
for kdump where even the 1st kernel has no direct access. And further
more, two additional protections are added,
 - save the copy randomly in kdump reserved memory as suggested by Jan
 - clear the _PAGE_PRESENT flag of the page that stores the copy as
   suggested by Pingfan

This patch set only supports x86. There will be patches to support other
architectures once this patch set gets merged.

v6
 - Baoquan
   - support AMD SEV
   - drop unnecessary keys_header_size
   - improve commit message of [PATCH 4/7]
 
 - Greg
   - switch to configfs
   - move ifdef from .c to .h files
   - use tab instead of space for appended code comment
 
 - Process key description in a more flexible way to address problems
   found by Ondrej
 - improve cover letter
 - fix an compilation error as found by kernel test robot 

v5
 - Baoquan
   - limit the feature of placing kexec_buf randomly to kdump (CONFIG_CRASH_DUMP)
   - add documentation for added sysfs API 
   - allow to re-send init command to support the case of user switching to
     a different LUKS-encrypted target
   - make CONFIG_CRASH_DM_CRYPT depends on CONFIG_DM_CRYPT
   - check if the number of keys exceed KEY_NUM_MAX
   - rename (struct keys_header).key_count as (struct keys_header).total_keys
     to improve code readability
   - improve commit message
   - fix the failure of calling crash_exclude_mem_range (there is a split
     of mem_range)
   - use ret instead of r as return code
 
 - Greg
   - add documentation for added sysfs API 
   - avoid spamming kernel logs 
   - fix a buffer overflow issue
   - keep the state enums synced up with the string values
   - use sysfs_emit other than sprintf
   - explain KEY_NUM_MAX and KEY_SIZE_MAX
   - s/EXPORT_SYMBOL_GPL/EXPORT_SYMBOL/g
   - improve code readability
 
 - Rebase onto latest Linus tree


v4
- rebase onto latest Linus tree so Baoquan can apply the patches for
  code review
- fix kernel test robot warnings

v3
 - Support CPU/memory hot-plugging [Baoquan]
 - Don't save the keys temporarily to simplify the implementation [Baoquan]
 - Support multiple LUKS encrypted volumes
 - Read logon key instead of user key to improve security [Ondrej]
 - A kernel config option CRASH_DM_CRYPT for this feature (disabled by default)
 - Fix warnings found by kernel test robot
 - Rebase the code onto 6.9.0-rc5+

v2
 - work together with libscryptsetup's --link-vk-to-keyring/--volume-key-keyring APIs [Milan and Ondrej]
 - add the case where console virtual keyboard is untrusted for confidential VM
 - use dm_crypt_key instead of LUKS volume key [Milan and Eric]
 - fix some code format issues
 - don't move "struct kexec_segment" declaration
 - Rebase the code onto latest Linus tree (6.7.0)

v1
 - "Put the luks key handling related to crash_dump out into a separate
   file kernel/crash_dump_luks.c" [Baoquan]
 - Put the generic luks handling code before the x86 specific code to
   make it easier for other arches to follow suit [Baoquan]
 - Use phys_to_virt instead of "pfn -> page -> vaddr" [Dave Hansen]
 - Drop the RFC prefix [Dave Young]
 - Rebase the code onto latest Linus tree (6.4.0-rc4)

RFC v2
 - libcryptsetup interacts with the kernel via sysfs instead of "hacking"
   dm-crypt
   - to save a kdump copy of the LUKS volume key in 1st kernel
   - to add a logon key using the copy for libcryptsetup in kdump kernel [Milan]
   - to avoid the incorrect usage of LUKS master key in dm-crypt [Milan]
 - save the kdump copy of LUKS volume key randomly [Jan]
 - mark the kdump copy inaccessible [Pingfan]
 - Miscellaneous
   - explain when operations related to the LUKS volume key happen [Jan]
   - s/master key/volume key/g
   - use crash_ instead of kexec_ as function prefix
   - fix commit subject prefixes e.g. "x86, kdump" to x86/crash

Coiby Xu (7):
  kexec_file: allow to place kexec_buf randomly
  crash_dump: make dm crypt keys persist for the kdump kernel
  crash_dump: store dm crypt keys in kdump reserved memory
  crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging
  crash_dump: retrieve dm crypt keys in kdump kernel
  x86/crash: pass dm crypt keys to kdump kernel
  x86/crash: make the page that stores the dm crypt keys inaccessible

 arch/x86/kernel/crash.c            |  20 +-
 arch/x86/kernel/kexec-bzimage64.c  |   7 +
 arch/x86/kernel/machine_kexec_64.c |  22 ++
 include/linux/crash_core.h         |   7 +-
 include/linux/crash_dump.h         |   2 +
 include/linux/kexec.h              |  32 ++
 kernel/Kconfig.kexec               |   9 +
 kernel/Makefile                    |   1 +
 kernel/crash_dump_dm_crypt.c       | 459 +++++++++++++++++++++++++++++
 kernel/kexec_file.c                |   3 +
 10 files changed, 559 insertions(+), 3 deletions(-)
 create mode 100644 kernel/crash_dump_dm_crypt.c


base-commit: e42b1a9a2557aa94fee47f078633677198386a52
-- 
2.47.0

[PATCH v6 1/7] kexec_file: allow to place kexec_buf randomly

[Coiby Xu]

Currently, kexec_buf is placed in order which means for the same
machine, the info in the kexec_buf is always located at the same
position each time the machine is booted. This may cause a risk for
sensitive information like LUKS volume key. Now struct kexec_buf has a
new field random which indicates it's supposed to be placed in a random
position.

Suggested-by: Jan Pazdziora <jpazdziora@redhat.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/kexec.h | 28 ++++++++++++++++++++++++++++
 kernel/kexec_file.c   |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index f0e9f8eda7a3..0dc66ca2506a 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -25,6 +25,10 @@
 
 extern note_buf_t __percpu *crash_notes;
 
+#ifdef CONFIG_CRASH_DUMP
+#include <linux/prandom.h>
+#endif
+
 #ifdef CONFIG_KEXEC_CORE
 #include <linux/list.h>
 #include <linux/compat.h>
@@ -171,6 +175,7 @@ int kexec_image_post_load_cleanup_default(struct kimage *image);
  * @buf_min:	The buffer can't be placed below this address.
  * @buf_max:	The buffer can't be placed above this address.
  * @top_down:	Allocate from top of memory.
+ * @random:	Place the buffer at a random position.
  */
 struct kexec_buf {
 	struct kimage *image;
@@ -182,8 +187,31 @@ struct kexec_buf {
 	unsigned long buf_min;
 	unsigned long buf_max;
 	bool top_down;
+#ifdef CONFIG_CRASH_DUMP
+	bool random;
+#endif
 };
 
+
+#ifdef CONFIG_CRASH_DUMP
+static inline void kexec_random_start(unsigned long start, unsigned long end,
+				      struct kexec_buf *kbuf,
+				      unsigned long *temp_start)
+{
+	unsigned short i;
+
+	if (kbuf->random) {
+		get_random_bytes(&i, sizeof(unsigned short));
+		*temp_start = start + (end - start) / USHRT_MAX * i;
+	}
+}
+#else
+static inline void kexec_random_start(unsigned long start, unsigned long end,
+				      struct kexec_buf *kbuf,
+				      unsigned long *temp_start)
+{}
+#endif
+
 int kexec_load_purgatory(struct kimage *image, struct kexec_buf *kbuf);
 int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name,
 				   void *buf, unsigned int size,
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 3eedb8c226ad..06565d867b69 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -445,6 +445,7 @@ static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
 
 	temp_end = min(end, kbuf->buf_max);
 	temp_start = temp_end - kbuf->memsz + 1;
+	kexec_random_start(temp_start, temp_end, kbuf, &temp_start);
 
 	do {
 		/* align down start */
@@ -483,6 +484,8 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
 
 	temp_start = max(start, kbuf->buf_min);
 
+	kexec_random_start(temp_start, end, kbuf, &temp_start);
+
 	do {
 		temp_start = ALIGN(temp_start, kbuf->buf_align);
 		temp_end = temp_start + kbuf->memsz - 1;
-- 
2.47.0

Re: [PATCH v6 1/7] kexec_file: allow to place kexec_buf randomly

[Baoquan He]

On 10/29/24 at 01:52pm, Coiby Xu wrote:
> Currently, kexec_buf is placed in order which means for the same
> machine, the info in the kexec_buf is always located at the same
> position each time the machine is booted. This may cause a risk for
> sensitive information like LUKS volume key. Now struct kexec_buf has a
> new field random which indicates it's supposed to be placed in a random
> position.

This change the generic code, but you don't mention this only takes
effect in kdump case, won't impact kexec reboot case. I got this from
code, while this should be mentioned in log.

> 
> Suggested-by: Jan Pazdziora <jpazdziora@redhat.com>
> Signed-off-by: Coiby Xu <coxu@redhat.com>
> ---
>  include/linux/kexec.h | 28 ++++++++++++++++++++++++++++
>  kernel/kexec_file.c   |  3 +++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index f0e9f8eda7a3..0dc66ca2506a 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -25,6 +25,10 @@
>  
>  extern note_buf_t __percpu *crash_notes;
>  
> +#ifdef CONFIG_CRASH_DUMP
> +#include <linux/prandom.h>
> +#endif
> +
>  #ifdef CONFIG_KEXEC_CORE
>  #include <linux/list.h>
>  #include <linux/compat.h>
> @@ -171,6 +175,7 @@ int kexec_image_post_load_cleanup_default(struct kimage *image);
>   * @buf_min:	The buffer can't be placed below this address.
>   * @buf_max:	The buffer can't be placed above this address.
>   * @top_down:	Allocate from top of memory.
> + * @random:	Place the buffer at a random position.
>   */
>  struct kexec_buf {
>  	struct kimage *image;
> @@ -182,8 +187,31 @@ struct kexec_buf {
>  	unsigned long buf_min;
>  	unsigned long buf_max;
>  	bool top_down;
> +#ifdef CONFIG_CRASH_DUMP
> +	bool random;
> +#endif
>  };
>  
> +
> +#ifdef CONFIG_CRASH_DUMP
> +static inline void kexec_random_start(unsigned long start, unsigned long end,
                      ~~~~~~~~~~~~~~~~~~
This function name is very confusing. I thought it's a starting to
randomize at the first glance, then realized it's to randomize the
starting position of range.
> +				      struct kexec_buf *kbuf,
> +				      unsigned long *temp_start)
> +{
> +	unsigned short i;
> +
> +	if (kbuf->random) {
> +		get_random_bytes(&i, sizeof(unsigned short));
> +		*temp_start = start + (end - start) / USHRT_MAX * i;
> +	}
> +}
> +#else
> +static inline void kexec_random_start(unsigned long start, unsigned long end,
> +				      struct kexec_buf *kbuf,
> +				      unsigned long *temp_start)
> +{}
> +#endif
> +
>  int kexec_load_purgatory(struct kimage *image, struct kexec_buf *kbuf);
>  int kexec_purgatory_get_set_symbol(struct kimage *image, const char *name,
>  				   void *buf, unsigned int size,
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3eedb8c226ad..06565d867b69 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -445,6 +445,7 @@ static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
>  
>  	temp_end = min(end, kbuf->buf_max);
>  	temp_start = temp_end - kbuf->memsz + 1;
> +	kexec_random_start(temp_start, temp_end, kbuf, &temp_start);
>  
>  	do {
>  		/* align down start */
> @@ -483,6 +484,8 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
>  
>  	temp_start = max(start, kbuf->buf_min);
>  
> +	kexec_random_start(temp_start, end, kbuf, &temp_start);
> +
>  	do {
>  		temp_start = ALIGN(temp_start, kbuf->buf_align);
>  		temp_end = temp_start + kbuf->memsz - 1;
> -- 
> 2.47.0
> 

Re: [PATCH v6 1/7] kexec_file: allow to place kexec_buf randomly

[Coiby Xu]

On Fri, Nov 29, 2024 at 09:38:58AM +0800, Baoquan He wrote:
>On 10/29/24 at 01:52pm, Coiby Xu wrote:
>> Currently, kexec_buf is placed in order which means for the same
>> machine, the info in the kexec_buf is always located at the same
>> position each time the machine is booted. This may cause a risk for
>> sensitive information like LUKS volume key. Now struct kexec_buf has a
>> new field random which indicates it's supposed to be placed in a random
>> position.
>
>This change the generic code, but you don't mention this only takes
>effect in kdump case, won't impact kexec reboot case. I got this from
>code, while this should be mentioned in log.

Thanks for the reminder! I'll put a note in the commit msg.

>
[...]
>> +
>> +#ifdef CONFIG_CRASH_DUMP
>> +static inline void kexec_random_start(unsigned long start, unsigned long end,
>                      ~~~~~~~~~~~~~~~~~~
>This function name is very confusing. I thought it's a starting to
>randomize at the first glance, then realized it's to randomize the
>starting position of range.

Thanks for raising this concern! I'll rename it to
kexec_random_range_start.

-- 
Best regards,
Coiby

[PATCH v6 2/7] crash_dump: make dm crypt keys persist for the kdump kernel

[Coiby Xu]

A configfs /sys/kernel/config/crash_dm_crypt_keys is provided for user
space to make the dm crypt keys persist for the kdump kernel. Take the
case of dumping to a LUKS-encrypted target as an example, here is the
life cycle of the kdump copies of LUKS volume keys,

 1. After the 1st kernel loads the initramfs during boot, systemd uses
    an user-input passphrase to de-crypt the LUKS volume keys or simply
    TPM-sealed volume keys and then save the volume keys to specified
    keyring (using the --link-vk-to-keyring API) and the keys will expire
    within specified time.

 2. A user space tool (kdump initramfs loader like kdump-utils) create
    key items inside /sys/kernel/config/crash_dm_crypt_keys to inform
    the 1st kernel which keys are needed.

 3. When the kdump initramfs is loaded by the kexec_file_load
    syscall, the 1st kernel will iterate created key items, save the
    keys to kdump reserved memory.

 4. When the 1st kernel crashes and the kdump initramfs is booted, the
    kdump initramfs asks the kdump kernel to create a user key using the
    key stored in kdump reserved memory by writing yes to
    /sys/kernel/crash_dm_crypt_keys/restore. Then the LUKS encrypted
    device is unlocked with libcryptsetup's --volume-key-keyring API.

 5. The system gets rebooted to the 1st kernel after dumping vmcore to
    the LUKS encrypted device is finished

Eventually the keys have to stay in the kdump reserved memory for the
kdump kernel to unlock encrypted volumes. During this process, some
measures like letting the keys expire withing specified time are
desirable to reduce security risk.

This patch assumes,
1) there are 128 LUKS devices at maximum to be unlocked thus MAX_KEY_NUM=128.
2) a key description won't exceed 128 bytes thus KEY_DESC_MAX_LEN=128.

And here is a demo on how to interact with
/sys/kernel/config/crash_dm_crypt_keys,

    # Add key #1
    mkdir /sys/kernel/config/crash_dm_crypt_keys/7d26b7b4-e342-4d2d-b660-7426b0996720
    # Add key #1's description
    echo cryptsetup:7d26b7b4-e342-4d2d-b660-7426b0996720 > /sys/kernel/config/crash_dm_crypt_keys/description

    # how many keys do we have now?
    cat /sys/kernel/config/crash_dm_crypt_keys/count
    1

    # Add key#2 in the same way

    # how many keys do we have now?
    cat /sys/kernel/config/crash_dm_crypt_keys/count
    2

    # the tree structure of /crash_dm_crypt_keys configfs
    tree /sys/kernel/config/crash_dm_crypt_keys/
    /sys/kernel/config/crash_dm_crypt_keys/
    ├── 7d26b7b4-e342-4d2d-b660-7426b0996720
    │   └── description
    ├── count
    ├── fce2cd38-4d59-4317-8ce2-1fd24d52c46a
    │   └── description

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 kernel/Kconfig.kexec         |   9 ++
 kernel/Makefile              |   1 +
 kernel/crash_dump_dm_crypt.c | 154 +++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 kernel/crash_dump_dm_crypt.c

diff --git a/kernel/Kconfig.kexec b/kernel/Kconfig.kexec
index 6c34e63c88ff..1349878d6565 100644
--- a/kernel/Kconfig.kexec
+++ b/kernel/Kconfig.kexec
@@ -116,6 +116,15 @@ config CRASH_DUMP
 	  For s390, this option also enables zfcpdump.
 	  See also <file:Documentation/arch/s390/zfcpdump.rst>
 
+config CRASH_DM_CRYPT
+	bool "Support saving crash dump to dm-crypt encrypted volume"
+	depends on CRASH_DUMP
+	depends on DM_CRYPT
+	help
+	  With this option enabled, user space can intereact with
+	  /sys/kernel/config/crash_dm_crypt_keys to make the dm crypt keys
+	  persistent for the crash dump kernel.
+
 config CRASH_HOTPLUG
 	bool "Update the crash elfcorehdr on system configuration changes"
 	default y
diff --git a/kernel/Makefile b/kernel/Makefile
index 87866b037fbe..9d1cabf1ec46 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -72,6 +72,7 @@ obj-$(CONFIG_VMCORE_INFO) += vmcore_info.o elfcorehdr.o
 obj-$(CONFIG_CRASH_RESERVE) += crash_reserve.o
 obj-$(CONFIG_KEXEC_CORE) += kexec_core.o
 obj-$(CONFIG_CRASH_DUMP) += crash_core.o
+obj-$(CONFIG_CRASH_DM_CRYPT) += crash_dump_dm_crypt.o
 obj-$(CONFIG_KEXEC) += kexec.o
 obj-$(CONFIG_KEXEC_FILE) += kexec_file.o
 obj-$(CONFIG_KEXEC_ELF) += kexec_elf.o
diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
new file mode 100644
index 000000000000..62a3c47d8b3b
--- /dev/null
+++ b/kernel/crash_dump_dm_crypt.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <keys/user-type.h>
+#include <linux/crash_dump.h>
+#include <linux/configfs.h>
+#include <linux/module.h>
+
+#define KEY_NUM_MAX 128	/* maximum dm crypt keys */
+#define KEY_DESC_MAX_LEN 128	/* maximum dm crypt key description size */
+
+static unsigned int key_count;
+
+struct config_key {
+	struct config_item item;
+	const char *description;
+};
+
+static inline struct config_key *to_config_key(struct config_item *item)
+{
+	return container_of(item, struct config_key, item);
+}
+
+static ssize_t config_key_description_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%s\n", to_config_key(item)->description);
+}
+
+static ssize_t config_key_description_store(struct config_item *item,
+					    const char *page, size_t count)
+{
+	struct config_key *config_key = to_config_key(item);
+	size_t len;
+	int ret;
+
+	ret = -EINVAL;
+	len = strcspn(page, "\n");
+
+	if (len > KEY_DESC_MAX_LEN) {
+		pr_err("The key description shouldn't exceed %u characters", KEY_DESC_MAX_LEN);
+		return ret;
+	}
+
+	if (!len)
+		return ret;
+
+	kfree(config_key->description);
+	ret = -ENOMEM;
+	config_key->description = kmemdup_nul(page, len, GFP_KERNEL);
+	if (!config_key->description)
+		return ret;
+
+	return count;
+}
+
+CONFIGFS_ATTR(config_key_, description);
+
+static struct configfs_attribute *config_key_attrs[] = {
+	&config_key_attr_description,
+	NULL,
+};
+
+static void config_key_release(struct config_item *item)
+{
+	kfree(to_config_key(item));
+	key_count--;
+}
+
+static struct configfs_item_operations config_key_item_ops = {
+	.release = config_key_release,
+};
+
+static const struct config_item_type config_key_type = {
+	.ct_item_ops = &config_key_item_ops,
+	.ct_attrs = config_key_attrs,
+	.ct_owner = THIS_MODULE,
+};
+
+static struct config_item *config_keys_make_item(struct config_group *group,
+						 const char *name)
+{
+	struct config_key *config_key;
+
+	if (key_count > KEY_NUM_MAX) {
+		pr_err("Only %u keys at maximum to be created\n", KEY_NUM_MAX);
+		return ERR_PTR(-EINVAL);
+	}
+
+	config_key = kzalloc(sizeof(struct config_key), GFP_KERNEL);
+	if (!config_key)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&config_key->item, name, &config_key_type);
+
+	key_count++;
+
+	return &config_key->item;
+}
+
+static ssize_t config_keys_count_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n", key_count);
+}
+
+CONFIGFS_ATTR_RO(config_keys_, count);
+
+static struct configfs_attribute *config_keys_attrs[] = {
+	&config_keys_attr_count,
+	NULL,
+};
+
+/*
+ * Note that, since no extra work is required on ->drop_item(),
+ * no ->drop_item() is provided.
+ */
+static struct configfs_group_operations config_keys_group_ops = {
+	.make_item = config_keys_make_item,
+};
+
+static const struct config_item_type config_keys_type = {
+	.ct_group_ops = &config_keys_group_ops,
+	.ct_attrs = config_keys_attrs,
+	.ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem config_keys_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "crash_dm_crypt_keys",
+			.ci_type = &config_keys_type,
+		},
+	},
+};
+
+static int __init configfs_dmcrypt_keys_init(void)
+{
+	int ret;
+
+	config_group_init(&config_keys_subsys.su_group);
+	mutex_init(&config_keys_subsys.su_mutex);
+	ret = configfs_register_subsystem(&config_keys_subsys);
+	if (ret) {
+		pr_err("Error %d while registering subsystem %s\n", ret,
+		       config_keys_subsys.su_group.cg_item.ci_namebuf);
+		goto out_unregister;
+	}
+
+	return 0;
+
+out_unregister:
+	configfs_unregister_subsystem(&config_keys_subsys);
+
+	return ret;
+}
+
+module_init(configfs_dmcrypt_keys_init);
-- 
2.47.0

[PATCH v6 3/7] crash_dump: store dm crypt keys in kdump reserved memory

[Coiby Xu]

When the kdump kernel image and initrd are loaded, the dm crypts keys
will be read from keyring and then stored in kdump reserved memory.

Assume a key won't exceed 256 bytes thus MAX_KEY_SIZE=256 according to
"cryptsetup benchmark".

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_core.h   |   6 +-
 include/linux/kexec.h        |   4 ++
 kernel/crash_dump_dm_crypt.c | 129 +++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 44305336314e..2e6782239034 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -34,7 +34,11 @@ static inline void arch_kexec_protect_crashkres(void) { }
 static inline void arch_kexec_unprotect_crashkres(void) { }
 #endif
 
-
+#ifdef CONFIG_CRASH_DM_CRYPT
+int crash_load_dm_crypt_keys(struct kimage *image);
+#else
+static inline int crash_load_dm_crypt_keys(struct kimage *image) {return 0; }
+#endif
 
 #ifndef arch_crash_handle_hotplug_event
 static inline void arch_crash_handle_hotplug_event(struct kimage *image, void *arg) { }
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 0dc66ca2506a..5bda0978bab3 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -396,6 +396,10 @@ struct kimage {
 	void *elf_headers;
 	unsigned long elf_headers_sz;
 	unsigned long elf_load_addr;
+
+	/* dm crypt keys buffer */
+	unsigned long dm_crypt_keys_addr;
+	unsigned long dm_crypt_keys_sz;
 };
 
 /* kexec interface functions */
diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
index 62a3c47d8b3b..ec2ec2967242 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -1,14 +1,62 @@
 // SPDX-License-Identifier: GPL-2.0-only
+#include <linux/key.h>
+#include <linux/keyctl.h>
 #include <keys/user-type.h>
 #include <linux/crash_dump.h>
 #include <linux/configfs.h>
 #include <linux/module.h>
 
 #define KEY_NUM_MAX 128	/* maximum dm crypt keys */
+#define KEY_SIZE_MAX 256	/* maximum dm crypt key size */
 #define KEY_DESC_MAX_LEN 128	/* maximum dm crypt key description size */
 
 static unsigned int key_count;
 
+struct dm_crypt_key {
+	unsigned int key_size;
+	char key_desc[KEY_DESC_MAX_LEN];
+	u8 data[KEY_SIZE_MAX];
+};
+
+static struct keys_header {
+	unsigned int total_keys;
+	struct dm_crypt_key keys[] __counted_by(total_keys);
+} *keys_header;
+
+static size_t get_keys_header_size(size_t total_keys)
+{
+	return struct_size(keys_header, keys, total_keys);
+}
+
+static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
+{
+	const struct user_key_payload *ukp;
+	struct key *key;
+
+	kexec_dprintk("Requesting key %s", dm_key->key_desc);
+	key = request_key(&key_type_logon, dm_key->key_desc, NULL);
+
+	if (IS_ERR(key)) {
+		pr_warn("No such key %s\n", dm_key->key_desc);
+		return PTR_ERR(key);
+	}
+
+	ukp = user_key_payload_locked(key);
+	if (!ukp)
+		return -EKEYREVOKED;
+
+	if (ukp->datalen > KEY_SIZE_MAX) {
+		pr_err("Key size %u exceeds maximum (%u)\n", ukp->datalen, KEY_SIZE_MAX);
+		return -EINVAL;
+	}
+
+	memcpy(dm_key->data, ukp->data, ukp->datalen);
+	dm_key->key_size = ukp->datalen;
+	kexec_dprintk("Get dm crypt key (size=%u) %s: %8ph\n", dm_key->key_size,
+		      dm_key->key_desc, dm_key->data);
+	return 0;
+}
+
 struct config_key {
 	struct config_item item;
 	const char *description;
@@ -130,6 +178,87 @@ static struct configfs_subsystem config_keys_subsys = {
 	},
 };
 
+static int build_keys_header(void)
+{
+	struct config_item *item = NULL;
+	struct config_key *key;
+	int i, r;
+
+	if (keys_header != NULL)
+		kvfree(keys_header);
+
+	keys_header = kzalloc(get_keys_header_size(key_count), GFP_KERNEL);
+	if (!keys_header)
+		return -ENOMEM;
+
+	keys_header->total_keys = key_count;
+
+	i = 0;
+	list_for_each_entry(item, &config_keys_subsys.su_group.cg_children,
+			    ci_entry) {
+		if (item->ci_type != &config_key_type)
+			continue;
+
+		key = to_config_key(item);
+
+		strscpy(keys_header->keys[i].key_desc, key->description,
+			KEY_DESC_MAX_LEN);
+		r = read_key_from_user_keying(&keys_header->keys[i]);
+		if (r != 0) {
+			kexec_dprintk("Failed to read key %s\n",
+				      keys_header->keys[i].key_desc);
+			return r;
+		}
+		i++;
+		kexec_dprintk("Found key: %s\n", item->ci_name);
+	}
+
+	return 0;
+}
+
+int crash_load_dm_crypt_keys(struct kimage *image)
+{
+	struct kexec_buf kbuf = {
+		.image = image,
+		.buf_min = 0,
+		.buf_max = ULONG_MAX,
+		.top_down = false,
+		.random = true,
+	};
+	int r;
+
+
+	if (key_count <= 0) {
+		kexec_dprintk("No dm-crypt keys\n");
+		return -EINVAL;
+	}
+
+	image->dm_crypt_keys_addr = 0;
+	r = build_keys_header();
+	if (r)
+		return r;
+
+	kbuf.buffer = keys_header;
+	kbuf.bufsz = get_keys_header_size(key_count);
+
+	kbuf.memsz = kbuf.bufsz;
+	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
+	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
+	r = kexec_add_buffer(&kbuf);
+	if (r) {
+		kvfree((void *)kbuf.buffer);
+		return r;
+	}
+	image->dm_crypt_keys_addr = kbuf.mem;
+	image->dm_crypt_keys_sz = kbuf.bufsz;
+	kexec_dprintk(
+		"Loaded dm crypt keys to kexec_buffer bufsz=0x%lx memsz=0x%lx\n",
+		kbuf.bufsz, kbuf.bufsz);
+
+	return r;
+}
+
+
 static int __init configfs_dmcrypt_keys_init(void)
 {
 	int ret;
-- 
2.47.0

Re: [PATCH v6 3/7] crash_dump: store dm crypt keys in kdump reserved memory

[kernel test robot]

Hi Coiby,

kernel test robot noticed the following build errors:

[auto build test ERROR on e42b1a9a2557aa94fee47f078633677198386a52]

url:    https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20241029-135449
base:   e42b1a9a2557aa94fee47f078633677198386a52
patch link:    https://lore.kernel.org/r/20241029055223.210039-4-coxu%40redhat.com
patch subject: [PATCH v6 3/7] crash_dump: store dm crypt keys in kdump reserved memory
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20241029/202410292237.HA9vMqbC-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241029/202410292237.HA9vMqbC-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410292237.HA9vMqbC-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   kernel/crash_dump_dm_crypt.c: In function 'crash_load_dm_crypt_keys':
>> kernel/crash_dump_dm_crypt.c:221:16: error: variable 'kbuf' has initializer but incomplete type
     221 |         struct kexec_buf kbuf = {
         |                ^~~~~~~~~
>> kernel/crash_dump_dm_crypt.c:222:18: error: 'struct kexec_buf' has no member named 'image'
     222 |                 .image = image,
         |                  ^~~~~
>> kernel/crash_dump_dm_crypt.c:222:26: warning: excess elements in struct initializer
     222 |                 .image = image,
         |                          ^~~~~
   kernel/crash_dump_dm_crypt.c:222:26: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:223:18: error: 'struct kexec_buf' has no member named 'buf_min'
     223 |                 .buf_min = 0,
         |                  ^~~~~~~
   kernel/crash_dump_dm_crypt.c:223:28: warning: excess elements in struct initializer
     223 |                 .buf_min = 0,
         |                            ^
   kernel/crash_dump_dm_crypt.c:223:28: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:224:18: error: 'struct kexec_buf' has no member named 'buf_max'
     224 |                 .buf_max = ULONG_MAX,
         |                  ^~~~~~~
   In file included from include/linux/limits.h:7,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/loongarch/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/rcupdate.h:27,
                    from include/linux/rbtree.h:24,
                    from include/linux/key.h:15,
                    from kernel/crash_dump_dm_crypt.c:2:
>> include/vdso/limits.h:13:25: warning: excess elements in struct initializer
      13 | #define ULONG_MAX       (~0UL)
         |                         ^
   kernel/crash_dump_dm_crypt.c:224:28: note: in expansion of macro 'ULONG_MAX'
     224 |                 .buf_max = ULONG_MAX,
         |                            ^~~~~~~~~
   include/vdso/limits.h:13:25: note: (near initialization for 'kbuf')
      13 | #define ULONG_MAX       (~0UL)
         |                         ^
   kernel/crash_dump_dm_crypt.c:224:28: note: in expansion of macro 'ULONG_MAX'
     224 |                 .buf_max = ULONG_MAX,
         |                            ^~~~~~~~~
>> kernel/crash_dump_dm_crypt.c:225:18: error: 'struct kexec_buf' has no member named 'top_down'
     225 |                 .top_down = false,
         |                  ^~~~~~~~
   kernel/crash_dump_dm_crypt.c:225:29: warning: excess elements in struct initializer
     225 |                 .top_down = false,
         |                             ^~~~~
   kernel/crash_dump_dm_crypt.c:225:29: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:226:18: error: 'struct kexec_buf' has no member named 'random'
     226 |                 .random = true,
         |                  ^~~~~~
   kernel/crash_dump_dm_crypt.c:226:27: warning: excess elements in struct initializer
     226 |                 .random = true,
         |                           ^~~~
   kernel/crash_dump_dm_crypt.c:226:27: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:221:26: error: storage size of 'kbuf' isn't known
     221 |         struct kexec_buf kbuf = {
         |                          ^~~~
>> kernel/crash_dump_dm_crypt.c:246:20: error: 'KEXEC_BUF_MEM_UNKNOWN' undeclared (first use in this function)
     246 |         kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
         |                    ^~~~~~~~~~~~~~~~~~~~~
   kernel/crash_dump_dm_crypt.c:246:20: note: each undeclared identifier is reported only once for each function it appears in
>> kernel/crash_dump_dm_crypt.c:247:13: error: implicit declaration of function 'kexec_add_buffer' [-Wimplicit-function-declaration]
     247 |         r = kexec_add_buffer(&kbuf);
         |             ^~~~~~~~~~~~~~~~
>> kernel/crash_dump_dm_crypt.c:221:26: warning: unused variable 'kbuf' [-Wunused-variable]
     221 |         struct kexec_buf kbuf = {
         |                          ^~~~


vim +/kbuf +221 kernel/crash_dump_dm_crypt.c

   218	
   219	int crash_load_dm_crypt_keys(struct kimage *image)
   220	{
 > 221		struct kexec_buf kbuf = {
 > 222			.image = image,
 > 223			.buf_min = 0,
 > 224			.buf_max = ULONG_MAX,
 > 225			.top_down = false,
 > 226			.random = true,
   227		};
   228		int r;
   229	
   230	
   231		if (key_count <= 0) {
   232			kexec_dprintk("No dm-crypt keys\n");
   233			return -EINVAL;
   234		}
   235	
   236		image->dm_crypt_keys_addr = 0;
   237		r = build_keys_header();
   238		if (r)
   239			return r;
   240	
   241		kbuf.buffer = keys_header;
   242		kbuf.bufsz = get_keys_header_size(key_count);
   243	
   244		kbuf.memsz = kbuf.bufsz;
   245		kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
 > 246		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
 > 247		r = kexec_add_buffer(&kbuf);
   248		if (r) {
   249			kvfree((void *)kbuf.buffer);
   250			return r;
   251		}
   252		image->dm_crypt_keys_addr = kbuf.mem;
   253		image->dm_crypt_keys_sz = kbuf.bufsz;
   254		kexec_dprintk(
   255			"Loaded dm crypt keys to kexec_buffer bufsz=0x%lx memsz=0x%lx\n",
   256			kbuf.bufsz, kbuf.bufsz);
   257	
   258		return r;
   259	}
   260	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 3/7] crash_dump: store dm crypt keys in kdump reserved memory

[Coiby Xu]

On Tue, Oct 29, 2024 at 10:41:50PM +0800, kernel test robot wrote:
>Hi Coiby,
>
>kernel test robot noticed the following build errors:
>
>[auto build test ERROR on e42b1a9a2557aa94fee47f078633677198386a52]
>
>url:    https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20241029-135449
>base:   e42b1a9a2557aa94fee47f078633677198386a52
>patch link:    https://lore.kernel.org/r/20241029055223.210039-4-coxu%40redhat.com
>patch subject: [PATCH v6 3/7] crash_dump: store dm crypt keys in kdump reserved memory
>config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20241029/202410292237.HA9vMqbC-lkp@intel.com/config)
>compiler: loongarch64-linux-gcc (GCC) 14.1.0
>reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241029/202410292237.HA9vMqbC-lkp@intel.com/reproduce)
>
>If you fix the issue in a separate patch/commit (i.e. not just a new version of
>the same patch/commit), kindly add following tags
>| Reported-by: kernel test robot <lkp@intel.com>
>| Closes: https://lore.kernel.org/oe-kbuild-all/202410292237.HA9vMqbC-lkp@intel.com/
>
>All error/warnings (new ones prefixed by >>):
>
>   kernel/crash_dump_dm_crypt.c: In function 'crash_load_dm_crypt_keys':
>>> kernel/crash_dump_dm_crypt.c:221:16: error: variable 'kbuf' has initializer but incomplete type
>     221 |         struct kexec_buf kbuf = {
>         |                ^~~~~~~~~
>>> kernel/crash_dump_dm_crypt.c:222:18: error: 'struct kexec_buf' has no member named 'image'
>     222 |                 .image = image,
>         |                  ^~~~~
>>> kernel/crash_dump_dm_crypt.c:222:26: warning: excess elements in struct initializer
>     222 |                 .image = image,
>         |                          ^~~~~
>   kernel/crash_dump_dm_crypt.c:222:26: note: (near initialization for 'kbuf')
>>> kernel/crash_dump_dm_crypt.c:223:18: error: 'struct kexec_buf' has no member named 'buf_min'
>     223 |                 .buf_min = 0,
>         |                  ^~~~~~~
>   kernel/crash_dump_dm_crypt.c:223:28: warning: excess elements in struct initializer
>     223 |                 .buf_min = 0,
>         |                            ^
>   kernel/crash_dump_dm_crypt.c:223:28: note: (near initialization for 'kbuf')
>>> kernel/crash_dump_dm_crypt.c:224:18: error: 'struct kexec_buf' has no member named 'buf_max'
>     224 |                 .buf_max = ULONG_MAX,
>         |                  ^~~~~~~
>   In file included from include/linux/limits.h:7,
>                    from include/linux/thread_info.h:12,
>                    from include/asm-generic/preempt.h:5,
>                    from ./arch/loongarch/include/generated/asm/preempt.h:1,
>                    from include/linux/preempt.h:79,
>                    from include/linux/rcupdate.h:27,
>                    from include/linux/rbtree.h:24,
>                    from include/linux/key.h:15,
>                    from kernel/crash_dump_dm_crypt.c:2:
>>> include/vdso/limits.h:13:25: warning: excess elements in struct initializer
>      13 | #define ULONG_MAX       (~0UL)
>         |                         ^
>   kernel/crash_dump_dm_crypt.c:224:28: note: in expansion of macro 'ULONG_MAX'
>     224 |                 .buf_max = ULONG_MAX,
>         |                            ^~~~~~~~~
>   include/vdso/limits.h:13:25: note: (near initialization for 'kbuf')
>      13 | #define ULONG_MAX       (~0UL)
>         |                         ^
>   kernel/crash_dump_dm_crypt.c:224:28: note: in expansion of macro 'ULONG_MAX'
>     224 |                 .buf_max = ULONG_MAX,
>         |                            ^~~~~~~~~
>>> kernel/crash_dump_dm_crypt.c:225:18: error: 'struct kexec_buf' has no member named 'top_down'
>     225 |                 .top_down = false,
>         |                  ^~~~~~~~
>   kernel/crash_dump_dm_crypt.c:225:29: warning: excess elements in struct initializer
>     225 |                 .top_down = false,
>         |                             ^~~~~
>   kernel/crash_dump_dm_crypt.c:225:29: note: (near initialization for 'kbuf')
>>> kernel/crash_dump_dm_crypt.c:226:18: error: 'struct kexec_buf' has no member named 'random'
>     226 |                 .random = true,
>         |                  ^~~~~~
>   kernel/crash_dump_dm_crypt.c:226:27: warning: excess elements in struct initializer
>     226 |                 .random = true,
>         |                           ^~~~
>   kernel/crash_dump_dm_crypt.c:226:27: note: (near initialization for 'kbuf')
>>> kernel/crash_dump_dm_crypt.c:221:26: error: storage size of 'kbuf' isn't known
>     221 |         struct kexec_buf kbuf = {
>         |                          ^~~~
>>> kernel/crash_dump_dm_crypt.c:246:20: error: 'KEXEC_BUF_MEM_UNKNOWN' undeclared (first use in this function)
>     246 |         kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>         |                    ^~~~~~~~~~~~~~~~~~~~~
>   kernel/crash_dump_dm_crypt.c:246:20: note: each undeclared identifier is reported only once for each function it appears in
>>> kernel/crash_dump_dm_crypt.c:247:13: error: implicit declaration of function 'kexec_add_buffer' [-Wimplicit-function-declaration]
>     247 |         r = kexec_add_buffer(&kbuf);
>         |             ^~~~~~~~~~~~~~~~
>>> kernel/crash_dump_dm_crypt.c:221:26: warning: unused variable 'kbuf' [-Wunused-variable]
>     221 |         struct kexec_buf kbuf = {
>         |                          ^~~~
>
>
>vim +/kbuf +221 kernel/crash_dump_dm_crypt.c
>
>   218	
>   219	int crash_load_dm_crypt_keys(struct kimage *image)
>   220	{
> > 221		struct kexec_buf kbuf = {
> > 222			.image = image,
> > 223			.buf_min = 0,
> > 224			.buf_max = ULONG_MAX,
> > 225			.top_down = false,
> > 226			.random = true,
>   227		};
>   228		int r;
>   229	
>   230	
>   231		if (key_count <= 0) {
>   232			kexec_dprintk("No dm-crypt keys\n");
>   233			return -EINVAL;
>   234		}
>   235	
>   236		image->dm_crypt_keys_addr = 0;
>   237		r = build_keys_header();
>   238		if (r)
>   239			return r;
>   240	
>   241		kbuf.buffer = keys_header;
>   242		kbuf.bufsz = get_keys_header_size(key_count);
>   243	
>   244		kbuf.memsz = kbuf.bufsz;
>   245		kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> > 246		kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> > 247		r = kexec_add_buffer(&kbuf);
>   248		if (r) {
>   249			kvfree((void *)kbuf.buffer);
>   250			return r;
>   251		}
>   252		image->dm_crypt_keys_addr = kbuf.mem;
>   253		image->dm_crypt_keys_sz = kbuf.bufsz;
>   254		kexec_dprintk(
>   255			"Loaded dm crypt keys to kexec_buffer bufsz=0x%lx memsz=0x%lx\n",
>   256			kbuf.bufsz, kbuf.bufsz);
>   257	
>   258		return r;
>   259	}
>   260	
>
>-- 
>0-DAY CI Kernel Test Service
>https://github.com/intel/lkp-tests/wiki

Thanks for reporting this issue. I'll make CRASH_DM_CRYPT depend on
KEXEC_FILE to fix it in new version of patch set!

>_______________________________________________
>kexec mailing list
>kexec@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/kexec
>

-- 
Best regards,
Coiby

Re: [PATCH v6 3/7] crash_dump: store dm crypt keys in kdump reserved memory

[Baoquan He]

On 10/29/24 at 01:52pm, Coiby Xu wrote:
......
> +int crash_load_dm_crypt_keys(struct kimage *image)
> +{
> +	struct kexec_buf kbuf = {
> +		.image = image,
> +		.buf_min = 0,
> +		.buf_max = ULONG_MAX,
> +		.top_down = false,
> +		.random = true,
> +	};
> +	int r;
> +
> +
> +	if (key_count <= 0) {
> +		kexec_dprintk("No dm-crypt keys\n");
> +		return -EINVAL;
> +	}
> +
> +	image->dm_crypt_keys_addr = 0;
> +	r = build_keys_header();
> +	if (r)
> +		return r;
> +
> +	kbuf.buffer = keys_header;
> +	kbuf.bufsz = get_keys_header_size(key_count);
> +
> +	kbuf.memsz = kbuf.bufsz;
> +	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> +	r = kexec_add_buffer(&kbuf);
> +	if (r) {
> +		kvfree((void *)kbuf.buffer);
> +		return r;
> +	}
> +	image->dm_crypt_keys_addr = kbuf.mem;
> +	image->dm_crypt_keys_sz = kbuf.bufsz;

Wondering why not assigning kbuf.memsz, but bufsz.

> +	kexec_dprintk(
> +		"Loaded dm crypt keys to kexec_buffer bufsz=0x%lx memsz=0x%lx\n",
> +		kbuf.bufsz, kbuf.bufsz);
> +
> +	return r;
> +}
> +
> +
>  static int __init configfs_dmcrypt_keys_init(void)
>  {
>  	int ret;
> -- 
> 2.47.0
> 

Re: [PATCH v6 3/7] crash_dump: store dm crypt keys in kdump reserved memory

[Coiby Xu]

On Wed, Dec 11, 2024 at 08:58:27PM +0800, Baoquan He wrote:
>On 10/29/24 at 01:52pm, Coiby Xu wrote:
>......
>> +int crash_load_dm_crypt_keys(struct kimage *image)
>> +{
>> +	struct kexec_buf kbuf = {
>> +		.image = image,
>> +		.buf_min = 0,
>> +		.buf_max = ULONG_MAX,
>> +		.top_down = false,
>> +		.random = true,
>> +	};
>> +	int r;
>> +
>> +
>> +	if (key_count <= 0) {
>> +		kexec_dprintk("No dm-crypt keys\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	image->dm_crypt_keys_addr = 0;
>> +	r = build_keys_header();
>> +	if (r)
>> +		return r;
>> +
>> +	kbuf.buffer = keys_header;
>> +	kbuf.bufsz = get_keys_header_size(key_count);
>> +
>> +	kbuf.memsz = kbuf.bufsz;
>> +	kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>> +	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> +	r = kexec_add_buffer(&kbuf);
>> +	if (r) {
>> +		kvfree((void *)kbuf.buffer);
>> +		return r;
>> +	}
>> +	image->dm_crypt_keys_addr = kbuf.mem;
>> +	image->dm_crypt_keys_sz = kbuf.bufsz;
>
>Wondering why not assigning kbuf.memsz, but bufsz.

Thanks for the question! I think there is no need to use kbuf.memsz
after reading the code on setting image->elf_headers_sz. Currently all
arches except for x86 CONFIG_CRASH_HOTPLUG assigns kbuf.bufsz to
image->elf_headers_sz. For x86 CONFIG_CRASH_HOTPLUG, if I understand it
correctly, it assigns kbuf.memsz to kbuf.memsz because elfcorehdr
changes when there is memory/CPU hot-plugging. For dm crypt keys, there
is no so such concern, so assigning kbuf.bufsz shall be OK.

-- 
Best regards,
Coiby

[PATCH v6 4/7] crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging

[Coiby Xu]

When there are CPU and memory hot un/plugs, the dm crypt keys may need
to be reloaded again depending on the solution for crash hotplug
support. Currently, there are two solutions. One is to utilizes udev to
instruct user space to reload the kdump kernel image and initrd,
elfcorehdr and etc again. The other is to only update the elfcorehdr
segment introduced in commit 247262756121 ("crash:
add generic infrastructure for crash hotplug support").

For the 1st solution, the dm crypt keys need to be reloaded again. The
user space can write true to
/sys/kernel/config/crash_dm_crypt_key/reuse so the stored keys can be
re-used.

For the 2nd solution, the dm crypt keys don't need to be reloaded.
Currently, only x86 supports the 2nd solution. If the 2nd solution
gets extended to all arches, this patch can be dropped.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 kernel/crash_dump_dm_crypt.c | 52 +++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
index ec2ec2967242..51431f93fc1e 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -28,6 +28,20 @@ static size_t get_keys_header_size(size_t total_keys)
 	return struct_size(keys_header, keys, total_keys);
 }
 
+static void get_keys_from_kdump_reserved_memory(void)
+{
+	struct keys_header *keys_header_loaded;
+
+	arch_kexec_unprotect_crashkres();
+
+	keys_header_loaded = kmap_local_page(pfn_to_page(
+		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
+
+	memcpy(keys_header, keys_header_loaded, get_keys_header_size(key_count));
+	kunmap_local(keys_header_loaded);
+	arch_kexec_protect_crashkres();
+}
+
 static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
 {
 	const struct user_key_payload *ukp;
@@ -150,8 +164,36 @@ static ssize_t config_keys_count_show(struct config_item *item, char *page)
 
 CONFIGFS_ATTR_RO(config_keys_, count);
 
+static bool reuse;
+
+static ssize_t config_keys_reuse_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n", reuse);
+}
+
+static ssize_t config_keys_reuse_store(struct config_item *item,
+					   const char *page, size_t count)
+{
+	if (!kexec_crash_image || !kexec_crash_image->dm_crypt_keys_addr) {
+		kexec_dprintk(
+			"dm-crypt keys haven't be saved to crash-reserved memory\n");
+		return -EINVAL;
+	}
+
+	if (kstrtobool(page, &reuse))
+		return -EINVAL;
+
+	if (reuse)
+		get_keys_from_kdump_reserved_memory();
+
+	return count;
+}
+
+CONFIGFS_ATTR(config_keys_, reuse);
+
 static struct configfs_attribute *config_keys_attrs[] = {
 	&config_keys_attr_count,
+	&config_keys_attr_reuse,
 	NULL,
 };
 
@@ -233,10 +275,12 @@ int crash_load_dm_crypt_keys(struct kimage *image)
 		return -EINVAL;
 	}
 
-	image->dm_crypt_keys_addr = 0;
-	r = build_keys_header();
-	if (r)
-		return r;
+	if (!reuse) {
+		image->dm_crypt_keys_addr = 0;
+		r = build_keys_header();
+		if (r)
+			return r;
+	}
 
 	kbuf.buffer = keys_header;
 	kbuf.bufsz = get_keys_header_size(key_count);
-- 
2.47.0

Re: [PATCH v6 4/7] crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging

[Baoquan He]

On 10/29/24 at 01:52pm, Coiby Xu wrote:
> When there are CPU and memory hot un/plugs, the dm crypt keys may need
> to be reloaded again depending on the solution for crash hotplug
> support. Currently, there are two solutions. One is to utilizes udev to
> instruct user space to reload the kdump kernel image and initrd,
> elfcorehdr and etc again. The other is to only update the elfcorehdr
> segment introduced in commit 247262756121 ("crash:
> add generic infrastructure for crash hotplug support").
> 
> For the 1st solution, the dm crypt keys need to be reloaded again. The
> user space can write true to
> /sys/kernel/config/crash_dm_crypt_key/reuse so the stored keys can be
> re-used.
> 
> For the 2nd solution, the dm crypt keys don't need to be reloaded.
> Currently, only x86 supports the 2nd solution. If the 2nd solution
> gets extended to all arches, this patch can be dropped.
> 
> Signed-off-by: Coiby Xu <coxu@redhat.com>
> ---
>  kernel/crash_dump_dm_crypt.c | 52 +++++++++++++++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index ec2ec2967242..51431f93fc1e 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -28,6 +28,20 @@ static size_t get_keys_header_size(size_t total_keys)
>  	return struct_size(keys_header, keys, total_keys);
>  }
>  
> +static void get_keys_from_kdump_reserved_memory(void)
> +{
> +	struct keys_header *keys_header_loaded;
> +
> +	arch_kexec_unprotect_crashkres();
> +
> +	keys_header_loaded = kmap_local_page(pfn_to_page(
> +		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
> +
> +	memcpy(keys_header, keys_header_loaded, get_keys_header_size(key_count));
> +	kunmap_local(keys_header_loaded);
> +	arch_kexec_protect_crashkres();
> +}
> +
>  static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
>  {
>  	const struct user_key_payload *ukp;
> @@ -150,8 +164,36 @@ static ssize_t config_keys_count_show(struct config_item *item, char *page)
>  
>  CONFIGFS_ATTR_RO(config_keys_, count);
>  
> +static bool reuse;

Give it a meaningful name since it's a global variable, e.g
is_dm_key_reused?

> +
> +static ssize_t config_keys_reuse_show(struct config_item *item, char *page)
> +{
> +	return sprintf(page, "%d\n", reuse);
> +}
> +
> +static ssize_t config_keys_reuse_store(struct config_item *item,
> +					   const char *page, size_t count)
> +{
> +	if (!kexec_crash_image || !kexec_crash_image->dm_crypt_keys_addr) {
> +		kexec_dprintk(
> +			"dm-crypt keys haven't be saved to crash-reserved memory\n");
> +		return -EINVAL;
> +	}
> +
> +	if (kstrtobool(page, &reuse))
> +		return -EINVAL;
> +
> +	if (reuse)
> +		get_keys_from_kdump_reserved_memory();
> +
> +	return count;
> +}
> +
> +CONFIGFS_ATTR(config_keys_, reuse);
> +
>  static struct configfs_attribute *config_keys_attrs[] = {
>  	&config_keys_attr_count,
> +	&config_keys_attr_reuse,
>  	NULL,
>  };
>  

Re: [PATCH v6 4/7] crash_dump: reuse saved dm crypt keys for CPU/memory hot-plugging

[Coiby Xu]

On Wed, Dec 11, 2024 at 09:08:43PM +0800, Baoquan He wrote:
>On 10/29/24 at 01:52pm, Coiby Xu wrote:
>> When there are CPU and memory hot un/plugs, the dm crypt keys may need
>> to be reloaded again depending on the solution for crash hotplug
>> support. Currently, there are two solutions. One is to utilizes udev to
>> instruct user space to reload the kdump kernel image and initrd,
>> elfcorehdr and etc again. The other is to only update the elfcorehdr
>> segment introduced in commit 247262756121 ("crash:
>> add generic infrastructure for crash hotplug support").
>>
>> For the 1st solution, the dm crypt keys need to be reloaded again. The
>> user space can write true to
>> /sys/kernel/config/crash_dm_crypt_key/reuse so the stored keys can be
>> re-used.
>>
>> For the 2nd solution, the dm crypt keys don't need to be reloaded.
>> Currently, only x86 supports the 2nd solution. If the 2nd solution
>> gets extended to all arches, this patch can be dropped.
>>
>> Signed-off-by: Coiby Xu <coxu@redhat.com>
>> ---
>>  kernel/crash_dump_dm_crypt.c | 52 +++++++++++++++++++++++++++++++++---
>>  1 file changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>> index ec2ec2967242..51431f93fc1e 100644
>> --- a/kernel/crash_dump_dm_crypt.c
>> +++ b/kernel/crash_dump_dm_crypt.c
>> @@ -28,6 +28,20 @@ static size_t get_keys_header_size(size_t total_keys)
>>  	return struct_size(keys_header, keys, total_keys);
>>  }
>>
>> +static void get_keys_from_kdump_reserved_memory(void)
>> +{
>> +	struct keys_header *keys_header_loaded;
>> +
>> +	arch_kexec_unprotect_crashkres();
>> +
>> +	keys_header_loaded = kmap_local_page(pfn_to_page(
>> +		kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
>> +
>> +	memcpy(keys_header, keys_header_loaded, get_keys_header_size(key_count));
>> +	kunmap_local(keys_header_loaded);
>> +	arch_kexec_protect_crashkres();
>> +}
>> +
>>  static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
>>  {
>>  	const struct user_key_payload *ukp;
>> @@ -150,8 +164,36 @@ static ssize_t config_keys_count_show(struct config_item *item, char *page)
>>
>>  CONFIGFS_ATTR_RO(config_keys_, count);
>>
>> +static bool reuse;
>
>Give it a meaningful name since it's a global variable, e.g
>is_dm_key_reused?

Sure, I'll use is_dm_key_reused in next version! Thanks for the
suggestion to improve code readability!

-- 
Best regards,
Coiby

[PATCH v6 5/7] crash_dump: retrieve dm crypt keys in kdump kernel

[Coiby Xu]

Crash kernel will retrieve the dm crypt keys based on the dmcryptkeys
command line parameter. When user space writes the key description to
/sys/kernel/config/crash_dm_crypt_key/restore, the crash kernel will
save the encryption keys to the user keyring. Then user space e.g.
cryptsetup's --volume-key-keyring API can use it to unlock the encrypted
device.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 include/linux/crash_core.h   |   1 +
 include/linux/crash_dump.h   |   2 +
 kernel/crash_dump_dm_crypt.c | 134 ++++++++++++++++++++++++++++++++++-
 3 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 2e6782239034..d35726d6a415 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -36,6 +36,7 @@ static inline void arch_kexec_unprotect_crashkres(void) { }
 
 #ifdef CONFIG_CRASH_DM_CRYPT
 int crash_load_dm_crypt_keys(struct kimage *image);
+ssize_t dm_crypt_keys_read(char *buf, size_t count, u64 *ppos);
 #else
 static inline int crash_load_dm_crypt_keys(struct kimage *image) {return 0; }
 #endif
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index acc55626afdc..dfd8e4fe6129 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -15,6 +15,8 @@
 extern unsigned long long elfcorehdr_addr;
 extern unsigned long long elfcorehdr_size;
 
+extern unsigned long long dm_crypt_keys_addr;
+
 #ifdef CONFIG_CRASH_DUMP
 extern int elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size);
 extern void elfcorehdr_free(unsigned long long addr);
diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
index 51431f93fc1e..f72a88b7d106 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -3,6 +3,7 @@
 #include <linux/keyctl.h>
 #include <keys/user-type.h>
 #include <linux/crash_dump.h>
+#include <linux/cc_platform.h>
 #include <linux/configfs.h>
 #include <linux/module.h>
 
@@ -28,6 +29,61 @@ static size_t get_keys_header_size(size_t total_keys)
 	return struct_size(keys_header, keys, total_keys);
 }
 
+unsigned long long dm_crypt_keys_addr;
+EXPORT_SYMBOL_GPL(dm_crypt_keys_addr);
+
+static int __init setup_dmcryptkeys(char *arg)
+{
+	char *end;
+
+	if (!arg)
+		return -EINVAL;
+	dm_crypt_keys_addr = memparse(arg, &end);
+	if (end > arg)
+		return 0;
+
+	dm_crypt_keys_addr = 0;
+	return -EINVAL;
+}
+
+early_param("dmcryptkeys", setup_dmcryptkeys);
+
+/*
+ * Architectures may override this function to read dm crypt keys
+ */
+ssize_t __weak dm_crypt_keys_read(char *buf, size_t count, u64 *ppos)
+{
+	struct kvec kvec = { .iov_base = buf, .iov_len = count };
+	struct iov_iter iter;
+
+	iov_iter_kvec(&iter, READ, &kvec, 1, count);
+	return read_from_oldmem(&iter, count, ppos, cc_platform_has(CC_ATTR_MEM_ENCRYPT));
+}
+
+static int add_key_to_keyring(struct dm_crypt_key *dm_key,
+			      key_ref_t keyring_ref)
+{
+	key_ref_t key_ref;
+	int r;
+
+	/* create or update the requested key and add it to the target keyring */
+	key_ref = key_create_or_update(keyring_ref, "user", dm_key->key_desc,
+				       dm_key->data, dm_key->key_size,
+				       KEY_USR_ALL, KEY_ALLOC_IN_QUOTA);
+
+	if (!IS_ERR(key_ref)) {
+		r = key_ref_to_ptr(key_ref)->serial;
+		key_ref_put(key_ref);
+		kexec_dprintk("Success adding key %s", dm_key->key_desc);
+	} else {
+		r = PTR_ERR(key_ref);
+		kexec_dprintk("Error when adding key");
+	}
+
+	key_ref_put(keyring_ref);
+	return r;
+}
+
 static void get_keys_from_kdump_reserved_memory(void)
 {
 	struct keys_header *keys_header_loaded;
@@ -42,6 +98,47 @@ static void get_keys_from_kdump_reserved_memory(void)
 	arch_kexec_protect_crashkres();
 }
 
+static int restore_dm_crypt_keys_to_thread_keyring(void)
+{
+	struct dm_crypt_key *key;
+	size_t keys_header_size;
+	key_ref_t keyring_ref;
+	u64 addr;
+
+	/* find the target keyring (which must be writable) */
+	keyring_ref =
+		lookup_user_key(KEY_SPEC_USER_KEYRING, 0x01, KEY_NEED_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		kexec_dprintk("Failed to get the user keyring\n");
+		return PTR_ERR(keyring_ref);
+	}
+
+	addr = dm_crypt_keys_addr;
+	dm_crypt_keys_read((char *)&key_count, sizeof(key_count), &addr);
+	if (key_count < 0 || key_count > KEY_NUM_MAX) {
+		kexec_dprintk("Failed to read the number of dm-crypt keys\n");
+		return -1;
+	}
+
+	kexec_dprintk("There are %u keys\n", key_count);
+	addr = dm_crypt_keys_addr;
+
+	keys_header_size = get_keys_header_size(key_count);
+	keys_header = kzalloc(keys_header_size, GFP_KERNEL);
+	if (!keys_header)
+		return -ENOMEM;
+
+	dm_crypt_keys_read((char *)keys_header, keys_header_size, &addr);
+
+	for (int i = 0; i < keys_header->total_keys; i++) {
+		key = &keys_header->keys[i];
+		kexec_dprintk("Get key (size=%u)\n", key->key_size);
+		add_key_to_keyring(key, keyring_ref);
+	}
+
+	return 0;
+}
+
 static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
 {
 	const struct user_key_payload *ukp;
@@ -211,6 +308,37 @@ static const struct config_item_type config_keys_type = {
 	.ct_owner = THIS_MODULE,
 };
 
+static bool restore;
+
+static ssize_t config_keys_restore_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n", restore);
+}
+
+static ssize_t config_keys_restore_store(struct config_item *item,
+					  const char *page, size_t count)
+{
+	if (!restore)
+		restore_dm_crypt_keys_to_thread_keyring();
+
+	if (kstrtobool(page, &restore))
+		return -EINVAL;
+
+	return count;
+}
+
+CONFIGFS_ATTR(config_keys_, restore);
+
+static struct configfs_attribute *kdump_config_keys_attrs[] = {
+	&config_keys_attr_restore,
+	NULL,
+};
+
+static const struct config_item_type kdump_config_keys_type = {
+	.ct_attrs = kdump_config_keys_attrs,
+	.ct_owner = THIS_MODULE,
+};
+
 static struct configfs_subsystem config_keys_subsys = {
 	.su_group = {
 		.cg_item = {
@@ -302,11 +430,15 @@ int crash_load_dm_crypt_keys(struct kimage *image)
 	return r;
 }
 
-
 static int __init configfs_dmcrypt_keys_init(void)
 {
 	int ret;
 
+	if (is_kdump_kernel()) {
+		config_keys_subsys.su_group.cg_item.ci_type =
+			&kdump_config_keys_type;
+	}
+
 	config_group_init(&config_keys_subsys.su_group);
 	mutex_init(&config_keys_subsys.su_mutex);
 	ret = configfs_register_subsystem(&config_keys_subsys);
-- 
2.47.0

[PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

[Coiby Xu]

1st kernel will build up the kernel command parameter dmcryptkeys as
similar to elfcorehdr to pass the memory address of the stored info of
dm crypt key to kdump kernel.

Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 arch/x86/kernel/crash.c           | 20 ++++++++++++++++++--
 arch/x86/kernel/kexec-bzimage64.c |  7 +++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 340af8155658..99d50c31db02 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -278,6 +278,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 				 unsigned long long mend)
 {
 	unsigned long start, end;
+	int ret;
 
 	cmem->ranges[0].start = mstart;
 	cmem->ranges[0].end = mend;
@@ -286,22 +287,37 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
 	/* Exclude elf header region */
 	start = image->elf_load_addr;
 	end = start + image->elf_headers_sz - 1;
-	return crash_exclude_mem_range(cmem, start, end);
+	ret = crash_exclude_mem_range(cmem, start, end);
+
+	if (ret)
+		return ret;
+
+	/* Exclude dm crypt keys region */
+	if (image->dm_crypt_keys_addr) {
+		start = image->dm_crypt_keys_addr;
+		end = start + image->dm_crypt_keys_sz - 1;
+		return crash_exclude_mem_range(cmem, start, end);
+	}
+
+	return ret;
 }
 
 /* Prepare memory map for crash dump kernel */
 int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
 {
+	unsigned int max_nr_ranges = 3;
 	int i, ret = 0;
 	unsigned long flags;
 	struct e820_entry ei;
 	struct crash_memmap_data cmd;
 	struct crash_mem *cmem;
 
-	cmem = vzalloc(struct_size(cmem, ranges, 1));
+	cmem = vzalloc(struct_size(cmem, ranges, max_nr_ranges));
 	if (!cmem)
 		return -ENOMEM;
 
+	cmem->max_nr_ranges = max_nr_ranges;
+
 	memset(&cmd, 0, sizeof(struct crash_memmap_data));
 	cmd.params = params;
 
diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 68530fad05f7..9c94428927bd 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
 	if (image->type == KEXEC_TYPE_CRASH) {
 		len = sprintf(cmdline_ptr,
 			"elfcorehdr=0x%lx ", image->elf_load_addr);
+
+		if (image->dm_crypt_keys_addr != 0)
+			len += sprintf(cmdline_ptr + len,
+					"dmcryptkeys=0x%lx ", image->dm_crypt_keys_addr);
 	}
 	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
 	cmdline_len += len;
@@ -441,6 +445,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
 		ret = crash_load_segments(image);
 		if (ret)
 			return ERR_PTR(ret);
+		ret = crash_load_dm_crypt_keys(image);
+		if (ret)
+			pr_debug("Either no dm crypt key or error to retrieve the dm crypt key\n");
 	}
 #endif
 
-- 
2.47.0

Re: [PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

[Baoquan He]

On 10/29/24 at 01:52pm, Coiby Xu wrote:
> 1st kernel will build up the kernel command parameter dmcryptkeys as
> similar to elfcorehdr to pass the memory address of the stored info of
> dm crypt key to kdump kernel.
> 
> Signed-off-by: Coiby Xu <coxu@redhat.com>
> ---
>  arch/x86/kernel/crash.c           | 20 ++++++++++++++++++--
>  arch/x86/kernel/kexec-bzimage64.c |  7 +++++++
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 340af8155658..99d50c31db02 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -278,6 +278,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
>  				 unsigned long long mend)
>  {
>  	unsigned long start, end;
> +	int ret;
>  
>  	cmem->ranges[0].start = mstart;
>  	cmem->ranges[0].end = mend;
> @@ -286,22 +287,37 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
>  	/* Exclude elf header region */
>  	start = image->elf_load_addr;
>  	end = start + image->elf_headers_sz - 1;
> -	return crash_exclude_mem_range(cmem, start, end);
> +	ret = crash_exclude_mem_range(cmem, start, end);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Exclude dm crypt keys region */
> +	if (image->dm_crypt_keys_addr) {
> +		start = image->dm_crypt_keys_addr;
> +		end = start + image->dm_crypt_keys_sz - 1;
> +		return crash_exclude_mem_range(cmem, start, end);
> +	}
> +
> +	return ret;
>  }
>  
>  /* Prepare memory map for crash dump kernel */
>  int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>  {
> +	unsigned int max_nr_ranges = 3;

Define a macro and add code comment to explain why this value is taken?

>  	int i, ret = 0;
>  	unsigned long flags;
>  	struct e820_entry ei;
>  	struct crash_memmap_data cmd;
>  	struct crash_mem *cmem;
>  
> -	cmem = vzalloc(struct_size(cmem, ranges, 1));
> +	cmem = vzalloc(struct_size(cmem, ranges, max_nr_ranges));
>  	if (!cmem)
>  		return -ENOMEM;
>  
> +	cmem->max_nr_ranges = max_nr_ranges;
> +
>  	memset(&cmd, 0, sizeof(struct crash_memmap_data));
>  	cmd.params = params;
>  
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 68530fad05f7..9c94428927bd 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
>  	if (image->type == KEXEC_TYPE_CRASH) {
>  		len = sprintf(cmdline_ptr,
>  			"elfcorehdr=0x%lx ", image->elf_load_addr);
> +
> +		if (image->dm_crypt_keys_addr != 0)
> +			len += sprintf(cmdline_ptr + len,
> +					"dmcryptkeys=0x%lx ", image->dm_crypt_keys_addr);
>  	}
>  	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
>  	cmdline_len += len;
> @@ -441,6 +445,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>  		ret = crash_load_segments(image);
>  		if (ret)
>  			return ERR_PTR(ret);
> +		ret = crash_load_dm_crypt_keys(image);
> +		if (ret)
> +			pr_debug("Either no dm crypt key or error to retrieve the dm crypt key\n");

If it's error, do we need change ti to pr_debug?
>  	}
>  #endif
>  
> -- 
> 2.47.0
> 

Re: [PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

[Coiby Xu]

On Wed, Dec 11, 2024 at 08:55:52PM +0800, Baoquan He wrote:
>On 10/29/24 at 01:52pm, Coiby Xu wrote:
>> 1st kernel will build up the kernel command parameter dmcryptkeys as
>> similar to elfcorehdr to pass the memory address of the stored info of
>> dm crypt key to kdump kernel.
>>
>> Signed-off-by: Coiby Xu <coxu@redhat.com>
>> ---
>>  arch/x86/kernel/crash.c           | 20 ++++++++++++++++++--
>>  arch/x86/kernel/kexec-bzimage64.c |  7 +++++++
>>  2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
>> index 340af8155658..99d50c31db02 100644
>> --- a/arch/x86/kernel/crash.c
>> +++ b/arch/x86/kernel/crash.c
>> @@ -278,6 +278,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
>>  				 unsigned long long mend)
>>  {
>>  	unsigned long start, end;
>> +	int ret;
>>
>>  	cmem->ranges[0].start = mstart;
>>  	cmem->ranges[0].end = mend;
>> @@ -286,22 +287,37 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
>>  	/* Exclude elf header region */
>>  	start = image->elf_load_addr;
>>  	end = start + image->elf_headers_sz - 1;
>> -	return crash_exclude_mem_range(cmem, start, end);
>> +	ret = crash_exclude_mem_range(cmem, start, end);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Exclude dm crypt keys region */
>> +	if (image->dm_crypt_keys_addr) {
>> +		start = image->dm_crypt_keys_addr;
>> +		end = start + image->dm_crypt_keys_sz - 1;
>> +		return crash_exclude_mem_range(cmem, start, end);
>> +	}
>> +
>> +	return ret;
>>  }
>>
>>  /* Prepare memory map for crash dump kernel */
>>  int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
>>  {
>> +	unsigned int max_nr_ranges = 3;
>
>Define a macro and add code comment to explain why this value is taken?

Good suggestion! While drafting the reason for max_nr_ranges=3, I
realize max_nr_ranges=2 is sufficient. I also notice the fill_up_crash_elf_data
function in the same source file has similar comment. So next version
I'll add the following change,


      int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
      {
     +       unsigned int nr_ranges = 0;
             int i, ret = 0;
             unsigned long flags;
             struct e820_entry ei;
             struct crash_memmap_data cmd;
             struct crash_mem *cmem;
      
     -       cmem = vzalloc(struct_size(cmem, ranges, 1));
     +       /*
     +        * Using random kexec_buf for passing dm crypt keys may cause a range
     +        * split. So use two slots here.
     +        */
     +       nr_ranges = 2;
     +       cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
             if (!cmem)
                     return -ENOMEM;
      
     +       cmem->max_nr_ranges = nr_ranges;
     +       cmem->nr_ranges = 0;

>
>>  	int i, ret = 0;
>>  	unsigned long flags;
>>  	struct e820_entry ei;
>>  	struct crash_memmap_data cmd;
>>  	struct crash_mem *cmem;
>>
>> -	cmem = vzalloc(struct_size(cmem, ranges, 1));
>> +	cmem = vzalloc(struct_size(cmem, ranges, max_nr_ranges));
>>  	if (!cmem)
>>  		return -ENOMEM;
>>
>> +	cmem->max_nr_ranges = max_nr_ranges;
>> +
>>  	memset(&cmd, 0, sizeof(struct crash_memmap_data));
>>  	cmd.params = params;
>>
>> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
>> index 68530fad05f7..9c94428927bd 100644
>> --- a/arch/x86/kernel/kexec-bzimage64.c
>> +++ b/arch/x86/kernel/kexec-bzimage64.c
>> @@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
>>  	if (image->type == KEXEC_TYPE_CRASH) {
>>  		len = sprintf(cmdline_ptr,
>>  			"elfcorehdr=0x%lx ", image->elf_load_addr);
>> +
>> +		if (image->dm_crypt_keys_addr != 0)
>> +			len += sprintf(cmdline_ptr + len,
>> +					"dmcryptkeys=0x%lx ", image->dm_crypt_keys_addr);
>>  	}
>>  	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
>>  	cmdline_len += len;
>> @@ -441,6 +445,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>>  		ret = crash_load_segments(image);
>>  		if (ret)
>>  			return ERR_PTR(ret);
>> +		ret = crash_load_dm_crypt_keys(image);
>> +		if (ret)
>> +			pr_debug("Either no dm crypt key or error to retrieve the dm crypt key\n");
>
>If it's error, do we need change ti to pr_debug?

Thanks for raising the concern! I think it's OK to let
crash_load_dm_crypt_keys fail since disk encryption may not be used for
kdump, thus pr_debug is sufficient. Or have I misunderstood your comment?

>>  	}
>>  #endif
>>
>> --
>> 2.47.0
>>
>

-- 
Best regards,
Coiby

Re: [PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

[Baoquan He]

On 12/23/24 at 09:16am, Coiby Xu wrote:
> On Wed, Dec 11, 2024 at 08:55:52PM +0800, Baoquan He wrote:
> > On 10/29/24 at 01:52pm, Coiby Xu wrote:
...... 
> > > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > > index 68530fad05f7..9c94428927bd 100644
> > > --- a/arch/x86/kernel/kexec-bzimage64.c
> > > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > > @@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
> > >  	if (image->type == KEXEC_TYPE_CRASH) {
> > >  		len = sprintf(cmdline_ptr,
> > >  			"elfcorehdr=0x%lx ", image->elf_load_addr);
> > > +
> > > +		if (image->dm_crypt_keys_addr != 0)
> > > +			len += sprintf(cmdline_ptr + len,
> > > +					"dmcryptkeys=0x%lx ", image->dm_crypt_keys_addr);
> > >  	}
> > >  	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
> > >  	cmdline_len += len;
> > > @@ -441,6 +445,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
> > >  		ret = crash_load_segments(image);
> > >  		if (ret)
> > >  			return ERR_PTR(ret);
> > > +		ret = crash_load_dm_crypt_keys(image);
> > > +		if (ret)
> > > +			pr_debug("Either no dm crypt key or error to retrieve the dm crypt key\n");
> > 
> > If it's error, do we need change ti to pr_debug?
> 
> Thanks for raising the concern! I think it's OK to let
> crash_load_dm_crypt_keys fail since disk encryption may not be used for
> kdump, thus pr_debug is sufficient. Or have I misunderstood your comment?

If crash_load_dm_crypt_keys() returned error, shouldn't we handle them
separately? If disk encryption is not used, we return a specific value
to indicate that, surely no need to pr_err(). However, if disk
encryption is used for kdump but error is caused and failed
crash_load_dm_crypt_keys(), why don't we error out for the case?

Maybe below change can be made to differentiate them?

diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
index f72a88b7d106..327f84ea57c5 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -400,7 +400,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
 
        if (key_count <= 0) {
                kexec_dprintk("No dm-crypt keys\n");
-               return -EINVAL;
+               return -ENOENT;
        }

Re: [PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

[Coiby Xu]

On Thu, Dec 26, 2024 at 11:48:04AM +0800, Baoquan He wrote:
>On 12/23/24 at 09:16am, Coiby Xu wrote:
>> On Wed, Dec 11, 2024 at 08:55:52PM +0800, Baoquan He wrote:
>> > On 10/29/24 at 01:52pm, Coiby Xu wrote:
>......
>> > > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
>> > > index 68530fad05f7..9c94428927bd 100644
>> > > --- a/arch/x86/kernel/kexec-bzimage64.c
>> > > +++ b/arch/x86/kernel/kexec-bzimage64.c
>> > > @@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
>> > >  	if (image->type == KEXEC_TYPE_CRASH) {
>> > >  		len = sprintf(cmdline_ptr,
>> > >  			"elfcorehdr=0x%lx ", image->elf_load_addr);
>> > > +
>> > > +		if (image->dm_crypt_keys_addr != 0)
>> > > +			len += sprintf(cmdline_ptr + len,
>> > > +					"dmcryptkeys=0x%lx ", image->dm_crypt_keys_addr);
>> > >  	}
>> > >  	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
>> > >  	cmdline_len += len;
>> > > @@ -441,6 +445,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
>> > >  		ret = crash_load_segments(image);
>> > >  		if (ret)
>> > >  			return ERR_PTR(ret);
>> > > +		ret = crash_load_dm_crypt_keys(image);
>> > > +		if (ret)
>> > > +			pr_debug("Either no dm crypt key or error to retrieve the dm crypt key\n");
>> >
>> > If it's error, do we need change ti to pr_debug?
>>
>> Thanks for raising the concern! I think it's OK to let
>> crash_load_dm_crypt_keys fail since disk encryption may not be used for
>> kdump, thus pr_debug is sufficient. Or have I misunderstood your comment?
>
>If crash_load_dm_crypt_keys() returned error, shouldn't we handle them
>separately? If disk encryption is not used, we return a specific value
>to indicate that, surely no need to pr_err(). However, if disk
>encryption is used for kdump but error is caused and failed
>crash_load_dm_crypt_keys(), why don't we error out for the case?
>
>Maybe below change can be made to differentiate them?
>
>diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>index f72a88b7d106..327f84ea57c5 100644
>--- a/kernel/crash_dump_dm_crypt.c
>+++ b/kernel/crash_dump_dm_crypt.c
>@@ -400,7 +400,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>
>        if (key_count <= 0) {
>                kexec_dprintk("No dm-crypt keys\n");
>-               return -EINVAL;
>+               return -ENOENT;
>        }

Now I see your point! Thanks for the explanation and suggestion! Yes,
it's better to fail early if crash_load_dm_crypt_keys failed when
encrypted disk is used for kdump . I'll apply the change to next
version.

-- 
Best regards,
Coiby

[PATCH v6 7/7] x86/crash: make the page that stores the dm crypt keys inaccessible

[Coiby Xu]

This adds an addition layer of protection for the saved copy of dm
crypt key. Trying to access the saved copy will cause page fault.

Suggested-by: Pingfan Liu <kernelfans@gmail.com>
Signed-off-by: Coiby Xu <coxu@redhat.com>
---
 arch/x86/kernel/machine_kexec_64.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 9c9ac606893e..7389b912ba43 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -579,13 +579,35 @@ static void kexec_mark_crashkres(bool protect)
 	kexec_mark_range(control, crashk_res.end, protect);
 }
 
+/* make the memory storing dm crypt keys in/accessible */
+static void kexec_mark_dm_crypt_keys(bool protect)
+{
+	unsigned long start_paddr, end_paddr;
+	unsigned int nr_pages;
+
+	if (kexec_crash_image->dm_crypt_keys_addr) {
+		start_paddr = kexec_crash_image->dm_crypt_keys_addr;
+		end_paddr = start_paddr + kexec_crash_image->dm_crypt_keys_sz - 1;
+		nr_pages = (PAGE_ALIGN(end_paddr) - PAGE_ALIGN_DOWN(start_paddr))/PAGE_SIZE;
+		if (protect)
+			set_memory_np((unsigned long)phys_to_virt(start_paddr), nr_pages);
+		else
+			__set_memory_prot(
+				(unsigned long)phys_to_virt(start_paddr),
+				nr_pages,
+				__pgprot(_PAGE_PRESENT | _PAGE_NX | _PAGE_RW));
+	}
+}
+
 void arch_kexec_protect_crashkres(void)
 {
 	kexec_mark_crashkres(true);
+	kexec_mark_dm_crypt_keys(true);
 }
 
 void arch_kexec_unprotect_crashkres(void)
 {
+	kexec_mark_dm_crypt_keys(false);
 	kexec_mark_crashkres(false);
 }
 #endif
-- 
2.47.0

Re: [PATCH v6 0/7] Support kdump with LUKS encryption by reusing LUKS volume keys

[Baoquan He]

Hi Coiby,

On 10/29/24 at 01:52pm, Coiby Xu wrote:
> LUKS is the standard for Linux disk encryption, widely adopted by users,
> and in some cases, such as Confidential VMs, it is a requirement. With 
> kdump enabled, when the first kernel crashes, the system can boot into
> the kdump/crash kernel to dump the memory image (i.e., /proc/vmcore) 
> to a specified target. However, there are two challenges when dumping
> vmcore to a LUKS-encrypted device:

I am doing RHEL code rebasing to upstream kernel, will review this next
week.

Thanks
Baoquan

Re: [PATCH v6 0/7] Support kdump with LUKS encryption by reusing LUKS volume keys

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4004 bytes --]

On Tue, 2024-10-29 at 13:52 +0800, Coiby Xu wrote:
> LUKS is the standard for Linux disk encryption, widely adopted by users,
> and in some cases, such as Confidential VMs, it is a requirement. With 
> kdump enabled, when the first kernel crashes, the system can boot into
> the kdump/crash kernel to dump the memory image (i.e., /proc/vmcore) 
> to a specified target. However, there are two challenges when dumping
> vmcore to a LUKS-encrypted device:
> 
>  - 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; For cloud confidential VMs, depending on the policy the
>    kdump kernel may not be able to unseal the keys with TPM and the
>    console virtual keyboard is untrusted.
> 
>  - LUKS2 by default use the memory-hard Argon2 key derivation function
>    which is quite memory-consuming compared to the limited memory reserved
>    for kdump. Take Fedora example, by default, only 256M is reserved for
>    systems having memory between 4G-64G. With LUKS enabled, ~1300M needs
>    to be reserved for kdump. Note if the memory reserved for kdump can't
>    be used by 1st kernel i.e. an user sees ~1300M memory missing in the
>    1st kernel.
> 
> Besides users (at least for Fedora) usually expect kdump to work out of
> the box i.e. no manual password input or custom crashkernel value is
> needed. And it doesn't make sense to derivate the keys again in kdump
> kernel which seems to be redundant work.
> 
> This patch set addresses the above issues by making the LUKS volume keys
> persistent for kdump kernel with the help of cryptsetup's new APIs
> (--link-vk-to-keyring/--volume-key-keyring). Here is the life cycle of
> the kdump copies of LUKS volume keys,
> 
>  1. After the 1st kernel loads the initramfs during boot, systemd
>     use an user-input passphrase to de-crypt the LUKS volume keys
>     or TPM-sealed key and then save the volume keys to specified keyring
>     (using the --link-vk-to-keyring API) and the key will expire within
>     specified time.
> 
>  2. A user space tool (kdump initramfs loader like kdump-utils) create
>     key items inside /sys/kernel/config/crash_dm_crypt_keys to inform
>     the 1st kernel which keys are needed.
> 
>  3. When the kdump initramfs is loaded by the kexec_file_load
>     syscall, the 1st kernel will iterate created key items, save the
>     keys to kdump reserved memory.
> 
>  4. When the 1st kernel crashes and the kdump initramfs is booted, the
>     kdump initramfs asks the kdump kernel to create a user key using the
>     key stored in kdump reserved memory by writing yes to
>     /sys/kernel/crash_dm_crypt_keys/restore. Then the LUKS encrypted
>     device is unlocked with libcryptsetup's --volume-key-keyring API.
> 
>  5. The system gets rebooted to the 1st kernel after dumping vmcore to
>     the LUKS encrypted device is finished
> 
> After libcryptsetup saving the LUKS volume keys to specified keyring,
> whoever takes this should be responsible for the safety of these copies
> of keys. The keys will be saved in the memory area exclusively reserved
> for kdump where even the 1st kernel has no direct access. And further
> more, two additional protections are added,
>  - save the copy randomly in kdump reserved memory as suggested by Jan
>  - clear the _PAGE_PRESENT flag of the page that stores the copy as
>    suggested by Pingfan
> 
> This patch set only supports x86. There will be patches to support other
> architectures once this patch set gets merged.


I'm not very keen on inventing lots of different mechanisms for passing
data over kexec. Shouldn't this just be using the KHO infrastructure?

https://lwn.net/ml/linux-kernel/20240117144704.602-1-graf@amazon.com/

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v6 0/7] Support kdump with LUKS encryption by reusing LUKS volume keys

[Baoquan He]

On 12/03/24 at 05:53pm, David Woodhouse wrote:
> On Tue, 2024-10-29 at 13:52 +0800, Coiby Xu wrote:
> > LUKS is the standard for Linux disk encryption, widely adopted by users,
> > and in some cases, such as Confidential VMs, it is a requirement. With 
> > kdump enabled, when the first kernel crashes, the system can boot into
> > the kdump/crash kernel to dump the memory image (i.e., /proc/vmcore) 
> > to a specified target. However, there are two challenges when dumping
> > vmcore to a LUKS-encrypted device:
> > 
> >  - 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; For cloud confidential VMs, depending on the policy the
> >    kdump kernel may not be able to unseal the keys with TPM and the
> >    console virtual keyboard is untrusted.
> > 
> >  - LUKS2 by default use the memory-hard Argon2 key derivation function
> >    which is quite memory-consuming compared to the limited memory reserved
> >    for kdump. Take Fedora example, by default, only 256M is reserved for
> >    systems having memory between 4G-64G. With LUKS enabled, ~1300M needs
> >    to be reserved for kdump. Note if the memory reserved for kdump can't
> >    be used by 1st kernel i.e. an user sees ~1300M memory missing in the
> >    1st kernel.
> > 
> > Besides users (at least for Fedora) usually expect kdump to work out of
> > the box i.e. no manual password input or custom crashkernel value is
> > needed. And it doesn't make sense to derivate the keys again in kdump
> > kernel which seems to be redundant work.
> > 
> > This patch set addresses the above issues by making the LUKS volume keys
> > persistent for kdump kernel with the help of cryptsetup's new APIs
> > (--link-vk-to-keyring/--volume-key-keyring). Here is the life cycle of
> > the kdump copies of LUKS volume keys,
> > 
> >  1. After the 1st kernel loads the initramfs during boot, systemd
> >     use an user-input passphrase to de-crypt the LUKS volume keys
> >     or TPM-sealed key and then save the volume keys to specified keyring
> >     (using the --link-vk-to-keyring API) and the key will expire within
> >     specified time.
> > 
> >  2. A user space tool (kdump initramfs loader like kdump-utils) create
> >     key items inside /sys/kernel/config/crash_dm_crypt_keys to inform
> >     the 1st kernel which keys are needed.
> > 
> >  3. When the kdump initramfs is loaded by the kexec_file_load
> >     syscall, the 1st kernel will iterate created key items, save the
> >     keys to kdump reserved memory.
> > 
> >  4. When the 1st kernel crashes and the kdump initramfs is booted, the
> >     kdump initramfs asks the kdump kernel to create a user key using the
> >     key stored in kdump reserved memory by writing yes to
> >     /sys/kernel/crash_dm_crypt_keys/restore. Then the LUKS encrypted
> >     device is unlocked with libcryptsetup's --volume-key-keyring API.
> > 
> >  5. The system gets rebooted to the 1st kernel after dumping vmcore to
> >     the LUKS encrypted device is finished
> > 
> > After libcryptsetup saving the LUKS volume keys to specified keyring,
> > whoever takes this should be responsible for the safety of these copies
> > of keys. The keys will be saved in the memory area exclusively reserved
> > for kdump where even the 1st kernel has no direct access. And further
> > more, two additional protections are added,
> >  - save the copy randomly in kdump reserved memory as suggested by Jan
> >  - clear the _PAGE_PRESENT flag of the page that stores the copy as
> >    suggested by Pingfan
> > 
> > This patch set only supports x86. There will be patches to support other
> > architectures once this patch set gets merged.
> 
> 
> I'm not very keen on inventing lots of different mechanisms for passing
> data over kexec. Shouldn't this just be using the KHO infrastructure?

Hmm, this focuses on kdump side to fix a kdump drawback, not on kexec
reboot which KHO is related to. We have discussed this LUKS fix for a
long time, Coiby cooperated with LUKS package maintainers to have swept
all obstacles in userspace, now this patchset is doing its kernel part
change.

Yes, it retrieves the keyrings and store it into the existed crashkernel 
memory reserved for kdump kernel, then restore it in 2nd kernel. I think
KHO is gonna take the similar way, a preliminarily reserved memory,
while unified interfaces to store/restore data. Given KHO is still on
draft high level design, there's nothing for people to use for the time
being. I think later, when KHO is in mainline kernel, LUKS can change to
use KHO's interfaces to simplify codes.

Thanks
Baoquan

Re: [PATCH v6 0/7] Support kdump with LUKS encryption by reusing LUKS volume keys

[Baoquan He]

Hi Coiby,

On 10/29/24 at 01:52pm, Coiby Xu wrote:
> LUKS is the standard for Linux disk encryption, widely adopted by users,
> and in some cases, such as Confidential VMs, it is a requirement. With 
> kdump enabled, when the first kernel crashes, the system can boot into
> the kdump/crash kernel to dump the memory image (i.e., /proc/vmcore) 
> to a specified target. However, there are two challenges when dumping
> vmcore to a LUKS-encrypted device:

Other than those small concerns, and the lkp reported issue, the overral
series looks good to me. Thanks for the effort.

Thanks
Baoquan

Re: [PATCH v6 0/7] Support kdump with LUKS encryption by reusing LUKS volume keys

[Coiby Xu]

On Sat, Dec 14, 2024 at 10:24:26AM +0800, Baoquan He wrote:
>Hi Coiby,

Hi Baoquan,

>
>On 10/29/24 at 01:52pm, Coiby Xu wrote:
>> LUKS is the standard for Linux disk encryption, widely adopted by users,
>> and in some cases, such as Confidential VMs, it is a requirement. With
>> kdump enabled, when the first kernel crashes, the system can boot into
>> the kdump/crash kernel to dump the memory image (i.e., /proc/vmcore)
>> to a specified target. However, there are two challenges when dumping
>> vmcore to a LUKS-encrypted device:
>
>Other than those small concerns, and the lkp reported issue, the overral
>series looks good to me. Thanks for the effort.

Thanks for carefully reviewing the patches and the suggestions to
improve the patch set!

>
>Thanks
>Baoquan
>

-- 
Best regards,
Coiby