| Message ID | 1439273796-25359-7-git-send-email-jlee@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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". > 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. > Reviewed-by: Jiri Kosina <jkosina@suse.com> > Tested-by: Jiri Kosina <jkosina@suse.com> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com> > --- > 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. > + 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. > + > + 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. > 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.
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@suse.com> > > Tested-by: Jiri Kosina <jkosina@suse.com> > > Signed-off-by: Lee, Chun-Yi <jlee@suse.com> > > --- > > 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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
On Wed, Sep 09, 2015 at 01:15:45PM +0100, Matt Fleming wrote: > 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. > Thanks for you point out. But, I want keep this logic to avoid someone create a runtime variable with the same name-SSID to confuse the key generator in EFI stub. > > > > 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 The job of key generator in EFI stub is to generate appropriate length of key for usage by later activities. It doesn't need to know how does kernel use which kind of hash algorithm, so I think do not need record the hash function. But, kernel should knows the exactly length of old key to decide if it needs to use flag to trigger key regeneration process in EFI stub to create new key for new length. Only the other hand, the hash algorithm of hibernation was setted in kernel compiler time even user can change it, so I think to forward the key length from querying efi variable is enough, don't need write the hash algorithm to EFI boot variable. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + 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; + } + } + + 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"); + } +} +#else +static void setup_hibernation_keys(struct boot_params *params) {} +#endif + /* * On success we return a pointer to a boot_params structure, and NULL * on failure. @@ -1420,6 +1478,8 @@ struct boot_params *efi_main(struct efi_config *c, setup_efi_pci(boot_params); + setup_hibernation_keys(boot_params); + status = efi_call_early(allocate_pool, EFI_LOADER_DATA, sizeof(*gdt), (void **)&gdt); if (status != EFI_SUCCESS) { 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 diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 56c6de9..aa88b3b 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -328,6 +328,9 @@ struct platform_hibernation_ops { #ifdef CONFIG_HIBERNATION +#define EFI_HIBERNATION_GUID \ + EFI_GUID(0xfe141863, 0xc070, 0x478e, 0xb8, 0xa3, 0x87, 0x8a, 0x5d, 0xc9, 0xef, 0x21) + /* HMAC Algorithm of Hibernate Signature */ #define HIBERNATION_HMAC "hmac(sha1)" #define HIBERNATION_DIGEST_SIZE 20