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.

quoted

quoted

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