On Thu, Aug 20, 2015 at 09:40:44PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:26PM, Lee, Chun-Yi wrote:
> > This patch adds codes in EFI stub for generating and storing the
> > HMAC key in EFI boot service variable for signing hibernate image.
> >
> > Per rcf2104, the length of HMAC-SHA1 hash result is 20 bytes, and
> > it recommended the length of key the same with hash rsult, means
> > also 20 bytes. Using longer key would not significantly increase
> > the function strength. Due to the nvram space is limited in some
> > UEFI machines, so using the minimal recommended length 20 bytes
> > key that will stored in boot service variable.
>
> I'm having a hard time understanding the middle part of this
> paragraph, specifically the part of the key and the hash result.
> There's a typo in the subject too s/siging/signing/and "Generating"
> should be "Generate".
>
In description, I want to explain why the length of key is 20 bytes.
I will try to explain more clear, if it still confuses reviewer then
I will remove the description.
> > The HMAC key stored in EFI boot service variable, GUID is
> > HIBERNATIONKey-fe141863-c070-478e-b8a3-878a5dc9ef21.
>
> I'd really like to see some of the explanation from your cover letter
> included in the commit message for this patch, and in particular why
> signing hibernate images is a good thing.
>
> Recording that for posterity in the commit message is going to be
> helpful when someone looks at this patch in 2 years time and wonders
> why RNG support was added to the EFI stub and why they might want to
> sign hibernate images.
>
Thanks for suggestion, I will move some description from cover letter
to here and add comment for history and RNG support.
> > Reviewed-by: Jiri Kosina <jkosina@xxxxxxxx>
> > Tested-by: Jiri Kosina <jkosina@xxxxxxxx>
> > Signed-off-by: Lee, Chun-Yi <jlee@xxxxxxxx>
> > ---
> > arch/x86/boot/compressed/eboot.c | 60 ++++++++++++++++++++++++++++++++++++++++
> > arch/x86/include/asm/suspend.h | 9 ++++++
> > include/linux/suspend.h | 3 ++
> > 3 files changed, 72 insertions(+)
> >
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 0ffb6db..463aa9b 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -12,6 +12,7 @@
> > #include <asm/efi.h>
> > #include <asm/setup.h>
> > #include <asm/desc.h>
> > +#include <asm/suspend.h>
> >
> > #include "../string.h"
> > #include "eboot.h"
> > @@ -1383,6 +1384,63 @@ free_mem_map:
> > return status;
> > }
> >
> > +#ifdef CONFIG_HIBERNATE_VERIFICATION
> > +#define HIBERNATION_KEY \
> > + ((efi_char16_t [15]) { 'H', 'I', 'B', 'E', 'R', 'N', 'A', 'T', 'I', 'O', 'N', 'K', 'e', 'y', 0 })
> > +#define HIBERNATION_KEY_ATTRIBUTE (EFI_VARIABLE_NON_VOLATILE | \
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS)
> > +
> > +static void setup_hibernation_keys(struct boot_params *params)
> > +{
> > + unsigned long key_size;
> > + unsigned long attributes;
> > + struct hibernation_keys *keys;
> > + efi_status_t status;
> > +
> > + /* Allocate setup_data to carry keys */
> > + status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> > + sizeof(struct hibernation_keys), &keys);
> > + if (status != EFI_SUCCESS) {
> > + efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
> > + return;
> > + }
> > +
> > + memset(keys, 0, sizeof(struct hibernation_keys));
> > +
> > + status = efi_call_early(get_variable, HIBERNATION_KEY,
> > + &EFI_HIBERNATION_GUID, &attributes,
> > + &key_size, keys->hibernation_key);
>
> Tiny nit, but could you put a new line here please? This is a large
> chunk of code.
>
OK, I will add new line here.
> > + if (status == EFI_SUCCESS && attributes != HIBERNATION_KEY_ATTRIBUTE) {
> > + efi_printk(sys_table, "A hibernation key is not boot service variable\n");
> > + memset(keys->hibernation_key, 0, HIBERNATION_DIGEST_SIZE);
> > + status = efi_call_early(set_variable, HIBERNATION_KEY,
> > + &EFI_HIBERNATION_GUID, attributes, 0,
> > + NULL);
> > + if (status == EFI_SUCCESS) {
> > + efi_printk(sys_table, "Cleaned existing hibernation key\n");
> > + status = EFI_NOT_FOUND;
> > + }
> > + }
>
> Hmm.. it's not clear to me that we should be deleting this EFI
> variable if the attributes are bogus. It would be safer to just bail.
>
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.
> > +
> > + if (status != EFI_SUCCESS) {
> > + efi_printk(sys_table, "Failed to get existing hibernation key\n");
> > +
> > + efi_get_random_key(sys_table, params, keys->hibernation_key,
> > + HIBERNATION_DIGEST_SIZE);
> > +
> > + status = efi_call_early(set_variable, HIBERNATION_KEY,
> > + &EFI_HIBERNATION_GUID,
> > + HIBERNATION_KEY_ATTRIBUTE,
> > + HIBERNATION_DIGEST_SIZE,
> > + keys->hibernation_key);
> > + if (status != EFI_SUCCESS)
> > + efi_printk(sys_table, "Failed to set hibernation key\n");
>
> You're leaking 'keys' here.
>
Oh, you are right, I will free keys here.
> > 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?
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.
> --
> Matt Fleming, Intel Open Source Technology Center
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
