Received-SPF: pass (google.com: domain of linux-kernel+bounces-206042-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 7 Jun 2024 20:26:54 +0800
From: Coiby Xu <coxu@redhat.com>
To: Baoquan He <bhe@redhat.com>
Cc: kexec@lists.infradead.org, Ondrej Kozina <okozina@redhat.com>, 
	Milan Broz <gmazyland@gmail.com>, Thomas Staudt <tstaudt@de.ibm.com>, 
	Daniel P =?utf-8?B?LiBCZXJyYW5nw6k=?= <berrange@redhat.com>, Kairui Song <ryncsn@gmail.com>, 
	Jan Pazdziora <jpazdziora@redhat.com>, Pingfan Liu <kernelfans@gmail.com>, 
	Dave Young <dyoung@redhat.com>, linux-kernel@vger.kernel.org, x86@kernel.org, 
	Dave Hansen <dave.hansen@intel.com>, Vitaly Kuznetsov <vkuznets@redhat.com>, 
	Vivek Goyal <vgoyal@redhat.com>, Kees Cook <keescook@chromium.org>, 
	"Gustavo A. R. Silva" <gustavoars@kernel.org>, 
	"open list:KERNEL HARDENING (not covered by other areas):Keyword:b__counted_byb" <linux-hardening@vger.kernel.org>
Subject: Re: [PATCH v4 2/7] crash_dump: make dm crypt keys persist for the
 kdump kernel
Message-ID: <y5ah5hdueqh5l5m4ll6qdebvnkvkdfbghvjprgyfeacajo3nhj@zg7qj6vfo3w6>
References: <20240523050451.788754-1-coxu@redhat.com>
 <20240523050451.788754-3-coxu@redhat.com>
 <ZmEo4rw8Yhu7tspb@MiWiFi-R3L-srv>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <ZmEo4rw8Yhu7tspb@MiWiFi-R3L-srv>

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