linux-kernel - Re: [PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z2zR9ND4u9j5QMBU@MiWiFi-R3L-srv>
Date: Thu, 26 Dec 2024 11:48:04 +0800
From: Baoquan He <bhe@...hat.com>
To: Coiby Xu <coxu@...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>,
	Greg KH <gregkh@...uxfoundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

On 12/23/24 at 09:16am, Coiby Xu wrote:
> On Wed, Dec 11, 2024 at 08:55:52PM +0800, Baoquan He wrote:
> > On 10/29/24 at 01:52pm, Coiby Xu wrote:
...... 
> > > 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");
> > 
> > If it's error, do we need change ti to pr_debug?
> 
> Thanks for raising the concern! I think it's OK to let
> crash_load_dm_crypt_keys fail since disk encryption may not be used for
> kdump, thus pr_debug is sufficient. Or have I misunderstood your comment?

If crash_load_dm_crypt_keys() returned error, shouldn't we handle them
separately? If disk encryption is not used, we return a specific value
to indicate that, surely no need to pr_err(). However, if disk
encryption is used for kdump but error is caused and failed
crash_load_dm_crypt_keys(), why don't we error out for the case?

Maybe below change can be made to differentiate them?

diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
index f72a88b7d106..327f84ea57c5 100644
--- a/kernel/crash_dump_dm_crypt.c
+++ b/kernel/crash_dump_dm_crypt.c
@@ -400,7 +400,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
 
        if (key_count <= 0) {
                kexec_dprintk("No dm-crypt keys\n");
-               return -EINVAL;
+               return -ENOENT;
        }