[PATCH v2] efi/efi_test: lock down /dev/efi_test and require CAP_SYS_ADMIN
The driver exposes EFI runtime services to user-space through an IOCTL
interface, calling the EFI services function pointers directly without
using the efivar API.
Disallow access to the /dev/efi_test character device when the kernel is
locked down to prevent arbitrary user-space to call EFI runtime services.
Also require CAP_SYS_ADMIN to open the chardev to prevent unprivileged
users to call the EFI runtime services, instead of just relying on the
chardev file mode bits for this.
The main user of this driver is the fwts [0] tool that already checks if
the effective user ID is 0 and fails otherwise. So this change shouldn't
cause any regression to this tool.
[0]: https://wiki.ubuntu.com/FirmwareTestSuite/Reference/uefivarinfo
Signed-off-by: Javier Martinez Canillas
Acked-by: Laszlo Ersek
---
Changes in v2:
- Also disable /dev/efi_test access when the kernel is locked down as
suggested by Matthew Garrett.
- Add Acked-by tag from Laszlo Ersek.
drivers/firmware/efi/test/efi_test.c | 8
include/linux/security.h | 1 +
security/lockdown/lockdown.c | 1 +
3 files changed, 10 insertions(+)
diff --git a/drivers/firmware/efi/test/efi_test.c
b/drivers/firmware/efi/test/efi_test.c
index 877745c3aaf..7baf48c01e7 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -717,6 +718,13 @@ static long efi_test_ioctl(struct file *file, unsigned int
cmd,
static int efi_test_open(struct inode *inode, struct file *file)
{
+ int ret = security_locked_down(LOCKDOWN_EFI_TEST);
+
+ if (ret)
+ return ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
/*
* nothing special to do here
* We do accept multiple open files at the same time as we
diff --git a/include/linux/security.h b/include/linux/security.h
index a8d59d612d2..9df7547afc0 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -105,6 +105,7 @@ enum lockdown_reason {
LOCKDOWN_NONE,
LOCKDOWN_MODULE_SIGNATURE,
LOCKDOWN_DEV_MEM,
+ LOCKDOWN_EFI_TEST,
LOCKDOWN_KEXEC,
LOCKDOWN_HIBERNATION,
LOCKDOWN_PCI_ACCESS,
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 8a10b43daf7..40b790536de 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -20,6 +20,7 @@ static const char *const
lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
[LOCKDOWN_NONE] = "none",
[LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading",
[LOCKDOWN_DEV_MEM] = "/dev/mem,kmem,port",
+ [LOCKDOWN_EFI_TEST] = "/dev/efi_test access",
[LOCKDOWN_KEXEC] = "kexec of unsigned images",
[LOCKDOWN_HIBERNATION] = "hibernation",
[LOCKDOWN_PCI_ACCESS] = "direct PCI access",
--
2.21.0
[PATCH] efi/efi_test: require CAP_SYS_ADMIN to open the chardev
The driver exposes EFI runtime services to user-space through an IOCTL
interface, calling the EFI services function pointers directly without
using the efivar API.
Among other things the driver allows to read and write EFI variables and
doesn't require CAP_SYS_ADMIN as is the case for the efivar sysfs driver.
To make sure that unprivileged users won't be able to access the exposed
EFI runtime services require CAP_SYS_ADMIN to open the character device,
instead of just relying on the chardev file mode bits to prevent this.
The main user of this driver is the fwts [0] tool, that already checks if
the effective user ID is 0 and fails otherwise. So adding the requirement
won't cause any regression to this tool.
[0]: https://wiki.ubuntu.com/FirmwareTestSuite/Reference/uefivarinfo
Signed-off-by: Javier Martinez Canillas
---
Hello,
We want to enable this driver in the Fedora kernel for testing purposes.
Currently the GetVariable() UEFI runtime service is used (through the
efivar sysfs interface) to test that OVMF is able to enter into SMM.
But there's a proposal to add a UEFI variable cache outside of SMM, to
speedup GetVariable() calls. So the plan is to call QueryVariableInfo()
instead that's also read-only and sufficiently infrequently called that
is not planned to be cached anytime soon.
Building the efi_test module will allow us to call this EFI service by
using the fwts uefivarinfo test. But we are worried that enabling this
driver could open a new attack vector and lead to unprivileged users
accessing the exposed EFI services.
This is also consistent with the efivar driver since it also requires
the CAP_SYS_ADMIN capability.
Best regards,
Javier
drivers/firmware/efi/test/efi_test.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/firmware/efi/test/efi_test.c
b/drivers/firmware/efi/test/efi_test.c
index 877745c3aaf..81de7374c42 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -717,6 +717,8 @@ static long efi_test_ioctl(struct file *file, unsigned int
cmd,
static int efi_test_open(struct inode *inode, struct file *file)
{
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
/*
* nothing special to do here
* We do accept multiple open files at the same time as we
--
2.21.0
Re: [PATCH 1/1] efi/libstub: tpm: zero initialize pointer variables for mixed mode
[adding linux-integrity and tpmdd-devel since this was discussed in these ML too] On 03/13/2018 03:09 PM, Ard Biesheuvel wrote: > As reported by Jeremy, running the new TPM libstub code in mixed mode > (i.e., 64-bit kernel on 32-bit UEFI) results in hangs when invoking > the TCG2 protocol, or when accessing the log_tbl pool allocation. > > The reason turns out to be that in both cases, the 64-bit pointer > variables are not fully initialized by the 32-bit EFI code, and so > we should take care to zero initialize these variables beforehand, > or we'll end up dereferencing bogus pointers. > > Reported-by: Jeremy Cline <jer...@jcline.org> > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org> > --- > drivers/firmware/efi/libstub/tpm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/tpm.c > b/drivers/firmware/efi/libstub/tpm.c > index da661bf8cb96..13c1edd37e96 100644 > --- a/drivers/firmware/efi/libstub/tpm.c > +++ b/drivers/firmware/efi/libstub/tpm.c > @@ -68,11 +68,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t > *sys_table_arg) > efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID; > efi_status_t status; > efi_physical_addr_t log_location, log_last_entry; > - struct linux_efi_tpm_eventlog *log_tbl; > + struct linux_efi_tpm_eventlog *log_tbl = NULL; > unsigned long first_entry_addr, last_entry_addr; > size_t log_size, last_entry_size; > efi_bool_t truncated; > - void *tcg2_protocol; > + void *tcg2_protocol = NULL; > > status = efi_call_early(locate_protocol, _guid, NULL, > _protocol); > Looks good to me. Reviewed-by: Javier Martinez Canillas <javi...@redhat.com> Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Regression from efi: call get_event_log before ExitBootServices
Hi Hans, On 03/07/2018 12:16 PM, Hans de Goede wrote: > Hi, > > On 07-03-18 09:41, Thiebaud Weksteen wrote: >> Hi, >> >> Thanks for testing and sending this report! This patch relies heavily on >> the functions exposed by the firmware. My first guess would be that some of >> these may not be implemented correctly by the manufacturer. >> >> Could you share more information on this specific device? > > I've the same device as Jeremy, but I just tried a 4.16-rc3 kernel > and I'm not seeing this problem, BIOS settings all default (I loaded > the BIOS defaults to make sure). > >> Do you have any link to the manufacturer website (I found [1] but it is >> based on an ARM CPU)? >> Do you have the option to update your firmware? Is a copy of the firmware >> available from the manufacturer? > > This is a really cheap Windows tablet which was given away for free in > the Netherlands with some home-schooling language courses, or something > similar. > > Both mine and Jeremy tablets come from a website in the Netherlands > where people can buy/sell used goods. > > Most relevant for this discussion I guess is that this device is > based on a Bay Trail Z3735G SoC, on which according to the internets: > https://embedded.communities.intel.com/thread/7868 > > The TPM 2.0 it contains is implemented as part of the TXE firmware. > > Since I cannot reproduce I'm thinking that maybe Jeremy actually has > some log messages in the TPM log, where as mine is empty. Is there a > way to make sure some messages are in there? > The UEFI firmware does some measurements and so does shim. So you should have some event logs. What version of shim are you using? And also would be good to know if it's the same shim version that Jeremy is using. > Regards, > > Hans > Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [tpmdd-devel] [PATCH v2 2/3] efi: call get_event_log before ExitBootServices
On 09/18/2017 02:11 PM, Thiebaud Weksteen wrote:
> On Thu, Sep 14, 2017 at 12:24 PM, Javier Martinez Canillas
> <javi...@redhat.com> wrote:
>> On 09/11/2017 12:00 PM, Thiebaud Weksteen via tpmdd-devel wrote:
[snip]
>>> +
>>> + if (status != EFI_SUCCESS) {
>>> + efi_printk(sys_table_arg,
>>> +"Unable to allocate memory for event log\n");
>>> + return;
>>> + }
>>
>> If this fails or any previous error that will prevent the event log table +
>> logs
>> to be allocated, shouldn't tpm_read_log_efi() be notified somehow? Since
>> AFAICT
>> it will still try to access them even if the EFI allocate_pool did not
>> succeed.
>>
>
> The implicit part that covers this case is in
> drivers/firmware/efi/efi.c. The match_config_table function will go
> through all the installed configuration tables and only fill up the
> associated member of the efi structure if it exists. In this case,
> .tpm_log will remains at EFI_INVALID_TABLE_ADDR unless
> efi_call_early(install_configuration_table, ...) is called. So no
> further processing is to be expected should the allocation failed.
>
I see, missed that. Thanks a lot for the explanation.
>>> + */
>>> +int __init efi_tpm_eventlog_init(void)
>>> +{
>>> + struct linux_efi_tpm_eventlog *tbl;
>>> + unsigned int tbl_size;
>>> +
>>
>> The functions efi_retrieve_tpm2_eventlog_1_2() and tpm_read_log_efi() are
>> using
>> log_tbl as variable name, so I would use it here too for consistency.
>>
>
> Done.
>
>>> + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR)
>>> + return 0;
>>> +
>>> + tbl = early_memremap(efi.tpm_log, sizeof(*tbl));
>>> + if (!tbl) {
>>> + pr_err("Failed to map TPM Event Log table @ 0x%lx\n",
>>> + efi.tpm_log);
>>> + return -ENOMEM;
>>> + }
>>> +
>>
>> Same question than before, if this fails then the table + logs memory won't
>> be
>> reserved but tpm_read_log_efi() will still try to access it. I'm not sure
>> what
>> is the correct way to notify though, maybe setting efi.tpm_log to 0 and then
>> in
>> tpm_read_log_efi() check efi.tpm_log for 0 or EFI_INVALID_TABLE_ADDR instead?
>>
>
> That's right. To keep it simple, it might be easier to just set
> tpm_log to EFI_INVALID_TABLE_ADDR if that happens. Added in the next
> version of the patch set.
>
Right. I wasn't sure if you wanted to distinguish between the two cases, but
that
is simpler indeed.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [tpmdd-devel] [PATCH v2 2/3] efi: call get_event_log before ExitBootServices
On 09/11/2017 12:00 PM, Thiebaud Weksteen via tpmdd-devel wrote:
> With TPM 2.0 specification, the event logs may only be accessible by
> calling an EFI Boot Service. Modify the EFI stub to copy the log area to
> a new Linux-specific EFI configuration table so it remains accessible
> once booted.
>
> When calling this service, it is possible to specify the expected format
> of the logs: TPM 1.2 (SHA1) or TPM 2.0 ("Crypto Agile"). For now, only the
> first format is retrieved.
>
> Signed-off-by: Thiebaud Weksteen <tw...@google.com>
> ---
[snip]
> +void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> +{
[snip]
> +
> + /* Allocate space for the logs and copy them. */
> + status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> + sizeof(*log_tbl) + log_size,
> + (void **) _tbl);
> +
> + if (status != EFI_SUCCESS) {
> + efi_printk(sys_table_arg,
> +"Unable to allocate memory for event log\n");
> + return;
> + }
If this fails or any previous error that will prevent the event log table + logs
to be allocated, shouldn't tpm_read_log_efi() be notified somehow? Since AFAICT
it will still try to access them even if the EFI allocate_pool did not succeed.
> + */
> +int __init efi_tpm_eventlog_init(void)
> +{
> + struct linux_efi_tpm_eventlog *tbl;
> + unsigned int tbl_size;
> +
The functions efi_retrieve_tpm2_eventlog_1_2() and tpm_read_log_efi() are using
log_tbl as variable name, so I would use it here too for consistency.
> + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR)
> + return 0;
> +
> + tbl = early_memremap(efi.tpm_log, sizeof(*tbl));
> + if (!tbl) {
> + pr_err("Failed to map TPM Event Log table @ 0x%lx\n",
> + efi.tpm_log);
> + return -ENOMEM;
> + }
> +
Same question than before, if this fails then the table + logs memory won't be
reserved but tpm_read_log_efi() will still try to access it. I'm not sure what
is the correct way to notify though, maybe setting efi.tpm_log to 0 and then in
tpm_read_log_efi() check efi.tpm_log for 0 or EFI_INVALID_TABLE_ADDR instead?
> + tbl_size = sizeof(*tbl) + tbl->size;
> + memblock_reserve(efi.tpm_log, tbl_size);
> + early_memunmap(tbl, sizeof(*tbl));
> + return 0;
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [tpmdd-devel] [PATCH v2 0/3] Call GetEventLog before ExitBootServices
Hello Thiebaud,
On 09/11/2017 12:00 PM, Thiebaud Weksteen via tpmdd-devel wrote:
> With TPM 1.2, the ACPI table ("TCPA") has two fields to recover the Event Log
> Area (LAML and LASA). These logs are useful to understand and rebuild the
> final values of PCRs.
>
> With TPM 2.0, the ACPI table ("TPM2") does not contain these fields anymore.
> The recommended method is now to call the GetEventLog EFI protocol before
> ExitBootServices.
>
> Implement this method within the EFI stub and create copy of the logs for the
> TPM device. This will create
> /sys/kernel/security/tpm0/binary_bios_measurements
> for TPM 2.0 devices (similarly to the current behaviour for TPM 1.2 devices).
>
I've tested your patches on a system with an Intel PTT firmware based TPM2.0 and
the measurements securityfs entry was correctly created and was able to read it:
$ cat /sys/class/tpm/tpm0/device/description
TPM 2.0 Device
$ hexdump /sys/kernel/security/tpm0/binary_bios_measurements | head -n2
000 0008 f504 15a0 1810 bf44
010 63d0 4fdb b8a4 f278 8dc7 c8aa 0014 0000
So please feel free to add:
Tested-by: Javier Martinez Canillas <javi...@redhat.com>
I also reviewed the patches and look good to me, I have just one question for
patch #2, but I'll comment there.
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
