Message-ID: <enehvyzptsb2daxnsybmkv5byeqrebgcgwpz3tlmqwpwfaxtmm@ca57dtcsmyna>
Date: Fri, 18 Oct 2024 09:44:37 +0800
From: Coiby Xu <coxu@...hat.com>
To: Baoquan He <bhe@...hat.com>
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>, 
	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

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@...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/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

Powered by blists - more mailing lists