From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753191AbbIIMP5 (ORCPT ); Wed, 9 Sep 2015 08:15:57 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:35978 "EHLO mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751783AbbIIMPs (ORCPT ); Wed, 9 Sep 2015 08:15:48 -0400 Date: Wed, 9 Sep 2015 13:15:45 +0100 From: Matt Fleming To: joeyli Cc: "Lee, Chun-Yi" , linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org, linux-pm@vger.kernel.org, "Rafael J. Wysocki" , Matthew Garrett , Len Brown , Pavel Machek , Josh Boyer , Vojtech Pavlik , Matt Fleming , Jiri Kosina , "H. Peter Anvin" , Ingo Molnar Subject: Re: [PATCH v2 06/16] x86/efi: Generating random HMAC key for siging hibernate image Message-ID: <20150909121545.GD4973@codeblueprint.co.uk> References: <1439273796-25359-1-git-send-email-jlee@suse.com> <1439273796-25359-7-git-send-email-jlee@suse.com> <20150820204044.GG2567@codeblueprint.co.uk> <20150827090452.GB27415@linux-rxt1.site> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150827090452.GB27415@linux-rxt1.site> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > #endif > > > + > > > +#ifdef CONFIG_HIBERNATE_VERIFICATION > > > +#include > > > + > > > +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