[PATCH V38 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an
EFI variable, which gives arbitrary code execution in ring 0. Prevent
that when the kernel is locked down.
Signed-off-by: Matthew Garrett
Acked-by: Ard Biesheuvel
Reviewed-by: Kees Cook
Cc: Ard Biesheuvel
Cc: linux-efi@vger.kernel.org
---
drivers/firmware/efi/efi.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index ad3b1f4866b3..776f479e5499 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
@@ -242,6 +243,11 @@ static void generic_ops_unregister(void)
static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
static int __init efivar_ssdt_setup(char *str)
{
+ int ret = security_locked_down(LOCKDOWN_ACPI_TABLES);
+
+ if (ret)
+ return ret;
+
if (strlen(str) < sizeof(efivar_ssdt))
memcpy(efivar_ssdt, str, strlen(str));
else
--
2.22.0.770.g0f2c4a37fd-goog
Re: [PATCH 5.3 regression fix] efi-stub: Fix get_efi_config_table on mixed-mode setups
On Wed, Aug 7, 2019 at 2:59 PM Hans de Goede wrote:
>
> Fix get_efi_config_table using the wrong structs when booting a
> 64 bit kernel on 32 bit firmware.
>
> Cc: Matthew Garrett
> Cc: Ard Biesheuvel
> Cc: Jarkko Sakkinen
> Fixes: 82d736ac56d7 ("Abstract out support for locating an EFI config table")
> Signed-off-by: Hans de Goede
Acked-By: Matthew Garrett
Good catch. I think fixing this is preferable to reverting - the
duplicate events are visible to userland, so there's a risk that apps
will end up depending on them if there's a release that behaves that
way. Presumably mixed mode isn't a thing on ARM?
[PATCH 5.3 regression fix] efi-stub: Fix get_efi_config_table on mixed-mode setups
Fix get_efi_config_table using the wrong structs when booting a
64 bit kernel on 32 bit firmware.
Cc: Matthew Garrett
Cc: Ard Biesheuvel
Cc: Jarkko Sakkinen
Fixes: 82d736ac56d7 ("Abstract out support for locating an EFI config table")
Signed-off-by: Hans de Goede
---
.../firmware/efi/libstub/efi-stub-helper.c| 38 +--
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c
b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 1db780c0f07b..3caae7f2cf56 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -927,17 +927,33 @@ efi_status_t efi_exit_boot_services(efi_system_table_t
*sys_table_arg,
return status;
}
+#define GET_EFI_CONFIG_TABLE(bits) \
+static void *get_efi_config_table##bits(efi_system_table_t *_sys_table,
\
+ efi_guid_t guid)\
+{ \
+ efi_system_table_##bits##_t *sys_table; \
+ efi_config_table_##bits##_t *tables;\
+ int i; \
+ \
+ sys_table = (typeof(sys_table))_sys_table; \
+ tables = (typeof(tables))(unsigned long)sys_table->tables; \
+ \
+ for (i = 0; i < sys_table->nr_tables; i++) {\
+ if (efi_guidcmp(tables[i].guid, guid) != 0) \
+ continue; \
+ \
+ return (void *)(unsigned long)tables[i].table; \
+ } \
+ \
+ return NULL;\
+}
+GET_EFI_CONFIG_TABLE(32)
+GET_EFI_CONFIG_TABLE(64)
+
void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid)
{
- efi_config_table_t *tables = (efi_config_table_t *)sys_table->tables;
- int i;
-
- for (i = 0; i < sys_table->nr_tables; i++) {
- if (efi_guidcmp(tables[i].guid, guid) != 0)
- continue;
-
- return (void *)tables[i].table;
- }
-
- return NULL;
+ if (efi_is_64bit())
+ return get_efi_config_table64(sys_table, guid);
+ else
+ return get_efi_config_table32(sys_table, guid);
}
--
2.22.0
Re: 5.3 boot regression caused by 5.3 TPM changes
Hi,
On 05-08-19 18:01, Ard Biesheuvel wrote:
On Sun, 4 Aug 2019 at 19:12, Hans de Goede wrote:
Hi,
On 04-08-19 17:33, Ard Biesheuvel wrote:
Hi Hans,
On Sun, 4 Aug 2019 at 13:00, Hans de Goede wrote:
Hi All,
While testing 5.3-rc2 on an Irbis TW90 Intel Cherry Trail based
tablet I noticed that it does not boot on this device.
A git bisect points to commit 166a2809d65b ("tpm: Don't duplicate
events from the final event log in the TCG2 log")
And I can confirm that reverting just that single commit makes
the TW90 boot again.
This machine uses AptIO firmware with base component versions
of: UEFI 2.4 PI 1.3. I've tried to reproduce the problem on
a Teclast X80 Pro which is also CHT based and also uses AptIO
firmware with the same base components. But it does not reproduce
there. Neither does the problem reproduce on a CHT tablet using
InsideH20 based firmware.
Note that these devices have a software/firmware TPM-2.0
implementation, they do not have an actual TPM chip.
Comparing TPM firmware setting between the 2 AptIO based
tablets the settings are identical, but the troublesome
TW90 does have some more setting then the X80, it has
the following settings which are not shown on the X80:
Active PCR banks: SHA-1 (read only)
Available PCR banks:SHA-1,SHA256 (read only)
TPM2.0 UEFI SPEC version: TCG_2 (other possible setting: TCG_1_2
Physical Presence SPEC ver: 1.2 (other possible setting: 1.3)
I have the feeling that at least the first 2 indicate that
the previous win10 installation has actually used the
TPM, where as on the X80 the TPM is uninitialized.
Note this is just a hunch I could be completely wrong.
I would be happy to run any commands to try and debug this
or to build a kernel with some patches to gather more info.
Note any kernel patches to printk some debug stuff need
to be based on 5.3 with 166a2809d65b reverted, without that
reverted the device will not boot, and thus I cannot collect
logs without it reverted.
Are you booting a 64-bit kernel on 32-bit firmware?
Yes you are right, I must say that this is somewhat surprising
most Cherry Trail devices do use 64 bit firmware (where as Bay Trail
typically uses 32 bit). But I just checked efibootmgr output and it
says it is booting: \EFI\FEDORA\SHIMIA32.EFI so yeah 32 bit firmware.
Recent Fedora releases take care of this so seamlessly I did not
even realize...
OK, so we'll have to find out how this patch affects 64-bit code
running on 32-bit firmware. The only EFI call in that patch is
get_config_table(), which is not actually a EFI boot service call but
a EFI stub helper that parses the config table array in the EFI system
table.
Ok, the problem indeed is the new get_efi_config_table() helper, it
does not make any calls, but it does interpret some structs which
have different sized members depending on if the firmware is 32 or 64 bit.
I've prepared a patch fixing this which I will send out after this mail.
Regards,
Hans
Re: 5.3 boot regression caused by 5.3 TPM changes
Hi,
On 07-08-19 22:13, Hans de Goede wrote:
Hi,
On 07-08-19 21:58, Hans de Goede wrote:
Hi,
On 05-08-19 18:01, Ard Biesheuvel wrote:
On Sun, 4 Aug 2019 at 19:12, Hans de Goede wrote:
Hi,
On 04-08-19 17:33, Ard Biesheuvel wrote:
Hi Hans,
On Sun, 4 Aug 2019 at 13:00, Hans de Goede wrote:
Hi All,
While testing 5.3-rc2 on an Irbis TW90 Intel Cherry Trail based
tablet I noticed that it does not boot on this device.
A git bisect points to commit 166a2809d65b ("tpm: Don't duplicate
events from the final event log in the TCG2 log")
And I can confirm that reverting just that single commit makes
the TW90 boot again.
This machine uses AptIO firmware with base component versions
of: UEFI 2.4 PI 1.3. I've tried to reproduce the problem on
a Teclast X80 Pro which is also CHT based and also uses AptIO
firmware with the same base components. But it does not reproduce
there. Neither does the problem reproduce on a CHT tablet using
InsideH20 based firmware.
Note that these devices have a software/firmware TPM-2.0
implementation, they do not have an actual TPM chip.
Comparing TPM firmware setting between the 2 AptIO based
tablets the settings are identical, but the troublesome
TW90 does have some more setting then the X80, it has
the following settings which are not shown on the X80:
Active PCR banks: SHA-1 (read only)
Available PCR banks: SHA-1,SHA256 (read only)
TPM2.0 UEFI SPEC version: TCG_2 (other possible setting: TCG_1_2
Physical Presence SPEC ver: 1.2 (other possible setting: 1.3)
I have the feeling that at least the first 2 indicate that
the previous win10 installation has actually used the
TPM, where as on the X80 the TPM is uninitialized.
Note this is just a hunch I could be completely wrong.
I would be happy to run any commands to try and debug this
or to build a kernel with some patches to gather more info.
Note any kernel patches to printk some debug stuff need
to be based on 5.3 with 166a2809d65b reverted, without that
reverted the device will not boot, and thus I cannot collect
logs without it reverted.
Are you booting a 64-bit kernel on 32-bit firmware?
Yes you are right, I must say that this is somewhat surprising
most Cherry Trail devices do use 64 bit firmware (where as Bay Trail
typically uses 32 bit). But I just checked efibootmgr output and it
says it is booting: \EFI\FEDORA\SHIMIA32.EFI so yeah 32 bit firmware.
Recent Fedora releases take care of this so seamlessly I did not
even realize...
OK, so we'll have to find out how this patch affects 64-bit code
running on 32-bit firmware.
I was not sure this really is a 32 bit firmware issue, as I believed
I saw 5.3 running fine on other 32 bit firmware devices, so I tried
this on another device with 32 bit UEFI and you're right this is a
32 bit issue.
The only EFI call in that patch is
get_config_table(), which is not actually a EFI boot service call but
a EFI stub helper that parses the config table array in the EFI system
table.
Well get_efi_config_table() is a new function in 5.3, introduced
specifically for the 166a2809d65b ("tpm: Don't duplicate events from the
final event log in the TCG2 log") commit.
It was introduced in commit 82d736ac56d7 ("Abstract out support for
locating an EFI config table") and after taking a good look at this
commit I'm pretty confident to say that the new get_efi_config_table()
function is the problem, as it is broken in multiple ways.
In itself the new get_efi_config_table() is just factoring out some
code used in drivers/firmware/efi/libstub/fdt.c into a new helper
for reuse and not making any functional changes to the factored out
code.
The problem is that the old code which it factors out contains a number
of assumptions which are true in the get_fdt() context from which it
was called but are not true when used in more generic code as is done
from the 166a2809d65b ("tpm: Don't duplicate events from the
final event log in the TCG2 log") commit.
There are 2 problems with the new get_efi_config_table() function:
1) sys_table->tables contains a physical address, we cannot just
cast that to a pointer and deref it, it needs to be early_memremap-ed
and then we deref the mapping. I'm somewhat amazed that this works
at all for the 64 bit case, but apparently it does.
2) sys_table->tables points to either an array of either
efi_config_table_64_t structd or an array of efi_config_table_32_t
structs. efi_config_table_t is a generic type for storing information
when parsing it should NOT be used for reading the actual tables
as they come from the firmware when parsing! Now efi_config_table_t
happens to be an exact match for efi_config_table_64_t when building
an x86_64 kernel, so this happens to work for the 64 bit firmware case.
The properway to deal with this would be to use the existing
efi_config_parse_tables() functionality from drivers/firmware/efi/efi.c
by adding entry for the LINUX_EFI_TPM_FINAL_LOG_GUID to the
common_tables[]
Re: 5.3 boot regression caused by 5.3 TPM changes
Hi,
On 07-08-19 21:58, Hans de Goede wrote:
Hi,
On 05-08-19 18:01, Ard Biesheuvel wrote:
On Sun, 4 Aug 2019 at 19:12, Hans de Goede wrote:
Hi,
On 04-08-19 17:33, Ard Biesheuvel wrote:
Hi Hans,
On Sun, 4 Aug 2019 at 13:00, Hans de Goede wrote:
Hi All,
While testing 5.3-rc2 on an Irbis TW90 Intel Cherry Trail based
tablet I noticed that it does not boot on this device.
A git bisect points to commit 166a2809d65b ("tpm: Don't duplicate
events from the final event log in the TCG2 log")
And I can confirm that reverting just that single commit makes
the TW90 boot again.
This machine uses AptIO firmware with base component versions
of: UEFI 2.4 PI 1.3. I've tried to reproduce the problem on
a Teclast X80 Pro which is also CHT based and also uses AptIO
firmware with the same base components. But it does not reproduce
there. Neither does the problem reproduce on a CHT tablet using
InsideH20 based firmware.
Note that these devices have a software/firmware TPM-2.0
implementation, they do not have an actual TPM chip.
Comparing TPM firmware setting between the 2 AptIO based
tablets the settings are identical, but the troublesome
TW90 does have some more setting then the X80, it has
the following settings which are not shown on the X80:
Active PCR banks: SHA-1 (read only)
Available PCR banks: SHA-1,SHA256 (read only)
TPM2.0 UEFI SPEC version: TCG_2 (other possible setting: TCG_1_2
Physical Presence SPEC ver: 1.2 (other possible setting: 1.3)
I have the feeling that at least the first 2 indicate that
the previous win10 installation has actually used the
TPM, where as on the X80 the TPM is uninitialized.
Note this is just a hunch I could be completely wrong.
I would be happy to run any commands to try and debug this
or to build a kernel with some patches to gather more info.
Note any kernel patches to printk some debug stuff need
to be based on 5.3 with 166a2809d65b reverted, without that
reverted the device will not boot, and thus I cannot collect
logs without it reverted.
Are you booting a 64-bit kernel on 32-bit firmware?
Yes you are right, I must say that this is somewhat surprising
most Cherry Trail devices do use 64 bit firmware (where as Bay Trail
typically uses 32 bit). But I just checked efibootmgr output and it
says it is booting: \EFI\FEDORA\SHIMIA32.EFI so yeah 32 bit firmware.
Recent Fedora releases take care of this so seamlessly I did not
even realize...
OK, so we'll have to find out how this patch affects 64-bit code
running on 32-bit firmware.
I was not sure this really is a 32 bit firmware issue, as I believed
I saw 5.3 running fine on other 32 bit firmware devices, so I tried
this on another device with 32 bit UEFI and you're right this is a
32 bit issue.
The only EFI call in that patch is
get_config_table(), which is not actually a EFI boot service call but
a EFI stub helper that parses the config table array in the EFI system
table.
Well get_efi_config_table() is a new function in 5.3, introduced
specifically for the 166a2809d65b ("tpm: Don't duplicate events from the
final event log in the TCG2 log") commit.
It was introduced in commit 82d736ac56d7 ("Abstract out support for
locating an EFI config table") and after taking a good look at this
commit I'm pretty confident to say that the new get_efi_config_table()
function is the problem, as it is broken in multiple ways.
In itself the new get_efi_config_table() is just factoring out some
code used in drivers/firmware/efi/libstub/fdt.c into a new helper
for reuse and not making any functional changes to the factored out
code.
The problem is that the old code which it factors out contains a number
of assumptions which are true in the get_fdt() context from which it
was called but are not true when used in more generic code as is done
from the 166a2809d65b ("tpm: Don't duplicate events from the
final event log in the TCG2 log") commit.
There are 2 problems with the new get_efi_config_table() function:
1) sys_table->tables contains a physical address, we cannot just
cast that to a pointer and deref it, it needs to be early_memremap-ed
and then we deref the mapping. I'm somewhat amazed that this works
at all for the 64 bit case, but apparently it does.
2) sys_table->tables points to either an array of either
efi_config_table_64_t structd or an array of efi_config_table_32_t
structs. efi_config_table_t is a generic type for storing information
when parsing it should NOT be used for reading the actual tables
as they come from the firmware when parsing! Now efi_config_table_t
happens to be an exact match for efi_config_table_64_t when building
an x86_64 kernel, so this happens to work for the 64 bit firmware case.
The properway to deal with this would be to use the existing
efi_config_parse_tables() functionality from drivers/firmware/efi/efi.c
by adding entry for the LINUX_EFI_TPM_FINAL_LOG_GUID to the
common_tables[] array in drivers/firmware/efi/efi.c and make that
Re: 5.3 boot regression caused by 5.3 TPM changes
Hi,
On 05-08-19 18:01, Ard Biesheuvel wrote:
On Sun, 4 Aug 2019 at 19:12, Hans de Goede wrote:
Hi,
On 04-08-19 17:33, Ard Biesheuvel wrote:
Hi Hans,
On Sun, 4 Aug 2019 at 13:00, Hans de Goede wrote:
Hi All,
While testing 5.3-rc2 on an Irbis TW90 Intel Cherry Trail based
tablet I noticed that it does not boot on this device.
A git bisect points to commit 166a2809d65b ("tpm: Don't duplicate
events from the final event log in the TCG2 log")
And I can confirm that reverting just that single commit makes
the TW90 boot again.
This machine uses AptIO firmware with base component versions
of: UEFI 2.4 PI 1.3. I've tried to reproduce the problem on
a Teclast X80 Pro which is also CHT based and also uses AptIO
firmware with the same base components. But it does not reproduce
there. Neither does the problem reproduce on a CHT tablet using
InsideH20 based firmware.
Note that these devices have a software/firmware TPM-2.0
implementation, they do not have an actual TPM chip.
Comparing TPM firmware setting between the 2 AptIO based
tablets the settings are identical, but the troublesome
TW90 does have some more setting then the X80, it has
the following settings which are not shown on the X80:
Active PCR banks: SHA-1 (read only)
Available PCR banks:SHA-1,SHA256 (read only)
TPM2.0 UEFI SPEC version: TCG_2 (other possible setting: TCG_1_2
Physical Presence SPEC ver: 1.2 (other possible setting: 1.3)
I have the feeling that at least the first 2 indicate that
the previous win10 installation has actually used the
TPM, where as on the X80 the TPM is uninitialized.
Note this is just a hunch I could be completely wrong.
I would be happy to run any commands to try and debug this
or to build a kernel with some patches to gather more info.
Note any kernel patches to printk some debug stuff need
to be based on 5.3 with 166a2809d65b reverted, without that
reverted the device will not boot, and thus I cannot collect
logs without it reverted.
Are you booting a 64-bit kernel on 32-bit firmware?
Yes you are right, I must say that this is somewhat surprising
most Cherry Trail devices do use 64 bit firmware (where as Bay Trail
typically uses 32 bit). But I just checked efibootmgr output and it
says it is booting: \EFI\FEDORA\SHIMIA32.EFI so yeah 32 bit firmware.
Recent Fedora releases take care of this so seamlessly I did not
even realize...
OK, so we'll have to find out how this patch affects 64-bit code
running on 32-bit firmware.
I was not sure this really is a 32 bit firmware issue, as I believed
I saw 5.3 running fine on other 32 bit firmware devices, so I tried
this on another device with 32 bit UEFI and you're right this is a
32 bit issue.
The only EFI call in that patch is
get_config_table(), which is not actually a EFI boot service call but
a EFI stub helper that parses the config table array in the EFI system
table.
Well get_efi_config_table() is a new function in 5.3, introduced
specifically for the 166a2809d65b ("tpm: Don't duplicate events from the
final event log in the TCG2 log") commit.
It was introduced in commit 82d736ac56d7 ("Abstract out support for
locating an EFI config table") and after taking a good look at this
commit I'm pretty confident to say that the new get_efi_config_table()
function is the problem, as it is broken in multiple ways.
In itself the new get_efi_config_table() is just factoring out some
code used in drivers/firmware/efi/libstub/fdt.c into a new helper
for reuse and not making any functional changes to the factored out
code.
The problem is that the old code which it factors out contains a number
of assumptions which are true in the get_fdt() context from which it
was called but are not true when used in more generic code as is done
from the 166a2809d65b ("tpm: Don't duplicate events from the
final event log in the TCG2 log") commit.
There are 2 problems with the new get_efi_config_table() function:
1) sys_table->tables contains a physical address, we cannot just
cast that to a pointer and deref it, it needs to be early_memremap-ed
and then we deref the mapping. I'm somewhat amazed that this works
at all for the 64 bit case, but apparently it does.
2) sys_table->tables points to either an array of either
efi_config_table_64_t structd or an array of efi_config_table_32_t
structs. efi_config_table_t is a generic type for storing information
when parsing it should NOT be used for reading the actual tables
as they come from the firmware when parsing! Now efi_config_table_t
happens to be an exact match for efi_config_table_64_t when building
an x86_64 kernel, so this happens to work for the 64 bit firmware case.
The properway to deal with this would be to use the existing
efi_config_parse_tables() functionality from drivers/firmware/efi/efi.c
by adding entry for the LINUX_EFI_TPM_FINAL_LOG_GUID to the
common_tables[] array in drivers/firmware/efi/efi.c and make that
entry store the table address (if found) in a
Re: [PATCH v1] Export Runtime Configuration Interface table to sysfs
On Thu, Jul 11, 2019 at 11:00:37PM +, Limonciello, Mario wrote: > > -Original Message- > > From: K, Narendra > > Sent: Wednesday, July 10, 2019 11:59 AM > > To: linux-efi@vger.kernel.org; ard.biesheu...@linaro.org; pjo...@redhat.com > > Cc: K, Narendra; Hayes, Stuart; Limonciello, Mario > > Subject: [PATCH v1] Export Runtime Configuration Interface table to sysfs > > > > From: Narendra K > > > > System firmware advertises the address of the 'Runtime Configuration > > Interface > > table version 2 (RCI2)' via an EFI Configuration Table entry. This code > > retrieves > > the RCI2 table from the address and exports it to sysfs as a binary > > attribute 'rci2' > > under /sys/firmware/efi/tables directory. > > The approach adopted is similar to the attribute 'DMI' under > > /sys/firmware/dmi/tables. > > > > RCI2 table contains BIOS HII in XML format and is used to populate BIOS > > setup > > page in Dell EMC OpenManage Server Administrator tool. > > The BIOS setup page contains BIOS tokens which can be configured. > > > > Signed-off-by: Narendra K > > Reviewed-by: Mario Limonciello Hi Ard, Does the version 1 of the patch look good ? Please share your thoughts. > > > --- > > Hi Ard, the review comment in the v0 version of the patch suggested that the > > kconfig symbol be set to Y for X86. I made a change to the suggestion. > > In the v1 version, I have set the symbol to N by default and added a note > > to the > > help section to make it Y for Dell EMC PowerEdge systems. If it needs to be > > changed, I will resubmit the patch after changing it to implement the > > suggestion. > > > > The patch is created on 'next' branch of efi tree. > > > > v0 -> v1: > > - Introduced a new Kconfig symbol CONFIG_EFI_RCI2_TABLE and compile > > RCI2 table support if it is set. Set the symbol to N by default. > > - Removed calling 'efi_rci2_sysfs_init' from drivers/firmware/efi/efi.c and > > made > > it a 'late_initcall' in drivers/firmware/efi/rci2_table.c. > > Removed the function declaration from include/linux/efi.h. > > > > RFC -> v0: > > - Removed rci2 table from struct efi and defined it in rci2_table.c similar > > to the > > way uv_systab_phys is defined in arch/x86/platform/uv/bios_uv.c > > - Removed the oem_tables array and added rci2 to common_tables array > > - Removed the string 'rci2' from the common_tables array so that it is not > > printed in dmesg. > > - Merged function 'efi_rci2_table_init' into 'efi_rci2_sysfs_init' function > > to avoid > > calling early_memremap/unmap functions. -- With regards, Narendra K
