LUKS is the standard for Linux disk encryption. Many users choose LUKS
and in some use cases like Confidential VM it's mandated. With kdump
enabled, when the 1st kernel crashes, the system could boot into the
kdump/crash kernel and dump the memory image i.e. /proc/vmcore to a
specified target. Currently, when dumping vmcore to a LUKS
encrypted device, there are two problems,
- 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 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 make 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
this kdump copy of LUKS volume keys,
1. After the 1st kernel loads the initramfs during boot, systemd
use an user-input passphrase or TPM-sealed key to de-crypt the LUKS
volume keys 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 builder) writes a key description to
/sys/kernel/crash_dm_crypt_keys to inform the 1st kernel to record the
key while building the kdump initramfs
3. The kexec_file_load syscall read the volume keys by recored key
descriptions and then save them key to kdump reserved memory and wipe the
copy.
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 to to /sys/kernel/crash_dm_crypt_keys. Then
the LUKS encrypted devide 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.
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 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 | 15 +-
arch/x86/kernel/kexec-bzimage64.c | 7 +
arch/x86/kernel/machine_kexec_64.c | 21 ++
include/linux/crash_core.h | 9 +-
include/linux/crash_dump.h | 2 +
include/linux/kexec.h | 6 +
kernel/Kconfig.kexec | 8 +
kernel/Makefile | 1 +
kernel/crash_dump_dm_crypt.c | 319 +++++++++++++++++++++++++++++
kernel/kexec_file.c | 15 ++
kernel/ksysfs.c | 22 ++
11 files changed, 423 insertions(+), 2 deletions(-)
create mode 100644 kernel/crash_dump_dm_crypt.c
--
2.44.0
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.
Signed-off-by: Coiby Xu <[email protected]>
---
include/linux/crash_core.h | 3 ++
include/linux/crash_dump.h | 2 +
include/linux/kexec.h | 4 ++
kernel/crash_dump_dm_crypt.c | 87 ++++++++++++++++++++++++++++++++++++
4 files changed, 96 insertions(+)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 98825b7e0ea6..1f3d5a4fa6c1 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -37,6 +37,9 @@ static inline void arch_kexec_unprotect_crashkres(void) { }
#ifdef CONFIG_CRASH_DM_CRYPT
int crash_sysfs_dm_crypt_keys_read(char *buf);
int crash_sysfs_dm_crypt_keys_write(const char *buf, size_t count);
+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
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/include/linux/kexec.h b/include/linux/kexec.h
index fc1e20d565d5..b6cedce66828 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -368,6 +368,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 847499cdcd42..b9997fb53351 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -1,4 +1,6 @@
// 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>
@@ -111,3 +113,88 @@ int crash_sysfs_dm_crypt_keys_read(char *buf)
return sprintf(buf, "%s\n", STATE_STR[state]);
}
EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_read);
+
+static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
+{
+ const struct user_key_payload *ukp;
+ struct key *key;
+
+ pr_debug("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;
+
+ memcpy(dm_key->data, ukp->data, ukp->datalen);
+ dm_key->key_size = ukp->datalen;
+ pr_debug("Get dm crypt key (size=%u) %s: %8ph\n", dm_key->key_size,
+ dm_key->key_desc, dm_key->data);
+ return 0;
+}
+
+static int build_keys_header(void)
+{
+ int i, r;
+
+ for (i = 0; i < key_count; i++) {
+ r = read_key_from_user_keying(&keys_header->keys[i]);
+ if (r != 0) {
+ pr_err("Failed to read key %s\n", keys_header->keys[i].key_desc);
+ return r;
+ }
+ }
+
+ 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 (state == FRESH)
+ return 0;
+
+ if (key_count != keys_header->key_count) {
+ pr_err("Only record %u keys (%u in total)\n", key_count,
+ keys_header->key_count);
+ return -EINVAL;
+ }
+
+ image->dm_crypt_keys_addr = 0;
+ r = build_keys_header();
+ if (r)
+ return r;
+
+ kbuf.buffer = keys_header;
+ kbuf.bufsz = keys_header_size;
+
+ kbuf.memsz = kbuf.bufsz;
+ kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
+ kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
+ r = kexec_add_buffer(&kbuf);
+ if (r) {
+ vfree((void *)kbuf.buffer);
+ return r;
+ }
+ state = LOADED;
+ image->dm_crypt_keys_addr = kbuf.mem;
+ image->dm_crypt_keys_sz = kbuf.bufsz;
+ pr_debug("Loaded dm crypt keys at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
+ image->dm_crypt_keys_addr, kbuf.bufsz, kbuf.bufsz);
+
+ return r;
+}
--
2.44.0
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 <[email protected]>
Signed-off-by: Coiby Xu <[email protected]>
---
include/linux/kexec.h | 2 ++
kernel/kexec_file.c | 15 +++++++++++++++
2 files changed, 17 insertions(+)
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 060835bb82d5..fc1e20d565d5 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -171,6 +171,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,6 +183,7 @@ struct kexec_buf {
unsigned long buf_min;
unsigned long buf_max;
bool top_down;
+ bool random;
};
int kexec_load_purgatory(struct kimage *image, struct kexec_buf *kbuf);
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 2d1db05fbf04..e0630fe30d43 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -25,6 +25,7 @@
#include <linux/elfcore.h>
#include <linux/kernel.h>
#include <linux/kernel_read_file.h>
+#include <linux/prandom.h>
#include <linux/syscalls.h>
#include <linux/vmalloc.h>
#include "kexec_internal.h"
@@ -432,6 +433,16 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
return ret;
}
+static unsigned long kexec_random_start(unsigned long start, unsigned long end)
+{
+ unsigned long temp_start;
+ unsigned short i;
+
+ get_random_bytes(&i, sizeof(unsigned short));
+ temp_start = start + (end - start) / USHRT_MAX * i;
+ return temp_start;
+}
+
static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
struct kexec_buf *kbuf)
{
@@ -440,6 +451,8 @@ 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;
+ if (kbuf->random)
+ temp_start = kexec_random_start(temp_start, temp_end);
do {
/* align down start */
@@ -477,6 +490,8 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
unsigned long temp_start, temp_end;
temp_start = max(start, kbuf->buf_min);
+ if (kbuf->random)
+ temp_start = kexec_random_start(temp_start, end);
do {
temp_start = ALIGN(temp_start, kbuf->buf_align);
--
2.44.0
A sysfs /sys/kernel/crash_dm_crypt_keys is provided for user space to make
the dm crypt keys persist for the kdump kernel. User space can send the
following commands,
- "init KEY_NUM"
Initialize needed structures
- "record KEY_DESC"
Record a key description. The key must be a logon key.
User space can also read this API to learn about current state.
Signed-off-by: Coiby Xu <[email protected]>
---
include/linux/crash_core.h | 5 +-
kernel/Kconfig.kexec | 8 +++
kernel/Makefile | 1 +
kernel/crash_dump_dm_crypt.c | 113 +++++++++++++++++++++++++++++++++++
kernel/ksysfs.c | 22 +++++++
5 files changed, 148 insertions(+), 1 deletion(-)
create mode 100644 kernel/crash_dump_dm_crypt.c
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index d33352c2e386..98825b7e0ea6 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -34,7 +34,10 @@ static inline void arch_kexec_protect_crashkres(void) { }
static inline void arch_kexec_unprotect_crashkres(void) { }
#endif
-
+#ifdef CONFIG_CRASH_DM_CRYPT
+int crash_sysfs_dm_crypt_keys_read(char *buf);
+int crash_sysfs_dm_crypt_keys_write(const char *buf, size_t count);
+#endif
#ifndef arch_crash_handle_hotplug_event
static inline void arch_crash_handle_hotplug_event(struct kimage *image) { }
diff --git a/kernel/Kconfig.kexec b/kernel/Kconfig.kexec
index 6c34e63c88ff..88525ad1c80a 100644
--- a/kernel/Kconfig.kexec
+++ b/kernel/Kconfig.kexec
@@ -116,6 +116,14 @@ 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
+ help
+ With this option enabled, user space can intereact with
+ /sys/kernel/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 3c13240dfc9f..f2e5b3e86d12 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..847499cdcd42
--- /dev/null
+++ b/kernel/crash_dump_dm_crypt.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <keys/user-type.h>
+#include <linux/crash_dump.h>
+
+#define KEY_NUM_MAX 128
+#define KEY_SIZE_MAX 256
+
+// The key scription has the format: cryptsetup:UUID 11+36+1(NULL)=48
+#define KEY_DESC_LEN 48
+
+static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded"};
+enum STATE_ENUM {
+ FRESH = 0,
+ INITIALIZED,
+ RECORDED,
+ LOADED,
+} state;
+
+static unsigned int key_count;
+static size_t keys_header_size;
+
+struct dm_crypt_key {
+ unsigned int key_size;
+ char key_desc[KEY_DESC_LEN];
+ u8 data[KEY_SIZE_MAX];
+};
+
+struct keys_header {
+ unsigned int key_count;
+ struct dm_crypt_key keys[] __counted_by(key_count);
+} *keys_header;
+
+static size_t get_keys_header_size(struct keys_header *keys_header,
+ size_t key_count)
+{
+ return struct_size(keys_header, keys, key_count);
+}
+
+static int init(const char *buf)
+{
+ unsigned int total_keys;
+ char dummy[5];
+
+ if (sscanf(buf, "%4s %u", dummy, &total_keys) != 2)
+ return -EINVAL;
+
+ if (key_count > KEY_NUM_MAX) {
+ pr_err("Exceed the maximum number of keys (KEY_NUM_MAX=%u)\n",
+ KEY_NUM_MAX);
+ return -EINVAL;
+ }
+
+ keys_header_size = get_keys_header_size(keys_header, total_keys);
+ key_count = 0;
+
+ keys_header = kzalloc(keys_header_size, GFP_KERNEL);
+ if (!keys_header)
+ return -ENOMEM;
+
+ keys_header->key_count = total_keys;
+ state = INITIALIZED;
+ return 0;
+}
+
+static int record_key_desc(const char *buf, struct dm_crypt_key *dm_key)
+{
+ char key_desc[KEY_DESC_LEN];
+ char dummy[7];
+
+ if (state != INITIALIZED)
+ pr_err("Please send the cmd 'init <KEY_NUM>' first\n");
+
+ if (sscanf(buf, "%6s %s", dummy, key_desc) != 2)
+ return -EINVAL;
+
+ if (key_count >= keys_header->key_count) {
+ pr_warn("Already have %u keys", key_count);
+ return -EINVAL;
+ }
+
+ strscpy(dm_key->key_desc, key_desc, KEY_DESC_LEN);
+ pr_debug("Key%d (%s) recorded\n", key_count, dm_key->key_desc);
+ key_count++;
+
+ if (key_count == keys_header->key_count)
+ state = RECORDED;
+
+ return 0;
+}
+
+static int process_cmd(const char *buf, size_t count)
+{
+ if (strncmp(buf, "init ", 5) == 0)
+ return init(buf);
+ else if (strncmp(buf, "record ", 7) == 0)
+ return record_key_desc(buf, &keys_header->keys[key_count]);
+
+ return -EINVAL;
+}
+
+int crash_sysfs_dm_crypt_keys_write(const char *buf, size_t count)
+{
+ if (!is_kdump_kernel())
+ return process_cmd(buf, count);
+ return -EINVAL;
+}
+EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_write);
+
+int crash_sysfs_dm_crypt_keys_read(char *buf)
+{
+ return sprintf(buf, "%s\n", STATE_STR[state]);
+}
+EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_read);
diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 495b69a71a5d..98cc84d5510c 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -167,6 +167,25 @@ static ssize_t vmcoreinfo_show(struct kobject *kobj,
}
KERNEL_ATTR_RO(vmcoreinfo);
+#ifdef CONFIG_CRASH_DM_CRYPT
+static ssize_t crash_dm_crypt_keys_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return crash_sysfs_dm_crypt_keys_read(buf);
+}
+
+static ssize_t crash_dm_crypt_keys_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+
+ ret = crash_sysfs_dm_crypt_keys_write(buf, count);
+ return ret < 0 ? ret : count;
+}
+KERNEL_ATTR_RW(crash_dm_crypt_keys);
+#endif /* CONFIG_CRASH_DM_CRYPT */
+
#ifdef CONFIG_CRASH_HOTPLUG
static ssize_t crash_elfcorehdr_size_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
@@ -271,6 +290,9 @@ static struct attribute * kernel_attrs[] = {
#endif
#ifdef CONFIG_VMCORE_INFO
&vmcoreinfo_attr.attr,
+#ifdef CONFIG_CRASH_DM_CRYPT
+ &crash_dm_crypt_keys_attr.attr,
+#endif
#ifdef CONFIG_CRASH_HOTPLUG
&crash_elfcorehdr_size_attr.attr,
#endif
--
2.44.0
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/crash_dm_crypt_key, 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 <[email protected]>
---
include/linux/crash_core.h | 1 +
kernel/crash_dump_dm_crypt.c | 99 +++++++++++++++++++++++++++++++++++-
2 files changed, 99 insertions(+), 1 deletion(-)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 1f3d5a4fa6c1..1996e0739b6d 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -38,6 +38,7 @@ static inline void arch_kexec_unprotect_crashkres(void) { }
int crash_sysfs_dm_crypt_keys_read(char *buf);
int crash_sysfs_dm_crypt_keys_write(const char *buf, size_t count);
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/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
index 6ac1fabdb6cb..9ddfe4c4d755 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -33,12 +33,67 @@ struct keys_header {
struct dm_crypt_key keys[] __counted_by(key_count);
} *keys_header;
+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);
+
static size_t get_keys_header_size(struct keys_header *keys_header,
size_t key_count)
{
return struct_size(keys_header, keys, key_count);
}
+/*
+ * Architectures may override this function to read dm crypt key
+ */
+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, false);
+}
+
+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);
+ pr_alert("Success adding key %s", dm_key->key_desc);
+ } else {
+ r = PTR_ERR(key_ref);
+ pr_alert("Error when adding key");
+ }
+
+ key_ref_put(keyring_ref);
+ return r;
+}
+
static int init(const char *buf)
{
unsigned int total_keys;
@@ -120,11 +175,53 @@ static int process_cmd(const char *buf, size_t count)
return -EINVAL;
}
+static int restore_dm_crypt_keys_to_thread_keyring(const char *key_desc)
+{
+ struct dm_crypt_key *key;
+ 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)) {
+ pr_alert("Failed to get keyring");
+ 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) {
+ pr_info("Failed to the number of dm_crypt keys\n");
+ return -1;
+ }
+
+ pr_debug("There are %u keys\n", key_count);
+ addr = dm_crypt_keys_addr;
+
+ keys_header_size = get_keys_header_size(keys_header, 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->key_count; i++) {
+ key = &keys_header->keys[i];
+ pr_alert("Get key (size=%u): %8ph...\n", key->key_size, key->data);
+ add_key_to_keyring(key, keyring_ref);
+ }
+
+ return 0;
+}
+
int crash_sysfs_dm_crypt_keys_write(const char *buf, size_t count)
{
if (!is_kdump_kernel())
return process_cmd(buf, count);
- return -EINVAL;
+ else
+ return restore_dm_crypt_keys_to_thread_keyring(buf);
}
EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_write);
--
2.44.0
When there is CPU/memory hot-plugging, the kdump kernel image and initrd
will be reloaded. The user space can write the "reuse" command to
/sys/kernel/crash_dm_crypt_key so the stored keys can be re-saved again.
Note currently only x86 (commit ea53ad9cf73b ("x86/crash: add x86 crash
hotplug support")) and ppc (WIP) supports the new infrastructure
(commit 247262756121 ("crash: add generic infrastructure for crash
hotplug support")). If the new infrastructure get extended to all arches,
this patch can be dropped.
Signed-off-by: Coiby Xu <[email protected]>
---
kernel/crash_dump_dm_crypt.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
index b9997fb53351..6ac1fabdb6cb 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -10,12 +10,13 @@
// The key scription has the format: cryptsetup:UUID 11+36+1(NULL)=48
#define KEY_DESC_LEN 48
-static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded"};
+static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded", "reuse"};
enum STATE_ENUM {
FRESH = 0,
INITIALIZED,
RECORDED,
LOADED,
+ REUSE,
} state;
static unsigned int key_count;
@@ -90,12 +91,31 @@ static int record_key_desc(const char *buf, struct dm_crypt_key *dm_key)
return 0;
}
+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, keys_header_size);
+ kunmap_local(keys_header_loaded);
+ state = RECORDED;
+}
+
static int process_cmd(const char *buf, size_t count)
{
if (strncmp(buf, "init ", 5) == 0)
return init(buf);
else if (strncmp(buf, "record ", 7) == 0)
return record_key_desc(buf, &keys_header->keys[key_count]);
+ else if (!strcmp(buf, "reuse")) {
+ state = REUSE;
+ get_keys_from_kdump_reserved_memory();
+ return 0;
+ }
return -EINVAL;
}
@@ -175,9 +195,11 @@ int crash_load_dm_crypt_keys(struct kimage *image)
}
image->dm_crypt_keys_addr = 0;
- r = build_keys_header();
- if (r)
- return r;
+ if (state != REUSE) {
+ r = build_keys_header();
+ if (r)
+ return r;
+ }
kbuf.buffer = keys_header;
kbuf.bufsz = keys_header_size;
--
2.44.0
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 <[email protected]>
---
arch/x86/kernel/crash.c | 15 ++++++++++++++-
arch/x86/kernel/kexec-bzimage64.c | 7 +++++++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index e74d0c4286c1..d852f9c99f0e 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -266,6 +266,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
unsigned long long mend)
{
unsigned long start, end;
+ int r;
cmem->ranges[0].start = mstart;
cmem->ranges[0].end = mend;
@@ -274,7 +275,19 @@ 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);
+ r = crash_exclude_mem_range(cmem, start, end);
+
+ if (r)
+ return r;
+
+ /* 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 r;
}
/* Prepare memory map for crash dump kernel */
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.44.0
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 <[email protected]>
Signed-off-by: Coiby Xu <[email protected]>
---
arch/x86/kernel/machine_kexec_64.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index b180d8e497c3..fc0a80f4254e 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -545,13 +545,34 @@ static void kexec_mark_crashkres(bool protect)
kexec_mark_range(control, crashk_res.end, protect);
}
+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.44.0
Hi Coiby,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc5 next-20240426]
[cannot apply to tip/x86/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20240425-180836
base: linus/master
patch link: https://lore.kernel.org/r/20240425100434.198925-3-coxu%40redhat.com
patch subject: [PATCH v3 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
config: x86_64-randconfig-r113-20240426 (https://download.01.org/0day-ci/archive/20240426/[email protected]/config)
compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240426/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> kernel/crash_dump_dm_crypt.c:31:3: sparse: sparse: symbol 'keys_header' was not declared. Should it be static?
vim +/keys_header +31 kernel/crash_dump_dm_crypt.c
27
28 struct keys_header {
29 unsigned int key_count;
30 struct dm_crypt_key keys[] __counted_by(key_count);
> 31 } *keys_header;
32
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 04/25/24 at 06:04pm, 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.
Do you want to randomize the key's position for both kdump and kexec
rebooting? Assume you only want to do that for kdump. If so, we may need
to make that more specific in code.
>
> Suggested-by: Jan Pazdziora <[email protected]>
> Signed-off-by: Coiby Xu <[email protected]>
> ---
> include/linux/kexec.h | 2 ++
> kernel/kexec_file.c | 15 +++++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 060835bb82d5..fc1e20d565d5 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -171,6 +171,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,6 +183,7 @@ struct kexec_buf {
> unsigned long buf_min;
> unsigned long buf_max;
> bool top_down;
> + bool random;
> };
>
> int kexec_load_purgatory(struct kimage *image, struct kexec_buf *kbuf);
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 2d1db05fbf04..e0630fe30d43 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -25,6 +25,7 @@
> #include <linux/elfcore.h>
> #include <linux/kernel.h>
> #include <linux/kernel_read_file.h>
> +#include <linux/prandom.h>
> #include <linux/syscalls.h>
> #include <linux/vmalloc.h>
> #include "kexec_internal.h"
> @@ -432,6 +433,16 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
> return ret;
> }
>
> +static unsigned long kexec_random_start(unsigned long start, unsigned long end)
> +{
> + unsigned long temp_start;
> + unsigned short i;
> +
> + get_random_bytes(&i, sizeof(unsigned short));
> + temp_start = start + (end - start) / USHRT_MAX * i;
> + return temp_start;
> +}
> +
> static int locate_mem_hole_top_down(unsigned long start, unsigned long end,
> struct kexec_buf *kbuf)
> {
> @@ -440,6 +451,8 @@ 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;
> + if (kbuf->random)
> + temp_start = kexec_random_start(temp_start, temp_end);
>
> do {
> /* align down start */
> @@ -477,6 +490,8 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
> unsigned long temp_start, temp_end;
>
> temp_start = max(start, kbuf->buf_min);
> + if (kbuf->random)
> + temp_start = kexec_random_start(temp_start, end);
>
> do {
> temp_start = ALIGN(temp_start, kbuf->buf_align);
> --
> 2.44.0
>
Please don't add [email protected] in the public list because it's a
internal mailing list or aliase. And I got error when replying.
On 04/25/24 at 06:04pm, Coiby Xu wrote:
> LUKS is the standard for Linux disk encryption. Many users choose LUKS
> and in some use cases like Confidential VM it's mandated. With kdump
> enabled, when the 1st kernel crashes, the system could boot into the
> kdump/crash kernel and dump the memory image i.e. /proc/vmcore to a
> specified target. Currently, when dumping vmcore to a LUKS
> encrypted device, there are two problems,
>
> - 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 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 make 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
> this kdump copy of LUKS volume keys,
>
> 1. After the 1st kernel loads the initramfs during boot, systemd
> use an user-input passphrase or TPM-sealed key to de-crypt the LUKS
> volume keys 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 builder) writes a key description to
> /sys/kernel/crash_dm_crypt_keys to inform the 1st kernel to record the
> key while building the kdump initramfs
>
> 3. The kexec_file_load syscall read the volume keys by recored key
> descriptions and then save them key to kdump reserved memory and wipe the
> copy.
>
> 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 to to /sys/kernel/crash_dm_crypt_keys. Then
> the LUKS encrypted devide 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.
>
> 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 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 | 15 +-
> arch/x86/kernel/kexec-bzimage64.c | 7 +
> arch/x86/kernel/machine_kexec_64.c | 21 ++
> include/linux/crash_core.h | 9 +-
> include/linux/crash_dump.h | 2 +
> include/linux/kexec.h | 6 +
> kernel/Kconfig.kexec | 8 +
> kernel/Makefile | 1 +
> kernel/crash_dump_dm_crypt.c | 319 +++++++++++++++++++++++++++++
> kernel/kexec_file.c | 15 ++
> kernel/ksysfs.c | 22 ++
> 11 files changed, 423 insertions(+), 2 deletions(-)
> create mode 100644 kernel/crash_dump_dm_crypt.c
>
> --
> 2.44.0
>
On Mon, May 20, 2024 at 02:18:09PM +0800, Baoquan He wrote:
>Please don't add [email protected] in the public list because it's a
>internal mailing list or aliase. And I got error when replying.
Thanks for the reminder! Actually it's a public mailing list and you
can find all emails publicly listed on [1]. I did some research and it
seems we should email to [email protected] instead. Quoting [2],
> To post a message to all the list members (who were subscribed with
> mail delivery enabled as of 2023.10.20), send email to
> [email protected]. You can no longer subscribe to
> [email protected], to subscribe to [email protected] please
> send email to [email protected].
[1] https://lore.kernel.org/dm-devel/
[2] https://listman.redhat.com/mailman/listinfo/dm-devel
--
Best regards,
Coiby
On Mon, May 20, 2024 at 02:16:43PM +0800, Baoquan He wrote:
>On 04/25/24 at 06:04pm, 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.
>
>Do you want to randomize the key's position for both kdump and kexec
>rebooting? Assume you only want to do that for kdump. If so, we may need
>to make that more specific in code.
Thanks for the suggestion! Currently, no one has requested this feature
for kexec reboot so yes, I only have kdump in mind. But kdump depends
on kexec thus I'm not sure how we can make it kdump specfic. Do you have
a further suggestion?
>diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>index 060835bb82d5..fc1e20d565d5 100644
>--- a/include/linux/kexec.h
>+++ b/include/linux/kexec.h
>@@ -171,6 +171,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.
How about a comment here saying this is currently only used by kdump.
--
Best regards,
Coiby
On 05/21/24 at 09:58am, Coiby Xu wrote:
> On Mon, May 20, 2024 at 02:16:43PM +0800, Baoquan He wrote:
> > On 04/25/24 at 06:04pm, 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.
> >
> > Do you want to randomize the key's position for both kdump and kexec
> > rebooting? Assume you only want to do that for kdump. If so, we may need
> > to make that more specific in code.
>
> Thanks for the suggestion! Currently, no one has requested this feature
> for kexec reboot so yes, I only have kdump in mind. But kdump depends
> on kexec thus I'm not sure how we can make it kdump specfic. Do you have
> a further suggestion?
I remember you said kexec reboot doesn't need the key passed from 1st
kernel to 2nd kernel because the 2nd kernel will calculate one during
boot.
kbuf has the information, the similar handling has been in
kernel/kexec_file.c:
#ifdef CONFIG_CRASH_DUMP
if (kbuf->image->type == KEXEC_TYPE_CRASH)
....;
#endif
>
>
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index 060835bb82d5..fc1e20d565d5 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -171,6 +171,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.
>
> How about a comment here saying this is currently only used by kdump.
No, it's not good. Please don't do this, let code tell it.
By the way, can you rebase this series on the latest v6.9 and resend? I
rebase my code and can't apply your patchset.
On 05/21/24 at 09:43am, Coiby Xu wrote:
> On Mon, May 20, 2024 at 02:18:09PM +0800, Baoquan He wrote:
> > Please don't add [email protected] in the public list because it's a
> > internal mailing list or aliase. And I got error when replying.
>
> Thanks for the reminder! Actually it's a public mailing list and you
> can find all emails publicly listed on [1]. I did some research and it
> seems we should email to [email protected] instead. Quoting [2],
I always got this when replying to your patch. Maybe you registered
before.
msmtp: recipient address [email protected] not accepted by the server
msmtp: server message: 550 5.1.1 <[email protected]>: Recipient address rejected: User unknown in local
recipient table
> > To post a message to all the list members (who were subscribed with
> > mail delivery enabled as of 2023.10.20), send email to
> > [email protected]. You can no longer subscribe to
> > [email protected], to subscribe to [email protected] please
> > send email to [email protected].
>
> [1] https://lore.kernel.org/dm-devel/
> [2] https://listman.redhat.com/mailman/listinfo/dm-devel
>
> --
> Best regards,
> Coiby
>
On 04/26/24 at 09:10pm, kernel test robot wrote:
> Hi Coiby,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.9-rc5 next-20240426]
> [cannot apply to tip/x86/core]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20240425-180836
> base: linus/master
> patch link: https://lore.kernel.org/r/20240425100434.198925-3-coxu%40redhat.com
> patch subject: [PATCH v3 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
> config: x86_64-randconfig-r113-20240426 (https://download.01.org/0day-ci/archive/20240426/[email protected]/config)
> compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240426/[email protected]/reproduce)
Please respond to the lkp report in time whether it's a problem or not,
otherwise the link will be unavailable.
>
> 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 <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> sparse warnings: (new ones prefixed by >>)
> >> kernel/crash_dump_dm_crypt.c:31:3: sparse: sparse: symbol 'keys_header' was not declared. Should it be static?
>
> vim +/keys_header +31 kernel/crash_dump_dm_crypt.c
>
> 27
> 28 struct keys_header {
> 29 unsigned int key_count;
> 30 struct dm_crypt_key keys[] __counted_by(key_count);
> > 31 } *keys_header;
> 32
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
On 04/25/24 at 06:04pm, Coiby Xu wrote:
> 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.
>
> Signed-off-by: Coiby Xu <[email protected]>
> ---
> include/linux/crash_core.h | 3 ++
> include/linux/crash_dump.h | 2 +
> include/linux/kexec.h | 4 ++
> kernel/crash_dump_dm_crypt.c | 87 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 96 insertions(+)
>
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index 98825b7e0ea6..1f3d5a4fa6c1 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -37,6 +37,9 @@ static inline void arch_kexec_unprotect_crashkres(void) { }
> #ifdef CONFIG_CRASH_DM_CRYPT
> int crash_sysfs_dm_crypt_keys_read(char *buf);
> int crash_sysfs_dm_crypt_keys_write(const char *buf, size_t count);
> +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
> 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/include/linux/kexec.h b/include/linux/kexec.h
> index fc1e20d565d5..b6cedce66828 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -368,6 +368,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 847499cdcd42..b9997fb53351 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -1,4 +1,6 @@
> // 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>
>
> @@ -111,3 +113,88 @@ int crash_sysfs_dm_crypt_keys_read(char *buf)
> return sprintf(buf, "%s\n", STATE_STR[state]);
> }
> EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_read);
> +
> +static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
> +{
> + const struct user_key_payload *ukp;
> + struct key *key;
> +
> + pr_debug("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;
> +
> + memcpy(dm_key->data, ukp->data, ukp->datalen);
> + dm_key->key_size = ukp->datalen;
> + pr_debug("Get dm crypt key (size=%u) %s: %8ph\n", dm_key->key_size,
> + dm_key->key_desc, dm_key->data);
> + return 0;
> +}
> +
> +static int build_keys_header(void)
> +{
> + int i, r;
> +
> + for (i = 0; i < key_count; i++) {
> + r = read_key_from_user_keying(&keys_header->keys[i]);
> + if (r != 0) {
> + pr_err("Failed to read key %s\n", keys_header->keys[i].key_desc);
> + return r;
> + }
> + }
> +
> + 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 (state == FRESH)
> + return 0;
> +
> + if (key_count != keys_header->key_count) {
> + pr_err("Only record %u keys (%u in total)\n", key_count,
> + keys_header->key_count);
> + return -EINVAL;
> + }
> +
> + image->dm_crypt_keys_addr = 0;
> + r = build_keys_header();
> + if (r)
> + return r;
> +
> + kbuf.buffer = keys_header;
> + kbuf.bufsz = keys_header_size;
> +
> + kbuf.memsz = kbuf.bufsz;
> + kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> + kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> + r = kexec_add_buffer(&kbuf);
> + if (r) {
> + vfree((void *)kbuf.buffer);
> + return r;
> + }
> + state = LOADED;
> + image->dm_crypt_keys_addr = kbuf.mem;
> + image->dm_crypt_keys_sz = kbuf.bufsz;
> + pr_debug("Loaded dm crypt keys at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
> + image->dm_crypt_keys_addr, kbuf.bufsz, kbuf.bufsz);
Please use kexec_dprintk() instead to print debugging message.
And you don't worry this printing will leak the key position and the
information?
> +
> + return r;
> +}
> --
> 2.44.0
>
On 04/25/24 at 06:04pm, Coiby Xu wrote:
> When there is CPU/memory hot-plugging, the kdump kernel image and initrd
> will be reloaded. The user space can write the "reuse" command to
> /sys/kernel/crash_dm_crypt_key so the stored keys can be re-saved again.
>
> Note currently only x86 (commit ea53ad9cf73b ("x86/crash: add x86 crash
> hotplug support")) and ppc (WIP) supports the new infrastructure
> (commit 247262756121 ("crash: add generic infrastructure for crash
> hotplug support")). If the new infrastructure get extended to all arches,
> this patch can be dropped.
>
> Signed-off-by: Coiby Xu <[email protected]>
> ---
> kernel/crash_dump_dm_crypt.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> index b9997fb53351..6ac1fabdb6cb 100644
> --- a/kernel/crash_dump_dm_crypt.c
> +++ b/kernel/crash_dump_dm_crypt.c
> @@ -10,12 +10,13 @@
> // The key scription has the format: cryptsetup:UUID 11+36+1(NULL)=48
> #define KEY_DESC_LEN 48
>
> -static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded"};
> +static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded", "reuse"};
> enum STATE_ENUM {
> FRESH = 0,
> INITIALIZED,
> RECORDED,
> LOADED,
> + REUSE,
> } state;
>
> static unsigned int key_count;
> @@ -90,12 +91,31 @@ static int record_key_desc(const char *buf, struct dm_crypt_key *dm_key)
> return 0;
> }
>
> +static void get_keys_from_kdump_reserved_memory(void)
> +{
> + struct keys_header *keys_header_loaded;
> +
> + arch_kexec_unprotect_crashkres();
I don't see the corresponging arch_kexec_protect_crashkres() in the
following flow. Aren't they a pair when used?
> +
> + keys_header_loaded = kmap_local_page(pfn_to_page(
> + kexec_crash_image->dm_crypt_keys_addr >> PAGE_SHIFT));
> +
> + memcpy(keys_header, keys_header_loaded, keys_header_size);
> + kunmap_local(keys_header_loaded);
> + state = RECORDED;
> +}
> +
> static int process_cmd(const char *buf, size_t count)
> {
> if (strncmp(buf, "init ", 5) == 0)
> return init(buf);
> else if (strncmp(buf, "record ", 7) == 0)
> return record_key_desc(buf, &keys_header->keys[key_count]);
> + else if (!strcmp(buf, "reuse")) {
> + state = REUSE;
> + get_keys_from_kdump_reserved_memory();
> + return 0;
> + }
>
> return -EINVAL;
> }
> @@ -175,9 +195,11 @@ int crash_load_dm_crypt_keys(struct kimage *image)
> }
>
> image->dm_crypt_keys_addr = 0;
> - r = build_keys_header();
> - if (r)
> - return r;
> + if (state != REUSE) {
> + r = build_keys_header();
> + if (r)
> + return r;
> + }
>
> kbuf.buffer = keys_header;
> kbuf.bufsz = keys_header_size;
> --
> 2.44.0
>
On 04/25/24 at 06:04pm, Coiby Xu wrote:
> 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 <[email protected]>
> Signed-off-by: Coiby Xu <[email protected]>
> ---
> arch/x86/kernel/machine_kexec_64.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index b180d8e497c3..fc0a80f4254e 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -545,13 +545,34 @@ static void kexec_mark_crashkres(bool protect)
> kexec_mark_range(control, crashk_res.end, protect);
> }
>
> +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);
Really? Are all x86 systems having this dm_crypt_keys and need be
handled in kdump?
> }
>
> void arch_kexec_unprotect_crashkres(void)
> {
> + kexec_mark_dm_crypt_keys(false);
> kexec_mark_crashkres(false);
> }
> #endif
> --
> 2.44.0
>
On Tue, May 21, 2024 at 11:20:35AM +0800, Baoquan He wrote:
>On 04/26/24 at 09:10pm, kernel test robot wrote:
>> Hi Coiby,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on linus/master]
>> [also build test WARNING on v6.9-rc5 next-20240426]
>> [cannot apply to tip/x86/core]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>
>> url: https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20240425-180836
>> base: linus/master
>> patch link: https://lore.kernel.org/r/20240425100434.198925-3-coxu%40redhat.com
>> patch subject: [PATCH v3 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
>> config: x86_64-randconfig-r113-20240426 (https://download.01.org/0day-ci/archive/20240426/[email protected]/config)
>> compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240426/[email protected]/reproduce)
>
>
>Please respond to the lkp report in time whether it's a problem or not,
>otherwise the link will be unavailable.
Thanks for the reminder! I've fixed the reported problem in v4.
>
>>
>> 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 <[email protected]>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>
>> sparse warnings: (new ones prefixed by >>)
>> >> kernel/crash_dump_dm_crypt.c:31:3: sparse: sparse: symbol 'keys_header' was not declared. Should it be static?
Yes, symbol 'keys_header' should be static! Thanks for the report!
>>
>> vim +/keys_header +31 kernel/crash_dump_dm_crypt.c
>>
>> 27
>> 28 struct keys_header {
>> 29 unsigned int key_count;
>> 30 struct dm_crypt_key keys[] __counted_by(key_count);
>> > 31 } *keys_header;
>> 32
>>
>> --
>> 0-DAY CI Kernel Test Service
>> https://github.com/intel/lkp-tests/wiki
>>
>
--
Best regards,
Coiby
On Tue, May 21, 2024 at 11:13:43AM +0800, Baoquan He wrote:
>On 05/21/24 at 09:58am, Coiby Xu wrote:
>> On Mon, May 20, 2024 at 02:16:43PM +0800, Baoquan He wrote:
>> > On 04/25/24 at 06:04pm, 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.
>> >
>> > Do you want to randomize the key's position for both kdump and kexec
>> > rebooting? Assume you only want to do that for kdump. If so, we may need
>> > to make that more specific in code.
>>
>> Thanks for the suggestion! Currently, no one has requested this feature
>> for kexec reboot so yes, I only have kdump in mind. But kdump depends
>> on kexec thus I'm not sure how we can make it kdump specfic. Do you have
>> a further suggestion?
>
>I remember you said kexec reboot doesn't need the key passed from 1st
>kernel to 2nd kernel because the 2nd kernel will calculate one during
>boot.
>
>kbuf has the information, the similar handling has been in
>kernel/kexec_file.c:
>
>#ifdef CONFIG_CRASH_DUMP
> if (kbuf->image->type == KEXEC_TYPE_CRASH)
> ....;
>#endif
Thanks for the suggestion! I'll wrap related code inside
CONFIG_CRASH_DUMP.
>
>> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> > index 060835bb82d5..fc1e20d565d5 100644
>> > --- a/include/linux/kexec.h
>> > +++ b/include/linux/kexec.h
>> > @@ -171,6 +171,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.
>>
>> How about a comment here saying this is currently only used by kdump.
>
>No, it's not good. Please don't do this, let code tell it.
>
>By the way, can you rebase this series on the latest v6.9 and resend? I
>rebase my code and can't apply your patchset.
Sure, v4 was sent.
--
Best regards,
Coiby
On Tue, May 21, 2024 at 11:42:52AM +0800, Baoquan He wrote:
>On 04/25/24 at 06:04pm, Coiby Xu wrote:
>> 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.
>>
>> Signed-off-by: Coiby Xu <[email protected]>
>> ---
>> include/linux/crash_core.h | 3 ++
>> include/linux/crash_dump.h | 2 +
>> include/linux/kexec.h | 4 ++
>> kernel/crash_dump_dm_crypt.c | 87 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 96 insertions(+)
>>
>> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
>> index 98825b7e0ea6..1f3d5a4fa6c1 100644
>> --- a/include/linux/crash_core.h
>> +++ b/include/linux/crash_core.h
>> @@ -37,6 +37,9 @@ static inline void arch_kexec_unprotect_crashkres(void) { }
>> #ifdef CONFIG_CRASH_DM_CRYPT
>> int crash_sysfs_dm_crypt_keys_read(char *buf);
>> int crash_sysfs_dm_crypt_keys_write(const char *buf, size_t count);
>> +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
>> 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/include/linux/kexec.h b/include/linux/kexec.h
>> index fc1e20d565d5..b6cedce66828 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -368,6 +368,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 847499cdcd42..b9997fb53351 100644
>> --- a/kernel/crash_dump_dm_crypt.c
>> +++ b/kernel/crash_dump_dm_crypt.c
>> @@ -1,4 +1,6 @@
>> // 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>
>>
>> @@ -111,3 +113,88 @@ int crash_sysfs_dm_crypt_keys_read(char *buf)
>> return sprintf(buf, "%s\n", STATE_STR[state]);
>> }
>> EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_read);
>> +
>> +static int read_key_from_user_keying(struct dm_crypt_key *dm_key)
>> +{
>> + const struct user_key_payload *ukp;
>> + struct key *key;
>> +
>> + pr_debug("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;
>> +
>> + memcpy(dm_key->data, ukp->data, ukp->datalen);
>> + dm_key->key_size = ukp->datalen;
>> + pr_debug("Get dm crypt key (size=%u) %s: %8ph\n", dm_key->key_size,
>> + dm_key->key_desc, dm_key->data);
>> + return 0;
>> +}
>> +
>> +static int build_keys_header(void)
>> +{
>> + int i, r;
>> +
>> + for (i = 0; i < key_count; i++) {
>> + r = read_key_from_user_keying(&keys_header->keys[i]);
>> + if (r != 0) {
>> + pr_err("Failed to read key %s\n", keys_header->keys[i].key_desc);
>> + return r;
>> + }
>> + }
>> +
>> + 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 (state == FRESH)
>> + return 0;
>> +
>> + if (key_count != keys_header->key_count) {
>> + pr_err("Only record %u keys (%u in total)\n", key_count,
>> + keys_header->key_count);
>> + return -EINVAL;
>> + }
>> +
>> + image->dm_crypt_keys_addr = 0;
>> + r = build_keys_header();
>> + if (r)
>> + return r;
>> +
>> + kbuf.buffer = keys_header;
>> + kbuf.bufsz = keys_header_size;
>> +
>> + kbuf.memsz = kbuf.bufsz;
>> + kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>> + kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> + r = kexec_add_buffer(&kbuf);
>> + if (r) {
>> + vfree((void *)kbuf.buffer);
>> + return r;
>> + }
>> + state = LOADED;
>> + image->dm_crypt_keys_addr = kbuf.mem;
>> + image->dm_crypt_keys_sz = kbuf.bufsz;
>> + pr_debug("Loaded dm crypt keys at 0x%lx bufsz=0x%lx memsz=0x%lx\n",
>> + image->dm_crypt_keys_addr, kbuf.bufsz, kbuf.bufsz);
>
>Please use kexec_dprintk() instead to print debugging message.
Thanks for pointing me to kexec_dprintk! I'll use kexec_dprintk.
>And you don't worry this printing will leak the key position and the
>information?
Thanks for raising this concern! I'll remove the key position info as it
seems kernel dyndbg can be easily enabled.
--
Best regards,
Coiby
On Tue, May 21, 2024 at 11:48:49AM +0800, Baoquan He wrote:
>On 04/25/24 at 06:04pm, Coiby Xu wrote:
>> When there is CPU/memory hot-plugging, the kdump kernel image and initrd
>> will be reloaded. The user space can write the "reuse" command to
>> /sys/kernel/crash_dm_crypt_key so the stored keys can be re-saved again.
>>
>> Note currently only x86 (commit ea53ad9cf73b ("x86/crash: add x86 crash
>> hotplug support")) and ppc (WIP) supports the new infrastructure
>> (commit 247262756121 ("crash: add generic infrastructure for crash
>> hotplug support")). If the new infrastructure get extended to all arches,
>> this patch can be dropped.
>>
>> Signed-off-by: Coiby Xu <[email protected]>
>> ---
>> kernel/crash_dump_dm_crypt.c | 30 ++++++++++++++++++++++++++----
>> 1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>> index b9997fb53351..6ac1fabdb6cb 100644
>> --- a/kernel/crash_dump_dm_crypt.c
>> +++ b/kernel/crash_dump_dm_crypt.c
>> @@ -10,12 +10,13 @@
>> // The key scription has the format: cryptsetup:UUID 11+36+1(NULL)=48
>> #define KEY_DESC_LEN 48
>>
>> -static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded"};
>> +static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded", "reuse"};
>> enum STATE_ENUM {
>> FRESH = 0,
>> INITIALIZED,
>> RECORDED,
>> LOADED,
>> + REUSE,
>> } state;
>>
>> static unsigned int key_count;
>> @@ -90,12 +91,31 @@ static int record_key_desc(const char *buf, struct dm_crypt_key *dm_key)
>> return 0;
>> }
>>
>> +static void get_keys_from_kdump_reserved_memory(void)
>> +{
>> + struct keys_header *keys_header_loaded;
>> +
>> + arch_kexec_unprotect_crashkres();
>
>I don't see the corresponging arch_kexec_protect_crashkres() in the
>following flow. Aren't they a pair when used?
Thanks for raising the concern! The user space is supposed to load the
kdump kernel as next step. But for code readability and security, it's
better to add arch_kexec_protect_crashkres.
--
Best regards,
Coiby
On Tue, May 21, 2024 at 11:51:00AM +0800, Baoquan He wrote:
>On 04/25/24 at 06:04pm, Coiby Xu wrote:
>> 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 <[email protected]>
>> Signed-off-by: Coiby Xu <[email protected]>
>> ---
>> arch/x86/kernel/machine_kexec_64.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
>> index b180d8e497c3..fc0a80f4254e 100644
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -545,13 +545,34 @@ static void kexec_mark_crashkres(bool protect)
>> kexec_mark_range(control, crashk_res.end, protect);
>> }
>>
>> +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);
>
>Really? Are all x86 systems having this dm_crypt_keys and need be
>handled in kdump?
I'm not sure if I understand the above question correctly. But
kexec_mark_dm_crypt_keys will only take effect when there are
dm_crypt_keys because there is a check on
kexec_crash_image->dm_crypt_keys_addr.
--
Best regards,
Coiby
On Tue, 21 May 2024 at 11:20, Baoquan He <[email protected]> wrote:
>
> On 05/21/24 at 09:43am, Coiby Xu wrote:
> > On Mon, May 20, 2024 at 02:18:09PM +0800, Baoquan He wrote:
> > > Please don't add [email protected] in the public list because it's a
> > > internal mailing list or aliase. And I got error when replying.
> >
> > Thanks for the reminder! Actually it's a public mailing list and you
> > can find all emails publicly listed on [1]. I did some research and it
> > seems we should email to [email protected] instead. Quoting [2],
>
> I always got this when replying to your patch. Maybe you registered
> before.
>
> msmtp: recipient address [email protected] not accepted by the server
> msmtp: server message: 550 5.1.1 <[email protected]>: Recipient address rejected: User unknown in local
> recipient table
Hi Baoquan and Coiby, lists on listman.redhat.com have all been
migrated somewhere else. For dm-devel, according to MAINTAINERS it is
[email protected]. I think the old address should not be used
otherwise it is expected to see some errors.
>
>
> > > To post a message to all the list members (who were subscribed with
> > > mail delivery enabled as of 2023.10.20), send email to
> > > [email protected]. You can no longer subscribe to
> > > [email protected], to subscribe to [email protected] please
> > > send email to [email protected].
> >
> > [1] https://lore.kernel.org/dm-devel/
> > [2] https://listman.redhat.com/mailman/listinfo/dm-devel
> >
> > --
> > Best regards,
> > Coiby
> >
>