linux-kernel - Re: [PATCH v2 06/16] x86/efi: Generating random HMAC key for siging hibernate image

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 9 Sep 2015 13:15:45 +0100
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	joeyli <jlee@...e.com>
Cc:	"Lee, Chun-Yi" <joeyli.kernel@...il.com>,
	linux-kernel@...r.kernel.org, linux-efi@...r.kernel.org,
	linux-pm@...r.kernel.org, "Rafael J. Wysocki" <rjw@...ysocki.net>,
	Matthew Garrett <matthew.garrett@...ula.com>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	Josh Boyer <jwboyer@...hat.com>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Matt Fleming <matt.fleming@...el.com>,
	Jiri Kosina <jkosina@...e.cz>,
	"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v2 06/16] x86/efi: Generating random HMAC key for siging
 hibernate image

On Thu, 27 Aug, at 05:04:52PM, joeyli wrote:
> 
> The purpose of checking attribute of hibernation key variable is 
> in case someone created a key variable on runtime environment _before_
> this kernel create boot service variable. That causes EFI stub may load
> a key that from non-secure environment.
> 
> That's why delete non-boot service variable and create new one here.
 
I think bailing is more appropriate in that case, not deleting the
variable. The environment is not what we expected it to be, so we
shouldn't tamper with it.

But I don't feel super strongly about this point. I just wanted to
raise the question of whether it actually makes sense to delete the
variable that we obviously didn't create or whether it makes more
sense to refuse to verify hibernation images.

> > > diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
> > > index 2fab6c2..ab463c4 100644
> > > --- a/arch/x86/include/asm/suspend.h
> > > +++ b/arch/x86/include/asm/suspend.h
> > > @@ -3,3 +3,12 @@
> > >  #else
> > >  # include <asm/suspend_64.h>
> > >  #endif
> > > +
> > > +#ifdef CONFIG_HIBERNATE_VERIFICATION
> > > +#include <linux/suspend.h>
> > > +
> > > +struct hibernation_keys {
> > > +	unsigned long hkey_status;
> > > +	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
> > > +};
> > > +#endif
> > 
> > Have you given any thought to how things are going to work if we
> > change the hash function in the future, or provide a choice? That
> > information doesn't appear anywhere in the above struct.
> > 
> 
> Do you mean the hash function of signing hibernation image changed, so the
> hibernation key also need to re-generate for new length?
 
Yeah, that kind of thing.

> I will add a field in struct to forward the length of hibernation key variable.
> In the future kernel can check the length to handle the change.

Would it also make sense to explicitly record the hash function used
as well as the length?

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/