* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-05-23 5:04 ` [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel Coiby Xu
@ 2024-05-23 7:21 ` Greg KH
2024-05-25 7:57 ` Coiby Xu
2024-06-04 8:51 ` Baoquan He
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2024-05-23 7:21 UTC (permalink / raw)
To: Coiby Xu
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Baoquan He, Dave Young, linux-kernel, x86, Dave Hansen,
Vitaly Kuznetsov, Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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 <coxu@redhat.com>
> ---
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-05-23 7:21 ` Greg KH
@ 2024-05-25 7:57 ` Coiby Xu
0 siblings, 0 replies; 13+ messages in thread
From: Coiby Xu @ 2024-05-25 7:57 UTC (permalink / raw)
To: Greg KH
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Baoquan He, Dave Young, linux-kernel, x86, Dave Hansen,
Vitaly Kuznetsov, Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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 <coxu@redhat.com>
>> ---
>> 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
>kexec@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/kexec
>
--
Best regards,
Coiby
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-05-23 5:04 ` [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel Coiby Xu
2024-05-23 7:21 ` Greg KH
@ 2024-06-04 8:51 ` Baoquan He
2024-06-07 12:27 ` Coiby Xu
2024-06-05 8:22 ` Baoquan He
2024-06-06 3:11 ` Baoquan He
3 siblings, 1 reply; 13+ messages in thread
From: Baoquan He @ 2024-06-04 8:51 UTC (permalink / raw)
To: Coiby Xu
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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 <coxu@redhat.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-06-04 8:51 ` Baoquan He
@ 2024-06-07 12:27 ` Coiby Xu
2024-06-10 2:00 ` Baoquan He
0 siblings, 1 reply; 13+ messages in thread
From: Coiby Xu @ 2024-06-07 12:27 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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 <coxu@redhat.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-06-07 12:27 ` Coiby Xu
@ 2024-06-10 2:00 ` Baoquan He
2024-10-18 1:44 ` Coiby Xu
0 siblings, 1 reply; 13+ messages in thread
From: Baoquan He @ 2024-06-10 2:00 UTC (permalink / raw)
To: Coiby Xu
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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 <coxu@redhat.com>
> > > ---
> > > 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?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-06-10 2:00 ` Baoquan He
@ 2024-10-18 1:44 ` Coiby Xu
0 siblings, 0 replies; 13+ messages in thread
From: Coiby Xu @ 2024-10-18 1:44 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
On Mon, Jun 10, 2024 at 10:00:05AM +0800, Baoquan He wrote:
>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.
Thanks for the suggestion! The patch set's goal is make the kdump kernel
to be able to unlock encrypted volumes so the keys have always to be
there to be ready for the kdump kernel. In fact, the 1st kernel also
keeps the keys in runtime memory because I/O data could be written back
to disk anytime and thus needs to be encrypted with the keys anytime.
But keys are sensitive data so we try to make it as safe as possible and
one measure for example to make it stay in the keyring for relatively
short time. I'll add a note in next version's commit message. Together
with the info of the life cycle of keys, hopefully it will bring some
clarities
>
>>
>> > 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 <coxu@redhat.com>
>> > > ---
>> > > 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.
Good suggestion! Yes, init_keys_header would be a much better choice!
Unfortunately a new version will switch to configfs. So I will keep this
lesson in mind and try to apply it to similar cases in the future.
>
>>
>> > > +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?
As new version will switch to configfs, process_cmd will no longer be
needed. But I'll try coming up with better names in the future. Thanks!
--
Best regards,
Coiby
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-05-23 5:04 ` [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel Coiby Xu
2024-05-23 7:21 ` Greg KH
2024-06-04 8:51 ` Baoquan He
@ 2024-06-05 8:22 ` Baoquan He
2024-06-07 12:27 ` Coiby Xu
2024-06-06 3:11 ` Baoquan He
3 siblings, 1 reply; 13+ messages in thread
From: Baoquan He @ 2024-06-05 8:22 UTC (permalink / raw)
To: Coiby Xu
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-06-05 8:22 ` Baoquan He
@ 2024-06-07 12:27 ` Coiby Xu
2024-06-10 1:18 ` Baoquan He
0 siblings, 1 reply; 13+ messages in thread
From: Coiby Xu @ 2024-06-07 12:27 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-06-07 12:27 ` Coiby Xu
@ 2024-06-10 1:18 ` Baoquan He
2024-10-18 1:02 ` Coiby Xu
0 siblings, 1 reply; 13+ messages in thread
From: Baoquan He @ 2024-06-10 1:18 UTC (permalink / raw)
To: Coiby Xu
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-06-10 1:18 ` Baoquan He
@ 2024-10-18 1:02 ` Coiby Xu
0 siblings, 0 replies; 13+ messages in thread
From: Coiby Xu @ 2024-10-18 1:02 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
On Mon, Jun 10, 2024 at 09:18:53AM +0800, Baoquan He wrote:
>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?
Yes, the kdump kernel can know how many keys by reading keys_header. But
1st kernel needs to know how many keys are recorded so far as user space
is expected to tell the kernel which keys are needed one by one and the
key_count will be increased witch each new key.
>
>>
>> >
>> > >
>> >
>> > > +
>> > > +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.
Sure, new version will have the global variable keys_header_size
removed which can make the code a bit easier to reason about, thanks!
>
>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.
Thanks for the suggestion! I'll wait for the consensus to be truly
reached next time.
And sorry, it takes longer time than I have expected to evaluate all the
feedback. In retrospective, maybe it's better to respond to each
feedback in a parallel manner than pondering over all the feedback
first.
--
Best regards,
Coiby
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-05-23 5:04 ` [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel Coiby Xu
` (2 preceding siblings ...)
2024-06-05 8:22 ` Baoquan He
@ 2024-06-06 3:11 ` Baoquan He
2024-06-07 12:26 ` Coiby Xu
3 siblings, 1 reply; 13+ messages in thread
From: Baoquan He @ 2024-06-06 3:11 UTC (permalink / raw)
To: Coiby Xu
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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 <coxu@redhat.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel
2024-06-06 3:11 ` Baoquan He
@ 2024-06-07 12:26 ` Coiby Xu
0 siblings, 0 replies; 13+ messages in thread
From: Coiby Xu @ 2024-06-07 12:26 UTC (permalink / raw)
To: Baoquan He
Cc: kexec, Ondrej Kozina, Milan Broz, Thomas Staudt,
Daniel P . Berrangé, Kairui Song, Jan Pazdziora, Pingfan Liu,
Dave Young, linux-kernel, x86, Dave Hansen, Vitaly Kuznetsov,
Vivek Goyal, Kees Cook, Gustavo A. R. Silva,
open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb
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 <coxu@redhat.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread