If a machine is reset while secrets are present in RAM, it may be
possible for code executed after the reboot to extract those secrets
from untouched memory. The Trusted Computing Group specified a mechanism
for requesting that the firmware clear all RAM on reset before booting
another OS. This is done by setting the MemoryOverwriteRequestControl
variable at startup. If userspace can ensure that all secrets are
removed as part of a controlled shutdown, it can reset this variable to
0 before triggering a hardware reboot.
Signed-off-by: Matthew Garrett <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 3 ++
drivers/firmware/efi/Kconfig | 10 ++++++
drivers/firmware/efi/libstub/Makefile | 1 +
drivers/firmware/efi/libstub/arm-stub.c | 3 ++
drivers/firmware/efi/libstub/tpm.c | 58 +++++++++++++++++++++++++++++++++
include/linux/efi.h | 7 ++++
6 files changed, 82 insertions(+)
create mode 100644 drivers/firmware/efi/libstub/tpm.c
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c3e869eaef0c..a1686f3dc295 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -997,6 +997,9 @@ struct boot_params *efi_main(struct efi_config *c,
if (boot_params->secure_boot == efi_secureboot_mode_unset)
boot_params->secure_boot = efi_get_secureboot(sys_table);
+ /* Ask the firmware to clear memory on unclean shutdown */
+ efi_enable_reset_attack_mitigation(sys_table);
+
setup_graphics(boot_params);
setup_efi_pci(boot_params);
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 394db40ed374..2b4c39fdfa91 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -151,6 +151,16 @@ config APPLE_PROPERTIES
If unsure, say Y if you have a Mac. Otherwise N.
+config RESET_ATTACK_MITIGATION
+ bool "Reset memory attack mitigation"
+ depends on EFI_STUB
+ help
+ Request that the firmware clear the contents of RAM after a reboot
+ using the TCG Platform Reset Attack Mitigation specification. This
+ protects against an attacker forcibly rebooting the system while it
+ still contains secrets in RAM, booting another OS and extracting the
+ secrets.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 37e24f525162..4b20a5516929 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -30,6 +30,7 @@ OBJECT_FILES_NON_STANDARD := y
KCOV_INSTRUMENT := n
lib-y := efi-stub-helper.o gop.o secureboot.o
+lib-$(CONFIG_RESET_ATTACK_MITIGATION) += tpm.o
# include the stub's generic dependencies from lib/ when building for ARM/arm64
arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 8181ac179d14..1cb2d1c070c3 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -192,6 +192,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
goto fail_free_cmdline;
}
+ /* Ask the firmware to clear memory on unclean shutdown */
+ efi_enable_reset_attack_mitigation(sys_table);
+
secure_boot = efi_get_secureboot(sys_table);
/*
diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
new file mode 100644
index 000000000000..45a12311adc3
--- /dev/null
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -0,0 +1,58 @@
+/*
+ * TPM handling.
+ *
+ * Copyright (C) 2016 CoreOS, Inc
+ * Copyright (C) 2017 Google, Inc.
+ * Matthew Garrett <[email protected]>
+ *
+ * This file is part of the Linux kernel, and is made available under the
+ * terms of the GNU General Public License version 2.
+ */
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+#include "efistub.h"
+
+static const efi_char16_t efi_MemoryOverWriteRequest_name[] = {
+ 'M', 'e', 'm', 'o', 'r', 'y', 'O', 'v', 'e', 'r', 'w', 'r', 'i', 't',
+ 'e', 'R', 'e', 'q', 'u', 'e', 's', 't', 'C', 'o', 'n', 't', 'r', 'o',
+ 'l', 0
+};
+
+#define MEMORY_ONLY_RESET_CONTROL_GUID \
+ EFI_GUID(0xe20939be, 0x32d4, 0x41be, 0xa1, 0x50, 0x89, 0x7f, 0x85, 0xd4, 0x98, 0x29)
+
+#define get_efi_var(name, vendor, ...) \
+ efi_call_runtime(get_variable, \
+ (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
+ __VA_ARGS__)
+
+#define set_efi_var(name, vendor, ...) \
+ efi_call_runtime(set_variable, \
+ (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
+ __VA_ARGS__)
+
+/*
+ * Enable reboot attack mitigation. This requests that the firmware clear the
+ * RAM on next reboot before proceeding with boot, ensuring that any secrets
+ * are cleared. If userland has ensured that all secrets have bene removed
+ * from RAM before reboot it can simply reset this variable.
+ */
+void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
+{
+ u8 val = 1;
+ efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
+ efi_status_t status;
+ unsigned long datasize = 0;
+
+ status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
+ NULL, &datasize, NULL);
+
+ if (status == EFI_NOT_FOUND)
+ return;
+
+ set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), val);
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8269bcb8ccf7..12e05118657c 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1497,6 +1497,13 @@ enum efi_secureboot_mode {
};
enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
+#ifdef CONFIG_RESET_ATTACK_MITIGATION
+void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg);
+#else
+static inline void
+efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
+#endif
+
/*
* Arch code can implement the following three template macros, avoiding
* reptition for the void/non-void return cases of {__,}efi_call_virt():
--
2.14.0.rc1.383.gd1ce394fe2-goog
On 4 August 2017 at 22:20, Matthew Garrett <[email protected]> wrote:
> If a machine is reset while secrets are present in RAM, it may be
> possible for code executed after the reboot to extract those secrets
> from untouched memory. The Trusted Computing Group specified a mechanism
> for requesting that the firmware clear all RAM on reset before booting
> another OS. This is done by setting the MemoryOverwriteRequestControl
> variable at startup. If userspace can ensure that all secrets are
> removed as part of a controlled shutdown, it can reset this variable to
> 0 before triggering a hardware reboot.
>
Shouldn't it be up to the kernel to decide whether this flag should be
cleared after userspace has indicated to it that it thinks it has
wiped all secrets from memory? The kernel itself may keep secrets as
well, and we may still crash in the time window between userspace
invoking shutdown and the kernel actually calling ResetSystem() in the
firmware.
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 3 ++
> drivers/firmware/efi/Kconfig | 10 ++++++
> drivers/firmware/efi/libstub/Makefile | 1 +
> drivers/firmware/efi/libstub/arm-stub.c | 3 ++
> drivers/firmware/efi/libstub/tpm.c | 58 +++++++++++++++++++++++++++++++++
> include/linux/efi.h | 7 ++++
> 6 files changed, 82 insertions(+)
> create mode 100644 drivers/firmware/efi/libstub/tpm.c
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index c3e869eaef0c..a1686f3dc295 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -997,6 +997,9 @@ struct boot_params *efi_main(struct efi_config *c,
> if (boot_params->secure_boot == efi_secureboot_mode_unset)
> boot_params->secure_boot = efi_get_secureboot(sys_table);
>
> + /* Ask the firmware to clear memory on unclean shutdown */
> + efi_enable_reset_attack_mitigation(sys_table);
> +
> setup_graphics(boot_params);
>
> setup_efi_pci(boot_params);
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 394db40ed374..2b4c39fdfa91 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -151,6 +151,16 @@ config APPLE_PROPERTIES
>
> If unsure, say Y if you have a Mac. Otherwise N.
>
> +config RESET_ATTACK_MITIGATION
> + bool "Reset memory attack mitigation"
> + depends on EFI_STUB
> + help
> + Request that the firmware clear the contents of RAM after a reboot
> + using the TCG Platform Reset Attack Mitigation specification. This
> + protects against an attacker forcibly rebooting the system while it
> + still contains secrets in RAM, booting another OS and extracting the
> + secrets.
> +
> endmenu
>
> config UEFI_CPER
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 37e24f525162..4b20a5516929 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -30,6 +30,7 @@ OBJECT_FILES_NON_STANDARD := y
> KCOV_INSTRUMENT := n
>
> lib-y := efi-stub-helper.o gop.o secureboot.o
> +lib-$(CONFIG_RESET_ATTACK_MITIGATION) += tpm.o
>
> # include the stub's generic dependencies from lib/ when building for ARM/arm64
> arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 8181ac179d14..1cb2d1c070c3 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -192,6 +192,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
> goto fail_free_cmdline;
> }
>
> + /* Ask the firmware to clear memory on unclean shutdown */
> + efi_enable_reset_attack_mitigation(sys_table);
> +
> secure_boot = efi_get_secureboot(sys_table);
>
> /*
> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> new file mode 100644
> index 000000000000..45a12311adc3
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -0,0 +1,58 @@
> +/*
> + * TPM handling.
> + *
> + * Copyright (C) 2016 CoreOS, Inc
> + * Copyright (C) 2017 Google, Inc.
> + * Matthew Garrett <[email protected]>
> + *
> + * This file is part of the Linux kernel, and is made available under the
> + * terms of the GNU General Public License version 2.
> + */
> +#include <linux/efi.h>
> +#include <asm/efi.h>
> +
> +#include "efistub.h"
> +
> +static const efi_char16_t efi_MemoryOverWriteRequest_name[] = {
> + 'M', 'e', 'm', 'o', 'r', 'y', 'O', 'v', 'e', 'r', 'w', 'r', 'i', 't',
> + 'e', 'R', 'e', 'q', 'u', 'e', 's', 't', 'C', 'o', 'n', 't', 'r', 'o',
> + 'l', 0
> +};
> +
> +#define MEMORY_ONLY_RESET_CONTROL_GUID \
> + EFI_GUID(0xe20939be, 0x32d4, 0x41be, 0xa1, 0x50, 0x89, 0x7f, 0x85, 0xd4, 0x98, 0x29)
> +
> +#define get_efi_var(name, vendor, ...) \
> + efi_call_runtime(get_variable, \
> + (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
> + __VA_ARGS__)
> +
> +#define set_efi_var(name, vendor, ...) \
> + efi_call_runtime(set_variable, \
> + (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
> + __VA_ARGS__)
> +
> +/*
> + * Enable reboot attack mitigation. This requests that the firmware clear the
> + * RAM on next reboot before proceeding with boot, ensuring that any secrets
> + * are cleared. If userland has ensured that all secrets have bene removed
> + * from RAM before reboot it can simply reset this variable.
> + */
> +void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
> +{
> + u8 val = 1;
> + efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
> + efi_status_t status;
> + unsigned long datasize = 0;
> +
> + status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> + NULL, &datasize, NULL);
> +
> + if (status == EFI_NOT_FOUND)
> + return;
> +
> + set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> + EFI_VARIABLE_NON_VOLATILE |
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), val);
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 8269bcb8ccf7..12e05118657c 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1497,6 +1497,13 @@ enum efi_secureboot_mode {
> };
> enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
>
> +#ifdef CONFIG_RESET_ATTACK_MITIGATION
> +void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg);
> +#else
> +static inline void
> +efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
> +#endif
> +
> /*
> * Arch code can implement the following three template macros, avoiding
> * reptition for the void/non-void return cases of {__,}efi_call_virt():
> --
> 2.14.0.rc1.383.gd1ce394fe2-goog
>
On Sat, Aug 5, 2017 at 2:50 AM, Ard Biesheuvel
<[email protected]> wrote:
> On 4 August 2017 at 22:20, Matthew Garrett <[email protected]> wrote:
>> If a machine is reset while secrets are present in RAM, it may be
>> possible for code executed after the reboot to extract those secrets
>> from untouched memory. The Trusted Computing Group specified a mechanism
>> for requesting that the firmware clear all RAM on reset before booting
>> another OS. This is done by setting the MemoryOverwriteRequestControl
>> variable at startup. If userspace can ensure that all secrets are
>> removed as part of a controlled shutdown, it can reset this variable to
>> 0 before triggering a hardware reboot.
>>
>
> Shouldn't it be up to the kernel to decide whether this flag should be
> cleared after userspace has indicated to it that it thinks it has
> wiped all secrets from memory? The kernel itself may keep secrets as
> well, and we may still crash in the time window between userspace
> invoking shutdown and the kernel actually calling ResetSystem() in the
> firmware.
What's the threat model? If there's no way for userland to ask the
kernel to drop any secrets it still holds, that seems like a problem
in any case. If the concern is that someone may be able to clear the
flag and then reboot in order to deliberately attempt to obtain kernel
secrets then there's no hugely easy way around this without special
casing the variable and preventing userland from being able to modify
it. There's a MemoryOverwriteRequestLock spec from Microsoft that
provides a mechanism for this (the firmware and the OS configure a
shared secret that controls access to MemoryOverwriteRequestControl,
so we'd keep that in the kernel and clear it on reboot), but that's
not implemented everywhere and we'd still want to base on top of this.
On Sat, Aug 05, 2017 at 09:35:22AM -0700, Matthew Garrett wrote:
> On Sat, Aug 5, 2017 at 2:50 AM, Ard Biesheuvel <[email protected]> wrote:
> > On 4 August 2017 at 22:20, Matthew Garrett <[email protected]> wrote:
> > > If a machine is reset while secrets are present in RAM, it may be
> > > possible for code executed after the reboot to extract those secrets
> > > from untouched memory. The Trusted Computing Group specified a mechanism
> > > for requesting that the firmware clear all RAM on reset before booting
> > > another OS. This is done by setting the MemoryOverwriteRequestControl
> > > variable at startup. If userspace can ensure that all secrets are
> > > removed as part of a controlled shutdown, it can reset this variable to
> > > 0 before triggering a hardware reboot.
> >
> > Shouldn't it be up to the kernel to decide whether this flag should be
> > cleared after userspace has indicated to it that it thinks it has
> > wiped all secrets from memory? The kernel itself may keep secrets as
> > well, and we may still crash in the time window between userspace
> > invoking shutdown and the kernel actually calling ResetSystem() in the
> > firmware.
>
> What's the threat model? If there's no way for userland to ask the
> kernel to drop any secrets it still holds, that seems like a problem
> in any case. If the concern is that someone may be able to clear the
> flag and then reboot in order to deliberately attempt to obtain kernel
> secrets then there's no hugely easy way around this without special
> casing the variable and preventing userland from being able to modify
> it. There's a MemoryOverwriteRequestLock spec from Microsoft that
> provides a mechanism for this (the firmware and the OS configure a
> shared secret that controls access to MemoryOverwriteRequestControl,
> so we'd keep that in the kernel and clear it on reboot), but that's
> not implemented everywhere and we'd still want to base on top of this.
Just an innocent question from a bystander, what's the downside of
unconditionally requesting that memory be overwritten? Does it
prolong reboot noticeably?
I've also wondered why you've chosen to put this in a separate file
rather than the existing secureboot.c, my naive understanding is that
TPM and SecureBoot is related but I'm not an expert on this. It would
allow you to reuse the existing get_efi_var() macro.
Thanks,
Lukas (aka the "abusive community" (?)
https://mobile.twitter.com/mjg59/status/893543164371283969 )
On Sat, Aug 5, 2017 at 10:34 AM, Lukas Wunner <[email protected]> wrote:
> Just an innocent question from a bystander, what's the downside of
> unconditionally requesting that memory be overwritten? Does it
> prolong reboot noticeably?
Yes, it's just to avoid stalling reboot for as long as it takes to clear RAM.
> I've also wondered why you've chosen to put this in a separate file
> rather than the existing secureboot.c, my naive understanding is that
> TPM and SecureBoot is related but I'm not an expert on this. It would
> allow you to reuse the existing get_efi_var() macro.
It's not related to Secure Boot (systems can have TPMs but not Secure
Boot, and vice versa), the spec is managed by a different body (TCG
rather than UEFI), and there'll be more TPM-related code for the boot
stub in future.
On 4 August 2017 at 22:20, Matthew Garrett <[email protected]> wrote:
> If a machine is reset while secrets are present in RAM, it may be
> possible for code executed after the reboot to extract those secrets
> from untouched memory. The Trusted Computing Group specified a mechanism
> for requesting that the firmware clear all RAM on reset before booting
> another OS. This is done by setting the MemoryOverwriteRequestControl
> variable at startup. If userspace can ensure that all secrets are
> removed as part of a controlled shutdown, it can reset this variable to
> 0 before triggering a hardware reboot.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 3 ++
> drivers/firmware/efi/Kconfig | 10 ++++++
> drivers/firmware/efi/libstub/Makefile | 1 +
> drivers/firmware/efi/libstub/arm-stub.c | 3 ++
> drivers/firmware/efi/libstub/tpm.c | 58 +++++++++++++++++++++++++++++++++
> include/linux/efi.h | 7 ++++
> 6 files changed, 82 insertions(+)
> create mode 100644 drivers/firmware/efi/libstub/tpm.c
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index c3e869eaef0c..a1686f3dc295 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -997,6 +997,9 @@ struct boot_params *efi_main(struct efi_config *c,
> if (boot_params->secure_boot == efi_secureboot_mode_unset)
> boot_params->secure_boot = efi_get_secureboot(sys_table);
>
> + /* Ask the firmware to clear memory on unclean shutdown */
> + efi_enable_reset_attack_mitigation(sys_table);
> +
> setup_graphics(boot_params);
>
> setup_efi_pci(boot_params);
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 394db40ed374..2b4c39fdfa91 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -151,6 +151,16 @@ config APPLE_PROPERTIES
>
> If unsure, say Y if you have a Mac. Otherwise N.
>
> +config RESET_ATTACK_MITIGATION
> + bool "Reset memory attack mitigation"
> + depends on EFI_STUB
> + help
> + Request that the firmware clear the contents of RAM after a reboot
> + using the TCG Platform Reset Attack Mitigation specification. This
> + protects against an attacker forcibly rebooting the system while it
> + still contains secrets in RAM, booting another OS and extracting the
> + secrets.
> +
> endmenu
>
> config UEFI_CPER
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 37e24f525162..4b20a5516929 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -30,6 +30,7 @@ OBJECT_FILES_NON_STANDARD := y
> KCOV_INSTRUMENT := n
>
> lib-y := efi-stub-helper.o gop.o secureboot.o
> +lib-$(CONFIG_RESET_ATTACK_MITIGATION) += tpm.o
>
> # include the stub's generic dependencies from lib/ when building for ARM/arm64
> arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 8181ac179d14..1cb2d1c070c3 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -192,6 +192,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
> goto fail_free_cmdline;
> }
>
> + /* Ask the firmware to clear memory on unclean shutdown */
> + efi_enable_reset_attack_mitigation(sys_table);
> +
> secure_boot = efi_get_secureboot(sys_table);
>
> /*
> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> new file mode 100644
> index 000000000000..45a12311adc3
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -0,0 +1,58 @@
> +/*
> + * TPM handling.
> + *
> + * Copyright (C) 2016 CoreOS, Inc
> + * Copyright (C) 2017 Google, Inc.
> + * Matthew Garrett <[email protected]>
> + *
> + * This file is part of the Linux kernel, and is made available under the
> + * terms of the GNU General Public License version 2.
> + */
> +#include <linux/efi.h>
> +#include <asm/efi.h>
> +
> +#include "efistub.h"
> +
> +static const efi_char16_t efi_MemoryOverWriteRequest_name[] = {
> + 'M', 'e', 'm', 'o', 'r', 'y', 'O', 'v', 'e', 'r', 'w', 'r', 'i', 't',
> + 'e', 'R', 'e', 'q', 'u', 'e', 's', 't', 'C', 'o', 'n', 't', 'r', 'o',
> + 'l', 0
> +};
> +
> +#define MEMORY_ONLY_RESET_CONTROL_GUID \
> + EFI_GUID(0xe20939be, 0x32d4, 0x41be, 0xa1, 0x50, 0x89, 0x7f, 0x85, 0xd4, 0x98, 0x29)
> +
> +#define get_efi_var(name, vendor, ...) \
> + efi_call_runtime(get_variable, \
> + (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
> + __VA_ARGS__)
> +
> +#define set_efi_var(name, vendor, ...) \
> + efi_call_runtime(set_variable, \
> + (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
> + __VA_ARGS__)
> +
> +/*
> + * Enable reboot attack mitigation. This requests that the firmware clear the
> + * RAM on next reboot before proceeding with boot, ensuring that any secrets
> + * are cleared. If userland has ensured that all secrets have bene removed
s/bene/been/
> + * from RAM before reboot it can simply reset this variable.
> + */
> +void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
> +{
> + u8 val = 1;
> + efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
> + efi_status_t status;
> + unsigned long datasize = 0;
> +
> + status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> + NULL, &datasize, NULL);
> +
> + if (status == EFI_NOT_FOUND)
> + return;
> +
> + set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> + EFI_VARIABLE_NON_VOLATILE |
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), val);
Shouldn't this be &val?
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 8269bcb8ccf7..12e05118657c 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1497,6 +1497,13 @@ enum efi_secureboot_mode {
> };
> enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
>
> +#ifdef CONFIG_RESET_ATTACK_MITIGATION
> +void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg);
> +#else
> +static inline void
> +efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) { }
> +#endif
> +
> /*
> * Arch code can implement the following three template macros, avoiding
> * reptition for the void/non-void return cases of {__,}efi_call_virt():
>> Shouldn't it be up to the kernel to decide whether this flag should be
>> cleared after userspace has indicated to it that it thinks it has
>> wiped all secrets from memory? The kernel itself may keep secrets as
>> well, and we may still crash in the time window between userspace
>> invoking shutdown and the kernel actually calling ResetSystem() in the
>> firmware.
>
> What's the threat model? If there's no way for userland to ask the
> kernel to drop any secrets it still holds, that seems like a problem
> in any case. If the concern is that someone may be able to clear the
> flag and then reboot in order to deliberately attempt to obtain kernel
> secrets then there's no hugely easy way around this without special
> casing the variable and preventing userland from being able to modify
> it. There's a MemoryOverwriteRequestLock spec from Microsoft that
> provides a mechanism for this (the firmware and the OS configure a
> shared secret that controls access to MemoryOverwriteRequestControl,
> so we'd keep that in the kernel and clear it on reboot), but that's
> not implemented everywhere and we'd still want to base on top of this.
So how would that work with, e.g., the keys for your encrypted file
system? Surely, you can't expect the kernel to drop that secret when
userland asks it to, so there will always be a window where userland
has set the variable but the kernel is not ready to drop its secrets
yet.
Thanks,
Ard.
On Fri, Aug 18, 2017 at 11:52 AM, Ard Biesheuvel
<[email protected]> wrote:
> On 4 August 2017 at 22:20, Matthew Garrett <[email protected]> wrote:
>> + * Enable reboot attack mitigation. This requests that the firmware clear the
>> + * RAM on next reboot before proceeding with boot, ensuring that any secrets
>> + * are cleared. If userland has ensured that all secrets have bene removed
>
> s/bene/been/
Thanks.
>> + set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>> + EFI_VARIABLE_NON_VOLATILE |
>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> + EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), val);
>
> Shouldn't this be &val?
Ooh, good catch - not sure how that got eaten.
>> What's the threat model? If there's no way for userland to ask the
>> kernel to drop any secrets it still holds, that seems like a problem
>> in any case. If the concern is that someone may be able to clear the
>> flag and then reboot in order to deliberately attempt to obtain kernel
>> secrets then there's no hugely easy way around this without special
>> casing the variable and preventing userland from being able to modify
>> it. There's a MemoryOverwriteRequestLock spec from Microsoft that
>> provides a mechanism for this (the firmware and the OS configure a
>> shared secret that controls access to MemoryOverwriteRequestControl,
>> so we'd keep that in the kernel and clear it on reboot), but that's
>> not implemented everywhere and we'd still want to base on top of this.
>
> So how would that work with, e.g., the keys for your encrypted file
> system? Surely, you can't expect the kernel to drop that secret when
> userland asks it to, so there will always be a window where userland
> has set the variable but the kernel is not ready to drop its secrets
> yet.
If the kernel doesn't synchronously zero the key when dm-crypt is torn
down, that feels like a bug?
On 18 August 2017 at 20:08, Matthew Garrett <[email protected]> wrote:
> On Fri, Aug 18, 2017 at 11:52 AM, Ard Biesheuvel
> <[email protected]> wrote:
>> On 4 August 2017 at 22:20, Matthew Garrett <[email protected]> wrote:
>>> + * Enable reboot attack mitigation. This requests that the firmware clear the
>>> + * RAM on next reboot before proceeding with boot, ensuring that any secrets
>>> + * are cleared. If userland has ensured that all secrets have bene removed
>>
>> s/bene/been/
>
> Thanks.
>
>>> + set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>> + EFI_VARIABLE_NON_VOLATILE |
>>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> + EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), val);
>>
>> Shouldn't this be &val?
>
> Ooh, good catch - not sure how that got eaten.
>
>>> What's the threat model? If there's no way for userland to ask the
>>> kernel to drop any secrets it still holds, that seems like a problem
>>> in any case. If the concern is that someone may be able to clear the
>>> flag and then reboot in order to deliberately attempt to obtain kernel
>>> secrets then there's no hugely easy way around this without special
>>> casing the variable and preventing userland from being able to modify
>>> it. There's a MemoryOverwriteRequestLock spec from Microsoft that
>>> provides a mechanism for this (the firmware and the OS configure a
>>> shared secret that controls access to MemoryOverwriteRequestControl,
>>> so we'd keep that in the kernel and clear it on reboot), but that's
>>> not implemented everywhere and we'd still want to base on top of this.
>>
>> So how would that work with, e.g., the keys for your encrypted file
>> system? Surely, you can't expect the kernel to drop that secret when
>> userland asks it to, so there will always be a window where userland
>> has set the variable but the kernel is not ready to drop its secrets
>> yet.
>
> If the kernel doesn't synchronously zero the key when dm-crypt is torn
> down, that feels like a bug?
Of course it should. But that is not the point. The point is that
userland is in no position to decide whether or not memory has been
sufficiently cleaned so that the firmware can omit wiping all of it
(in case you care enough about your secrets to enable that feature in
the first place).
Given that the string 'MemoryOverWriteRequest' does not appear in
today's EDK2, I don't suppose there is any urgency wrt getting this
queued for v4.14?
On Fri, Aug 18, 2017 at 12:29 PM, Ard Biesheuvel
<[email protected]> wrote:
> On 18 August 2017 at 20:08, Matthew Garrett <[email protected]> wrote:
>> If the kernel doesn't synchronously zero the key when dm-crypt is torn
>> down, that feels like a bug?
>
> Of course it should. But that is not the point. The point is that
> userland is in no position to decide whether or not memory has been
> sufficiently cleaned so that the firmware can omit wiping all of it
> (in case you care enough about your secrets to enable that feature in
> the first place).
The kernel is in no position to decide whether or not userland has
disposed of secrets either - at some point you need to trust a
component, and I have more faith that the kernel will do the right
thing. The only other option here seems to be a double opt-in (ie,
have userland assert that it's cleaned up, have the kernel set the
flag at reset time) but we're still assuming that the kernel is
behaving correctly, and if we assume that the kernel is behaving
correctly then we can just let userland set the flag anyway.
> Given that the string 'MemoryOverWriteRequest' does not appear in
> today's EDK2, I don't suppose there is any urgency wrt getting this
> queued for v4.14?
It's not implemented in EDK2, but it's in pretty much every vendor
implementation. All the machines I have here support this already.
On 18 August 2017 at 20:57, Matthew Garrett <[email protected]> wrote:
> On Fri, Aug 18, 2017 at 12:29 PM, Ard Biesheuvel
> <[email protected]> wrote:
>> On 18 August 2017 at 20:08, Matthew Garrett <[email protected]> wrote:
>>> If the kernel doesn't synchronously zero the key when dm-crypt is torn
>>> down, that feels like a bug?
>>
>> Of course it should. But that is not the point. The point is that
>> userland is in no position to decide whether or not memory has been
>> sufficiently cleaned so that the firmware can omit wiping all of it
>> (in case you care enough about your secrets to enable that feature in
>> the first place).
>
> The kernel is in no position to decide whether or not userland has
> disposed of secrets either - at some point you need to trust a
> component, and I have more faith that the kernel will do the right
> thing. The only other option here seems to be a double opt-in (ie,
> have userland assert that it's cleaned up, have the kernel set the
> flag at reset time) but we're still assuming that the kernel is
> behaving correctly, and if we assume that the kernel is behaving
> correctly then we can just let userland set the flag anyway.
>
OK, fair enough.
>> Given that the string 'MemoryOverWriteRequest' does not appear in
>> today's EDK2, I don't suppose there is any urgency wrt getting this
>> queued for v4.14?
>
> It's not implemented in EDK2, but it's in pretty much every vendor
> implementation. All the machines I have here support this already.
OK. I will get it queued. No need to resend, I can apply the fixes locally.
On Fri, Aug 18, 2017 at 1:19 PM, Ard Biesheuvel
<[email protected]> wrote:
> OK. I will get it queued. No need to resend, I can apply the fixes locally.
Thanks!
On Fri, Aug 18, 2017 at 12:08:34PM -0700, Matthew Garrett wrote:
> On Fri, Aug 18, 2017 at 11:52 AM, Ard Biesheuvel <[email protected]> wrote:
> > So how would that work with, e.g., the keys for your encrypted file
> > system? Surely, you can't expect the kernel to drop that secret when
> > userland asks it to, so there will always be a window where userland
> > has set the variable but the kernel is not ready to drop its secrets
> > yet.
>
> If the kernel doesn't synchronously zero the key when dm-crypt is torn
> down, that feels like a bug?
In the case of a root filesystem layered atop dm-crypt, tearing down
dm-crypt depends on userland, more specifically the init system.
With systemd + dracut it's possible to pivot back into an initrd
on shutdown and this allows for proper teardown of dm-crypt and
subsequent wiping of the key material from memory.
With initramfs-tools this is not possible and to the best of my
knowledge the key material is still resident in memory on shutdown:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=778849
Another case where key material may still be resident are IPsec tunnels.
Granted, user space could tear these down as well on shutdown but what
if it doesn't?
The patch itself seems to be of value, perhaps it can be moved forward
if the commit message is rephrased to leave open the contentious issue
if and when memory wiping by the firmware is disabled, to be discussed
separately. I'd vote for a machanism whereby user space signals to the
kernel that it no longer has secrets, and the kernel can then signal to
the firmware that memory need not be cleared if it also no longer holds
secrets. Just my 2 cents anyway.
Thanks!
Lukas