Date: Sat, 25 May 2024 15:57:20 +0800
From: Coiby Xu <coxu@...hat.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: kexec@...ts.infradead.org, Ondrej Kozina <okozina@...hat.com>, 
	Milan Broz <gmazyland@...il.com>, Thomas Staudt <tstaudt@...ibm.com>, 
	Daniel P . Berrangé <berrange@...hat.com>, Kairui Song <ryncsn@...il.com>, 
	Jan Pazdziora <jpazdziora@...hat.com>, Pingfan Liu <kernelfans@...il.com>, Baoquan He <bhe@...hat.com>, 
	Dave Young <dyoung@...hat.com>, linux-kernel@...r.kernel.org, x86@...nel.org, 
	Dave Hansen <dave.hansen@...el.com>, Vitaly Kuznetsov <vkuznets@...hat.com>, 
	Vivek Goyal <vgoyal@...hat.com>, Kees Cook <keescook@...omium.org>, 
	"Gustavo A. R. Silva" <gustavoars@...nel.org>, 
	"open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb" <linux-hardening@...r.kernel.org>
Subject: Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the
 kdump kernel

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@...hat.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@...ts.infradead.org
>http://lists.infradead.org/mailman/listinfo/kexec
>

-- 
Best regards,
Coiby

Powered by blists - more mailing lists