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.
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 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
base-commit: de7e71ef8bed222dd144d8878091ecb6d5dfd208
--
2.45.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 f0e9f8eda7a3..cc81b8a903ab 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 3d64290d24c9..06b77f9ac4cc 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"
@@ -437,6 +438,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)
{
@@ -445,6 +456,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 */
@@ -482,6 +495,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.45.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 44305336314e..6bff1c24efa3 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, void *arg) { }
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..78809189084a
--- /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"};
+static 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];
+};
+
+static 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 07fb5987b42b..2ba4dcbf5816 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.45.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 6bff1c24efa3..ab20829d0bc9 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 cc81b8a903ab..bd40f4208e1f 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -370,6 +370,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 78809189084a..89fec768fba8 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) {
+ kvfree((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.45.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 89fec768fba8..b4dc881cc867 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"};
static 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.45.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 ab20829d0bc9..d7308b6e83f4 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 b4dc881cc867..dd818581858b 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -33,12 +33,67 @@ static 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.45.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 f06501445cd9..74b3844ae53c 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.45.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.45.0
On Thu, May 23, 2024 at 01:04:43PM +0800, Coiby Xu wrote:
> 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.
"logon"? What is that?
>
> User space can also read this API to learn about current state.
But you don't document it in Documentation/ABI/ so we don't know if this
really is the case, and no one will know how to use it :(
> 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 44305336314e..6bff1c24efa3 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, void *arg) { }
> 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..78809189084a
> --- /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
Why these values?
> +
> +// 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 enum STATE_ENUM {
> + FRESH = 0,
> + INITIALIZED,
> + RECORDED,
> + LOADED,
> +} state;
How are you going to keep these enums synced up with the string values?
> +
> +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];
> +};
> +
> +static 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];
Why 5?
> +
> + if (sscanf(buf, "%4s %u", dummy, &total_keys) != 2)
Didn't you just overflow dummy now?
> + return -EINVAL;
> +
> + if (key_count > KEY_NUM_MAX) {
> + pr_err("Exceed the maximum number of keys (KEY_NUM_MAX=%u)\n",
> + KEY_NUM_MAX);
Do not let userspace spam the kernel log directly if it sends it invalid
data.
> + 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");
Again, don't let userspace spam the log.
> +
> + 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);
EXPORT_SYMBOL_GPL() as you are dealing with a sysfs api?
> +
> +int crash_sysfs_dm_crypt_keys_read(char *buf)
> +{
> + return sprintf(buf, "%s\n", STATE_STR[state]);
sysfs_emit() please.
> +}
> +EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_read);
Again, EXPORT_SYMBOL_GPL()?
> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> index 07fb5987b42b..2ba4dcbf5816 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;
Personally, I hate ? : lines, just write it out, the compiler is the
same and this way it is much more readable:
if (ret < 0)
return ret;
return count;
thanks,
greg k-h
Hi Coiby,
kernel test robot noticed the following build errors:
[auto build test ERROR on de7e71ef8bed222dd144d8878091ecb6d5dfd208]
url: https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20240523-130727
base: de7e71ef8bed222dd144d8878091ecb6d5dfd208
patch link: https://lore.kernel.org/r/20240523050451.788754-4-coxu%40redhat.com
patch subject: [PATCH v4 3/7] crash_dump: store dm keys in kdump reserved memory
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20240524/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240524/[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]/
All errors (new ones prefixed by >>):
kernel/crash_dump_dm_crypt.c: In function 'crash_load_dm_crypt_keys':
>> kernel/crash_dump_dm_crypt.c:158:16: error: variable 'kbuf' has initializer but incomplete type
158 | struct kexec_buf kbuf = {
| ^~~~~~~~~
>> kernel/crash_dump_dm_crypt.c:159:18: error: 'struct kexec_buf' has no member named 'image'
159 | .image = image,
| ^~~~~
kernel/crash_dump_dm_crypt.c:159:26: warning: excess elements in struct initializer
159 | .image = image,
| ^~~~~
kernel/crash_dump_dm_crypt.c:159:26: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:160:18: error: 'struct kexec_buf' has no member named 'buf_min'
160 | .buf_min = 0,
| ^~~~~~~
kernel/crash_dump_dm_crypt.c:160:28: warning: excess elements in struct initializer
160 | .buf_min = 0,
| ^
kernel/crash_dump_dm_crypt.c:160:28: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:161:18: error: 'struct kexec_buf' has no member named 'buf_max'
161 | .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/arm/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:161:28: note: in expansion of macro 'ULONG_MAX'
161 | .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:161:28: note: in expansion of macro 'ULONG_MAX'
161 | .buf_max = ULONG_MAX,
| ^~~~~~~~~
>> kernel/crash_dump_dm_crypt.c:162:18: error: 'struct kexec_buf' has no member named 'top_down'
162 | .top_down = false,
| ^~~~~~~~
kernel/crash_dump_dm_crypt.c:162:29: warning: excess elements in struct initializer
162 | .top_down = false,
| ^~~~~
kernel/crash_dump_dm_crypt.c:162:29: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:163:18: error: 'struct kexec_buf' has no member named 'random'
163 | .random = true,
| ^~~~~~
kernel/crash_dump_dm_crypt.c:163:27: warning: excess elements in struct initializer
163 | .random = true,
| ^~~~
kernel/crash_dump_dm_crypt.c:163:27: note: (near initialization for 'kbuf')
>> kernel/crash_dump_dm_crypt.c:158:26: error: storage size of 'kbuf' isn't known
158 | struct kexec_buf kbuf = {
| ^~~~
>> kernel/crash_dump_dm_crypt.c:187:20: error: 'KEXEC_BUF_MEM_UNKNOWN' undeclared (first use in this function)
187 | kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
| ^~~~~~~~~~~~~~~~~~~~~
kernel/crash_dump_dm_crypt.c:187:20: note: each undeclared identifier is reported only once for each function it appears in
>> kernel/crash_dump_dm_crypt.c:188:13: error: implicit declaration of function 'kexec_add_buffer' [-Werror=implicit-function-declaration]
188 | r = kexec_add_buffer(&kbuf);
| ^~~~~~~~~~~~~~~~
kernel/crash_dump_dm_crypt.c:158:26: warning: unused variable 'kbuf' [-Wunused-variable]
158 | struct kexec_buf kbuf = {
| ^~~~
cc1: some warnings being treated as errors
vim +/kbuf +158 kernel/crash_dump_dm_crypt.c
155
156 int crash_load_dm_crypt_keys(struct kimage *image)
157 {
> 158 struct kexec_buf kbuf = {
> 159 .image = image,
> 160 .buf_min = 0,
> 161 .buf_max = ULONG_MAX,
> 162 .top_down = false,
> 163 .random = true,
164 };
165
166 int r;
167
168 if (state == FRESH)
169 return 0;
170
171 if (key_count != keys_header->key_count) {
172 pr_err("Only record %u keys (%u in total)\n", key_count,
173 keys_header->key_count);
174 return -EINVAL;
175 }
176
177 image->dm_crypt_keys_addr = 0;
178 r = build_keys_header();
179 if (r)
180 return r;
181
182 kbuf.buffer = keys_header;
183 kbuf.bufsz = keys_header_size;
184
185 kbuf.memsz = kbuf.bufsz;
186 kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> 187 kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> 188 r = kexec_add_buffer(&kbuf);
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Greg,
Thanks for taking time to carefully review this patch!
On Thu, May 23, 2024 at 09:21:08AM +0200, Greg KH wrote:
>On Thu, May 23, 2024 at 01:04:43PM +0800, Coiby Xu wrote:
>> 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.
>
>"logon"? What is that?
Thanks for raising this question. A logon key is similar to a user key
but the payload can't be read by user space. I'll document this info and
also refer users to security/keys/core.rst for details.
>
>>
>> User space can also read this API to learn about current state.
>
>But you don't document it in Documentation/ABI/ so we don't know if this
>really is the case, and no one will know how to use it :(
Thanks for bringing this to my attention! A new version will include
Documentation/ABI/testing/crash_dm_crypt_keys to have all the details.
>
>> 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/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>> new file mode 100644
>> index 000000000000..78809189084a
>> --- /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
>
>Why these values?
For KEY_NUM_MAX, I assume the maximum number of LUKS encrypted volumes to
unlock is 128.
For KEY_SIZE_MAX, according to /proc/crypto and "cryptsetup benchmark",
the maximum key size is 64 bytes. So I assume 256 should be enough in
near future.
Thanks for raising the question which makes realize it's better to
include the assumptions in the commit message for domain experts to
examine!
>
>> +
>> +// 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 enum STATE_ENUM {
>> + FRESH = 0,
>> + INITIALIZED,
>> + RECORDED,
>> + LOADED,
>> +} state;
>
>How are you going to keep these enums synced up with the string values?
Thanks to prompt me to come up with a more reliable way! I should have
used the emums to index the array,
static const char *STATE_STR[] = {
[FRESH] = "fresh",
[INITIALIZED] = "initialized",
[RECORDED] = "recorded",
[LOADED] = "loaded"
};
>
>> +
>> +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];
>> +};
>> +
>> +static 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];
>
>Why 5?
Oh, I had len("init")+1(NULL) in mind when wrote 5. In retrospect, 5
is not necessary. And in fact there is no need to define this dummy
variable in the first place as explained in the next comment.
>
>> +
>> + if (sscanf(buf, "%4s %u", dummy, &total_keys) != 2)
>
>Didn't you just overflow dummy now?
Correct me if I'm wrong, but using the width specifier i.e. "%4s" won't
overflow dummy, right? Anyways, I think a better way is to simply use
sscanf(buf, "init %u",..)
instead thus no need for this dummy variable.
>
>> + return -EINVAL;
>> +
>> + if (key_count > KEY_NUM_MAX) {
>> + pr_err("Exceed the maximum number of keys (KEY_NUM_MAX=%u)\n",
>> + KEY_NUM_MAX);
>
>Do not let userspace spam the kernel log directly if it sends it invalid
>data.
Thanks for pointing out my oversight. I'll simply return -EINVAL in next
version.
>
>> + 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");
>
>Again, don't let userspace spam the log.
I'll fix this issue in next version. Thank you again!
>
>> +
>> + if (sscanf(buf, "%6s %s", dummy, key_desc) != 2)
Oh, key_desc could be overflowed here if there is a malicious user. I'll
have a check on the length of buf in next version.
[...]
>> +EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_write);
>
>EXPORT_SYMBOL_GPL() as you are dealing with a sysfs api?
>
>> +int crash_sysfs_dm_crypt_keys_read(char *buf)
>> +{
>> + return sprintf(buf, "%s\n", STATE_STR[state]);
>
>sysfs_emit() please.
This will be fixed, thanks!
>
>> +}
>> +EXPORT_SYMBOL(crash_sysfs_dm_crypt_keys_read);
>
>Again, EXPORT_SYMBOL_GPL()?
I'll replace two occurrences of EXPORT_SYMBOL with EXPORT_SYMBOL_GPL,
thanks!
>
>> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
>> index 07fb5987b42b..2ba4dcbf5816 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;
>
>Personally, I hate ? : lines, just write it out, the compiler is the
>same and this way it is much more readable:
> if (ret < 0)
> return ret;
> return count;
Thanks for suggesting more readable code! I'll apply it to next version!
>
>thanks,
>
>greg k-h
>
>_______________________________________________
>kexec mailing list
>[email protected]
>http://lists.infradead.org/mailman/listinfo/kexec
>
--
Best regards,
Coiby
On 05/23/24 at 01: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.
>
> 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 f0e9f8eda7a3..cc81b8a903ab 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 3d64290d24c9..06b77f9ac4cc 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"
> @@ -437,6 +438,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)
> {
> @@ -445,6 +456,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);
As we discussed before, this need be limited in kdump scope, seems v4
doesn't include the change.
>
> do {
> /* align down start */
> @@ -482,6 +495,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.45.0
>
Hi Coiby,
On 05/23/24 at 01:04pm, Coiby Xu wrote:
> 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.
From the subject, can I think the luks keys will persist forever? or
only for a while? If need and can only keep it for a while, can you
mention it and tell why and how it will be used. Because you add a lot
of codes, but only simply mention the sysfs, that doesn't make sense.
>
> 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 44305336314e..6bff1c24efa3 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, void *arg) { }
> 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..78809189084a
> --- /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"};
> +static 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];
> +};
> +
> +static 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)
~~~~ A more interesting name with more description?
> +{
> + 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 nobody use the count, why do you add it?
> +{
> + 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 07fb5987b42b..2ba4dcbf5816 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.45.0
>
On 05/23/24 at 01: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.
I am confused, what is the new infrastructure? And why this patch can be
dropped if 'the new infrastructure' is extended to all arches.
>
> 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 89fec768fba8..b4dc881cc867 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"};
> static 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;
Is the logic here wrong? Isn't it we return when it's REUSE. If not
REUSE, we need build_keys_header(), then add buffer?
> + }
>
> kbuf.buffer = keys_header;
> kbuf.bufsz = keys_header_size;
> --
> 2.45.0
>
Hi Coiby,
On 05/24/24 at 11:17am, kernel test robot wrote:
> Hi Coiby,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on de7e71ef8bed222dd144d8878091ecb6d5dfd208]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20240523-130727
> base: de7e71ef8bed222dd144d8878091ecb6d5dfd208
> patch link: https://lore.kernel.org/r/20240523050451.788754-4-coxu%40redhat.com
> patch subject: [PATCH v4 3/7] crash_dump: store dm keys in kdump reserved memory
> config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20240524/[email protected]/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240524/[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]/
>
> All errors (new ones prefixed by >>):
Please respond to lkp report sooner, otherwise the reproducer link could
be unavailable.
>
> kernel/crash_dump_dm_crypt.c: In function 'crash_load_dm_crypt_keys':
> >> kernel/crash_dump_dm_crypt.c:158:16: error: variable 'kbuf' has initializer but incomplete type
> 158 | struct kexec_buf kbuf = {
> | ^~~~~~~~~
> >> kernel/crash_dump_dm_crypt.c:159:18: error: 'struct kexec_buf' has no member named 'image'
> 159 | .image = image,
> | ^~~~~
> kernel/crash_dump_dm_crypt.c:159:26: warning: excess elements in struct initializer
> 159 | .image = image,
> | ^~~~~
> kernel/crash_dump_dm_crypt.c:159:26: note: (near initialization for 'kbuf')
> >> kernel/crash_dump_dm_crypt.c:160:18: error: 'struct kexec_buf' has no member named 'buf_min'
> 160 | .buf_min = 0,
> | ^~~~~~~
> kernel/crash_dump_dm_crypt.c:160:28: warning: excess elements in struct initializer
> 160 | .buf_min = 0,
> | ^
> kernel/crash_dump_dm_crypt.c:160:28: note: (near initialization for 'kbuf')
> >> kernel/crash_dump_dm_crypt.c:161:18: error: 'struct kexec_buf' has no member named 'buf_max'
> 161 | .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/arm/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:161:28: note: in expansion of macro 'ULONG_MAX'
> 161 | .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:161:28: note: in expansion of macro 'ULONG_MAX'
> 161 | .buf_max = ULONG_MAX,
> | ^~~~~~~~~
> >> kernel/crash_dump_dm_crypt.c:162:18: error: 'struct kexec_buf' has no member named 'top_down'
> 162 | .top_down = false,
> | ^~~~~~~~
> kernel/crash_dump_dm_crypt.c:162:29: warning: excess elements in struct initializer
> 162 | .top_down = false,
> | ^~~~~
> kernel/crash_dump_dm_crypt.c:162:29: note: (near initialization for 'kbuf')
> >> kernel/crash_dump_dm_crypt.c:163:18: error: 'struct kexec_buf' has no member named 'random'
> 163 | .random = true,
> | ^~~~~~
> kernel/crash_dump_dm_crypt.c:163:27: warning: excess elements in struct initializer
> 163 | .random = true,
> | ^~~~
> kernel/crash_dump_dm_crypt.c:163:27: note: (near initialization for 'kbuf')
> >> kernel/crash_dump_dm_crypt.c:158:26: error: storage size of 'kbuf' isn't known
> 158 | struct kexec_buf kbuf = {
> | ^~~~
> >> kernel/crash_dump_dm_crypt.c:187:20: error: 'KEXEC_BUF_MEM_UNKNOWN' undeclared (first use in this function)
> 187 | kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> | ^~~~~~~~~~~~~~~~~~~~~
> kernel/crash_dump_dm_crypt.c:187:20: note: each undeclared identifier is reported only once for each function it appears in
> >> kernel/crash_dump_dm_crypt.c:188:13: error: implicit declaration of function 'kexec_add_buffer' [-Werror=implicit-function-declaration]
> 188 | r = kexec_add_buffer(&kbuf);
> | ^~~~~~~~~~~~~~~~
> kernel/crash_dump_dm_crypt.c:158:26: warning: unused variable 'kbuf' [-Wunused-variable]
> 158 | struct kexec_buf kbuf = {
> | ^~~~
> cc1: some warnings being treated as errors
>
>
> vim +/kbuf +158 kernel/crash_dump_dm_crypt.c
>
> 155
> 156 int crash_load_dm_crypt_keys(struct kimage *image)
> 157 {
> > 158 struct kexec_buf kbuf = {
> > 159 .image = image,
> > 160 .buf_min = 0,
> > 161 .buf_max = ULONG_MAX,
> > 162 .top_down = false,
> > 163 .random = true,
> 164 };
> 165
> 166 int r;
> 167
> 168 if (state == FRESH)
> 169 return 0;
> 170
> 171 if (key_count != keys_header->key_count) {
> 172 pr_err("Only record %u keys (%u in total)\n", key_count,
> 173 keys_header->key_count);
> 174 return -EINVAL;
> 175 }
> 176
> 177 image->dm_crypt_keys_addr = 0;
> 178 r = build_keys_header();
> 179 if (r)
> 180 return r;
> 181
> 182 kbuf.buffer = keys_header;
> 183 kbuf.bufsz = keys_header_size;
> 184
> 185 kbuf.memsz = kbuf.bufsz;
> 186 kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
> > 187 kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> > 188 r = kexec_add_buffer(&kbuf);
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
On 05/23/24 at 01:04pm, Coiby Xu wrote:
.....
> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> new file mode 100644
> index 000000000000..78809189084a
> --- /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"};
> +static enum STATE_ENUM {
> + FRESH = 0,
> + INITIALIZED,
> + RECORDED,
> + LOADED,
> +} state;
> +
> +static unsigned int key_count;
> +static size_t keys_header_size;
These two global variables seems not so necessary. Please see comment at
below.
> +
> +struct dm_crypt_key {
> + unsigned int key_size;
> + char key_desc[KEY_DESC_LEN];
> + u8 data[KEY_SIZE_MAX];
> +};
> +
> +static struct keys_header {
> + unsigned int key_count;
~~~~~~~~
This is the max number a system have from init();
You can add one field member to record how many key slots have been
used.
> + struct dm_crypt_key keys[] __counted_by(key_count);
> +} *keys_header;
Maybe we can rearrange the keys_header like below, the name may not be
very appropriate though.
static struct keys_header {
unsigned int max_key_slots;
unsigned int used_key_slots;
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);
> +}
I personally don't think get_keys_header_size is so necessary. If we
have to keep it, may be we can remove the global variable
keys_header_size, we can call get_keys_header_size() and use local
variable to record the value instead.
On 05/23/24 at 01:04pm, Coiby Xu wrote:
> 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.
This patch highly lack document to describe what it's doing. For
example, how you will manipulate the /sys/kernel/crash_dm_crypt_keys to
initialize, record the keys, can you give examples how you opeate on
them?
>
> 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 44305336314e..6bff1c24efa3 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, void *arg) { }
> 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
Do we need add dependency on some security features, e.g KEYS?
The current code will enable the CRASH_DM_CRYPT regardless of the
existence of LUKS disk at all.
> + 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..78809189084a
> --- /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"};
> +static 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];
> +};
> +
> +static 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;
This is what I wondered and tried to find a document to get why. Can we
search in the current system and deduce how many keys we can could use
for kdump kernel? Or we have to retrieve and pass it from user space?
> +
> + if (key_count > KEY_NUM_MAX) {
> + pr_err("Exceed the maximum number of keys (KEY_NUM_MAX=%u)\n",
> + KEY_NUM_MAX);
> + return -EINVAL;
> + }
Why chekcing key_count in init()? Don't you need to check
total_keys instead? Clearly you don't do a boundary test for total_keys,
otherwise it will trigger issue.
> +
> + 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;
> +}
Please add more code comments, kernel-doc for your code, we can't assume
people reading these codes know the entire matter.
On 05/23/24 at 01:04pm, Coiby Xu wrote:
......
> +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);
Do we need create a x86 specific version to cope with the confidential
computing thing, e.g sme/tdx?
> +}
> +
On 05/23/24 at 01: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);
Isn't crashkernel region covering crypt keys' storing region? Do we need
mark it again specifically? Not sure if I miss anything.
> }
>
> void arch_kexec_unprotect_crashkres(void)
> {
> + kexec_mark_dm_crypt_keys(false);
> kexec_mark_crashkres(false);
> }
> #endif
> --
> 2.45.0
>
On 05/23/24 at 01:04pm, 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 <[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 f06501445cd9..74b3844ae53c 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;
~?
r is only to contain the returned value? Then you can call it ret as
many do in kernel code.
>
> 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);
> + }
You need adjust the array length of cmem->ranges[], I believe you will
cause the array overflow because the keys are randomly set and mostly
will be in the middle of crashkernel region.
> +
> + return r;
> }
>
> /* Prepare memory map for crash dump kernel */
Hi Coiby,
On 05/23/24 at 01: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,
I am done with this round of reviewing. The overall approach looks good
to me, while there are places to improve or fix. I have added comment on
all things I am concerned about, please check. Thanks for the effort.
By the way, do you get confirmation on the solution from encryption/keys
developer of redhat internally or upstream? With my understanding, it
looks good. It may need their confirmation or approval in some ways.
Thanks
Baoquan
On Wed, Jun 05, 2024 at 04:22:12PM +0800, Baoquan He wrote:
>On 05/23/24 at 01:04pm, Coiby Xu wrote:
>.....
>> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>> new file mode 100644
>> index 000000000000..78809189084a
>> --- /dev/null
>> +++ b/kernel/crash_dump_dm_crypt.c
>> @@ -0,0 +1,113 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
[...]
>> +
>> +static unsigned int key_count;
>> +static size_t keys_header_size;
>
>These two global variables seems not so necessary. Please see comment at
>below.
Thanks for the comment! But I think it's better to keep these two static
variables for reasons as will be explained later.
>
>> +
>> +struct dm_crypt_key {
>> + unsigned int key_size;
>> + char key_desc[KEY_DESC_LEN];
>> + u8 data[KEY_SIZE_MAX];
>> +};
>> +
>> +static struct keys_header {
>> + unsigned int key_count;
> ~~~~~~~~
> This is the max number a system have from init();
>You can add one field member to record how many key slots have been
>used.
>> + struct dm_crypt_key keys[] __counted_by(key_count);
>> +} *keys_header;
>
>Maybe we can rearrange the keys_header like below, the name may not be
>very appropriate though.
>
>static struct keys_header {
> unsigned int max_key_slots;
> unsigned int used_key_slots;
> struct dm_crypt_key keys[] __counted_by(key_count);
>} *keys_header;
Thanks for the suggestion! Since 1) KEY_NUM_MAX already defines the
maximum number of dm crypt keys 2) we only need to let the kdump kernel
now how many keys are saved, so I simply use total_keys instead of
key_count in struct keys_header in v5,
static struct keys_header {
unsigned int total_keys;
struct dm_crypt_key keys[] __counted_by(total_keys);
} *keys_header;
Hopefully this renaming will improve code readability.
>
>>
>
>> +
>> +static size_t get_keys_header_size(struct keys_header *keys_header,
>> + size_t key_count)
>> +{
>> + return struct_size(keys_header, keys, key_count);
>> +}
>
>I personally don't think get_keys_header_size is so necessary. If we
>have to keep it, may be we can remove the global variable
>keys_header_size, we can call get_keys_header_size() and use local
>variable to record the value instead.
Thanks for the suggestion! But the kdump kernel also need to call
get_keys_header_size in later patches.
--
Best regards,
Coiby
On Fri, Jun 07, 2024 at 05:50:15PM +0800, Baoquan He wrote:
>On 05/23/24 at 01:04pm, Coiby Xu wrote:
>......
>> +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);
>Do we need create a x86 specific version to cope with the confidential
>computing thing, e.g sme/tdx?
Thanks for raising the concern! I'll test sme/tdx and will fix it in v6
if any issue is found.
--
Best regards,
Coiby
On Thu, Jun 06, 2024 at 11:11:30AM +0800, Baoquan He wrote:
>On 05/23/24 at 01:04pm, Coiby Xu wrote:
>> 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.
>
>This patch highly lack document to describe what it's doing. For
>example, how you will manipulate the /sys/kernel/crash_dm_crypt_keys to
>initialize, record the keys, can you give examples how you opeate on
>them?
Thanks for the suggestion! v5 now includes
Documentation/ABI/testing/crash_dm_crypt_keys.
>
>>
>> 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 44305336314e..6bff1c24efa3 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, void *arg) { }
>> 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
>
>Do we need add dependency on some security features, e.g KEYS?
>The current code will enable the CRASH_DM_CRYPT regardless of the
>existence of LUKS disk at all.
Good suggestion! I make CRASH_DM_CRYPT depend on DM_CRYPT in v5.
> [...]
>> +static int init(const char *buf)
>> +{
>> + unsigned int total_keys;
>> + char dummy[5];
>> +
>> + if (sscanf(buf, "%4s %u", dummy, &total_keys) != 2)
>> + return -EINVAL;
>
>This is what I wondered and tried to find a document to get why. Can we
>search in the current system and deduce how many keys we can could use
>for kdump kernel? Or we have to retrieve and pass it from user space?
I don't think we can get this info from the kernel space directly
because the kernel don't know the kdump target. So we have to pass this
info from user space.
>
>> +
>> + if (key_count > KEY_NUM_MAX) {
>> + pr_err("Exceed the maximum number of keys (KEY_NUM_MAX=%u)\n",
>> + KEY_NUM_MAX);
>> + return -EINVAL;
>> + }
>
>Why chekcing key_count in init()? Don't you need to check
>total_keys instead? Clearly you don't do a boundary test for total_keys,
>otherwise it will trigger issue.
Good catch! Yes, I should check total_keys instead.
>
>> +
>> + 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;
>> +}
>
>Please add more code comments, kernel-doc for your code, we can't assume
>people reading these codes know the entire matter.
Thanks for the suggestion! I've added some comments and documentation in
v5. Please let me know if there is anything more to improve.
--
Best regards,
Coiby
Hi Baoquan,
On Fri, Jun 07, 2024 at 06:06:18PM +0800, Baoquan He wrote:
>Hi Coiby,
>
>On 05/23/24 at 01: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,
>
>I am done with this round of reviewing. The overall approach looks good
>to me, while there are places to improve or fix. I have added comment on
>all things I am concerned about, please check. Thanks for the effort.
Thanks for taking time on thoroughly reviewing the patches and
suggesting various improvements! I believe all comments have been
addressed now except for testing sme/tdx:)
>
>By the way, do you get confirmation on the solution from encryption/keys
>developer of redhat internally or upstream? With my understanding, it
>looks good. It may need their confirmation or approval in some ways.
Yes, actually cryptsetup's maintainer Milan pointed me to
libcryptsetup's new API and suggested me to reach out to Ondrej for
help. And Ondrej has already finished the major work in upstream
cryptsetup and systemd. But it will be good for them to have further
confirmation and I'll ask for their help. Thanks for the reminder!
>
>Thanks
>Baoquan
>
--
Best regards,
Coiby
On Tue, Jun 04, 2024 at 04:51:03PM +0800, Baoquan He wrote:
>Hi Coiby,
Hi Baoquan,
>
>On 05/23/24 at 01:04pm, Coiby Xu wrote:
>> 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.
>
>From the subject, can I think the luks keys will persist forever? or
>only for a while?
Yes, you are right. The keys need to stay in kdump reserved memory.
> If need and can only keep it for a while, can you
>mention it and tell why and how it will be used. Because you add a lot
>of codes, but only simply mention the sysfs, that doesn't make sense.
Thanks for raising the concern! I've added
Documentation/ABI/testing/crash_dm_crypt_keys and copy some text in the
cover letter to this patch in v5.
>
>>
>> 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 44305336314e..6bff1c24efa3 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
[...]
>> +static int init(const char *buf)
> ~~~~ A more interesting name with more description?
Thanks for the suggestion! I've added some comments for this function
in v5. But I can't come up with a better name after looking at current
kernel code. You are welcome to suggest any better name:)
>> +static int process_cmd(const char *buf, size_t count)
> ~~~~
>If nobody use the count, why do you add it?
Good catch! Yes, this is no need to use count in v4. But v5 now needs it to avoid
buffer overflow.
--
Best regards,
Coiby
On Tue, Jun 04, 2024 at 09:54:26PM +0800, Baoquan He wrote:
>Hi Coiby,
Hi Baoquan,
>
>On 05/24/24 at 11:17am, kernel test robot wrote:
>> Hi Coiby,
>>
>> kernel test robot noticed the following build errors:
>>
>> [auto build test ERROR on de7e71ef8bed222dd144d8878091ecb6d5dfd208]
>>
>> url: https://github.com/intel-lab-lkp/linux/commits/Coiby-Xu/kexec_file-allow-to-place-kexec_buf-randomly/20240523-130727
>> base: de7e71ef8bed222dd144d8878091ecb6d5dfd208
>> patch link: https://lore.kernel.org/r/20240523050451.788754-4-coxu%40redhat.com
>> patch subject: [PATCH v4 3/7] crash_dump: store dm keys in kdump reserved memory
>> config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20240524/[email protected]/config)
>> compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240524/[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]/
>>
>> All errors (new ones prefixed by >>):
>
>Please respond to lkp report sooner, otherwise the reproducer link could
>be unavailable.
Thanks for the reminder! I believe these errors are false positives
because 1) include/linux/kexec.h is included after "#include <linux/crash_dump.h>"
and 2) similar errors are seen for kernel/kexec_file.c by
"make ARCH=arm CROSS_COMPILE=arm-linux-gnu- kernel/kexec_file.o"
>
>>
>> kernel/crash_dump_dm_crypt.c: In function 'crash_load_dm_crypt_keys':
>> >> kernel/crash_dump_dm_crypt.c:158:16: error: variable 'kbuf' has initializer but incomplete type
>> 158 | struct kexec_buf kbuf = {
>> | ^~~~~~~~~
>> >> kernel/crash_dump_dm_crypt.c:159:18: error: 'struct kexec_buf' has no member named 'image'
>> 159 | .image = image,
>> | ^~~~~
>> kernel/crash_dump_dm_crypt.c:159:26: warning: excess elements in struct initializer
>> 159 | .image = image,
>> | ^~~~~
>> kernel/crash_dump_dm_crypt.c:159:26: note: (near initialization for 'kbuf')
>> >> kernel/crash_dump_dm_crypt.c:160:18: error: 'struct kexec_buf' has no member named 'buf_min'
>> 160 | .buf_min = 0,
>> | ^~~~~~~
>> kernel/crash_dump_dm_crypt.c:160:28: warning: excess elements in struct initializer
>> 160 | .buf_min = 0,
>> | ^
>> kernel/crash_dump_dm_crypt.c:160:28: note: (near initialization for 'kbuf')
>> >> kernel/crash_dump_dm_crypt.c:161:18: error: 'struct kexec_buf' has no member named 'buf_max'
>> 161 | .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/arm/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:161:28: note: in expansion of macro 'ULONG_MAX'
>> 161 | .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:161:28: note: in expansion of macro 'ULONG_MAX'
>> 161 | .buf_max = ULONG_MAX,
>> | ^~~~~~~~~
>> >> kernel/crash_dump_dm_crypt.c:162:18: error: 'struct kexec_buf' has no member named 'top_down'
>> 162 | .top_down = false,
>> | ^~~~~~~~
>> kernel/crash_dump_dm_crypt.c:162:29: warning: excess elements in struct initializer
>> 162 | .top_down = false,
>> | ^~~~~
>> kernel/crash_dump_dm_crypt.c:162:29: note: (near initialization for 'kbuf')
>> >> kernel/crash_dump_dm_crypt.c:163:18: error: 'struct kexec_buf' has no member named 'random'
>> 163 | .random = true,
>> | ^~~~~~
>> kernel/crash_dump_dm_crypt.c:163:27: warning: excess elements in struct initializer
>> 163 | .random = true,
>> | ^~~~
>> kernel/crash_dump_dm_crypt.c:163:27: note: (near initialization for 'kbuf')
>> >> kernel/crash_dump_dm_crypt.c:158:26: error: storage size of 'kbuf' isn't known
>> 158 | struct kexec_buf kbuf = {
>> | ^~~~
>> >> kernel/crash_dump_dm_crypt.c:187:20: error: 'KEXEC_BUF_MEM_UNKNOWN' undeclared (first use in this function)
>> 187 | kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> | ^~~~~~~~~~~~~~~~~~~~~
>> kernel/crash_dump_dm_crypt.c:187:20: note: each undeclared identifier is reported only once for each function it appears in
>> >> kernel/crash_dump_dm_crypt.c:188:13: error: implicit declaration of function 'kexec_add_buffer' [-Werror=implicit-function-declaration]
>> 188 | r = kexec_add_buffer(&kbuf);
>> | ^~~~~~~~~~~~~~~~
>> kernel/crash_dump_dm_crypt.c:158:26: warning: unused variable 'kbuf' [-Wunused-variable]
>> 158 | struct kexec_buf kbuf = {
>> | ^~~~
>> cc1: some warnings being treated as errors
>>
>>
>> vim +/kbuf +158 kernel/crash_dump_dm_crypt.c
>>
>> 155
>> 156 int crash_load_dm_crypt_keys(struct kimage *image)
>> 157 {
>> > 158 struct kexec_buf kbuf = {
>> > 159 .image = image,
>> > 160 .buf_min = 0,
>> > 161 .buf_max = ULONG_MAX,
>> > 162 .top_down = false,
>> > 163 .random = true,
>> 164 };
>> 165
>> 166 int r;
>> 167
>> 168 if (state == FRESH)
>> 169 return 0;
>> 170
>> 171 if (key_count != keys_header->key_count) {
>> 172 pr_err("Only record %u keys (%u in total)\n", key_count,
>> 173 keys_header->key_count);
>> 174 return -EINVAL;
>> 175 }
>> 176
>> 177 image->dm_crypt_keys_addr = 0;
>> 178 r = build_keys_header();
>> 179 if (r)
>> 180 return r;
>> 181
>> 182 kbuf.buffer = keys_header;
>> 183 kbuf.bufsz = keys_header_size;
>> 184
>> 185 kbuf.memsz = kbuf.bufsz;
>> 186 kbuf.buf_align = ELF_CORE_HEADER_ALIGN;
>> > 187 kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> > 188 r = kexec_add_buffer(&kbuf);
>>
>> --
>> 0-DAY CI Kernel Test Service
>> https://github.com/intel/lkp-tests/wiki
>>
>
--
Best regards,
Coiby
On Tue, Jun 04, 2024 at 03:41:35PM +0800, Baoquan He wrote:
>On 05/23/24 at 01: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.
>>
>> 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 f0e9f8eda7a3..cc81b8a903ab 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
[...]
>> @@ -445,6 +456,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);
>
>As we discussed before, this need be limited in kdump scope, seems v4
>doesn't include the change.
Yes, v4 was sent mainly for you to review the patches by applying to
latest Linus tree. v5 now include the change.
--
Best regards,
Coiby
On Fri, Jun 07, 2024 at 05:57:38PM +0800, Baoquan He wrote:
>On 05/23/24 at 01:04pm, 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 <[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 f06501445cd9..74b3844ae53c 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;
> ~?
>
>r is only to contain the returned value? Then you can call it ret as
>many do in kernel code.
Applied to v6, thanks for the suggestion!
>
>>
>> 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);
>> + }
>
>You need adjust the array length of cmem->ranges[], I believe you will
>cause the array overflow because the keys are randomly set and mostly
>will be in the middle of crashkernel region.
Yes, you are absolutely right. I've fixed in v6.
--
Best regards,
Coiby
On Fri, Jun 07, 2024 at 06:00:44PM +0800, Baoquan He wrote:
>On 05/23/24 at 01: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);
>
>Isn't crashkernel region covering crypt keys' storing region? Do we need
>mark it again specifically? Not sure if I miss anything.
kexec_mark_crashkres only makes the page read-only whereas
kexec_mark_dm_crypt_keys makes the memory inaccessible. I've added a
comment for this function in v5 and hopefully no one will be confused by
it.
--
Best regards,
Coiby
On 06/07/24 at 08:27pm, Coiby Xu wrote:
> On Wed, Jun 05, 2024 at 04:22:12PM +0800, Baoquan He wrote:
> > On 05/23/24 at 01:04pm, Coiby Xu wrote:
> > .....
> > > diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
> > > new file mode 100644
> > > index 000000000000..78809189084a
> > > --- /dev/null
> > > +++ b/kernel/crash_dump_dm_crypt.c
> > > @@ -0,0 +1,113 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> [...]
> > > +
> > > +static unsigned int key_count;
> > > +static size_t keys_header_size;
> >
> > These two global variables seems not so necessary. Please see comment at
> > below.
>
> Thanks for the comment! But I think it's better to keep these two static
> variables for reasons as will be explained later.
>
> >
> > > +
> > > +struct dm_crypt_key {
> > > + unsigned int key_size;
> > > + char key_desc[KEY_DESC_LEN];
> > > + u8 data[KEY_SIZE_MAX];
> > > +};
> > > +
> > > +static struct keys_header {
> > > + unsigned int key_count;
> > ~~~~~~~~
> > This is the max number a system have from init();
> > You can add one field member to record how many key slots have been
> > used.
> > > + struct dm_crypt_key keys[] __counted_by(key_count);
> > > +} *keys_header;
> >
> > Maybe we can rearrange the keys_header like below, the name may not be
> > very appropriate though.
> >
> > static struct keys_header {
> > unsigned int max_key_slots;
> > unsigned int used_key_slots;
> > struct dm_crypt_key keys[] __counted_by(key_count);
> > } *keys_header;
>
> Thanks for the suggestion! Since 1) KEY_NUM_MAX already defines the
> maximum number of dm crypt keys 2) we only need to let the kdump kernel
> now how many keys are saved, so I simply use total_keys instead of
> key_count in struct keys_header in v5,
>
> static struct keys_header {
> unsigned int total_keys;
> struct dm_crypt_key keys[] __counted_by(total_keys);
> } *keys_header;
>
> Hopefully this renaming will improve code readability.
If you add key_count into keys_header, then kdump kernel will know how
many keys are really saved and need be retrieved. What's your concern
when you have to put key_count outside and take it as a global variable?
>
> >
> > >
> >
> > > +
> > > +static size_t get_keys_header_size(struct keys_header *keys_header,
> > > + size_t key_count)
> > > +{
> > > + return struct_size(keys_header, keys, key_count);
> > > +}
> >
> > I personally don't think get_keys_header_size is so necessary. If we
> > have to keep it, may be we can remove the global variable
> > keys_header_size, we can call get_keys_header_size() and use local
> > variable to record the value instead.
>
> Thanks for the suggestion! But the kdump kernel also need to call
> get_keys_header_size in later patches.
If so, you can remove keys_header_size now. You can define local
variable to take the newly calculated value. keys_header_size seems not
so necesary.
By the way, you don't need to rush to post v5. When people review
patches, agreement need be reached after discussion. Then next version
can be posted.
On 06/07/24 at 08:27pm, Coiby Xu wrote:
> On Tue, Jun 04, 2024 at 04:51:03PM +0800, Baoquan He wrote:
> > Hi Coiby,
>
> Hi Baoquan,
>
> >
> > On 05/23/24 at 01:04pm, Coiby Xu wrote:
> > > 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.
> >
> > From the subject, can I think the luks keys will persist forever? or
> > only for a while?
>
> Yes, you are right. The keys need to stay in kdump reserved memory.
Hmm, there are two different concepts we may need differentiate. From
security keys's point of view, the keys need be stored for a while so
that kdump loading take action to get it, that's done through sysfs;
Froom kdump's point of view, the keys need be stored forever till kdump
kernel use it. I can't see what you are referring to from the subject,
esepcially you stress the newly added sysfs
/sys/kernel/crash_dm_crypt_keys.
>
> > If need and can only keep it for a while, can you
> > mention it and tell why and how it will be used. Because you add a lot
> > of codes, but only simply mention the sysfs, that doesn't make sense.
>
> Thanks for raising the concern! I've added
> Documentation/ABI/testing/crash_dm_crypt_keys and copy some text in the
> cover letter to this patch in v5.
>
> >
> > >
> > > 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 44305336314e..6bff1c24efa3 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
> [...]
> > > +static int init(const char *buf)
> > ~~~~ A more interesting name with more description?
>
> Thanks for the suggestion! I've added some comments for this function
> in v5. But I can't come up with a better name after looking at current
> kernel code. You are welcome to suggest any better name:)
Usually init() is for the whole driver module. Your init() here only
receive the passed total keys number, and allocate the key_header, how
can you simply name it init()? If you call it init_keys_header(), I
would think it's much more meaningful.
>
> > > +static int process_cmd(const char *buf, size_t count)
> > ~~~~
> > If nobody use the count, why do you add it?
>
> Good catch! Yes, this is no need to use count in v4. But v5 now needs it to avoid
> buffer overflow.
OK, did you add code comment telling what 'count' stands for?
And the name 'process_cmd()' is also ambiguous. We may need avoid this
kind of name, e.g process_cmd, do_things, handle_stuff. Can you add a
more specific name?