Re: [PATCH v3 RESEND] efi: Do not import certificates from UEFI Secure Boot for T2 Macs

[Aditya Garg]

> On 12-Apr-2022, at 6:02 PM, Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> 
> On Sun, 2022-04-10 at 10:49 +0000, Aditya Garg wrote:
>> From: Aditya Garg <gargaditya08@xxxxxxxx>
>> 
>> On T2 Macs, the secure boot is handled by the T2 Chip. If enabled, only
>> macOS and Windows are allowed to boot on these machines. Thus we need to
>> disable secure boot for Linux.
> 
> The end result might be "disable secure boot for Linux", but that isn't
> what the code is actually doing. As a result of not being able to read
> or load certificates, secure boot cannot be enabled. Please be more
> precise.
I’ll fix this
> 
>> If we boot into Linux after disabling
>> secure boot, if CONFIG_LOAD_UEFI_KEYS is enabled, EFI Runtime services
>> fail to start, with the following logs in dmesg
>> 
>> Call Trace:
>> <TASK>
>> page_fault_oops+0x4f/0x2c0
>> ? search_bpf_extables+0x6b/0x80
>> ? search_module_extables+0x50/0x80
>> ? search_exception_tables+0x5b/0x60
>> kernelmode_fixup_or_oops+0x9e/0x110
>> __bad_area_nosemaphore+0x155/0x190
>> bad_area_nosemaphore+0x16/0x20
>> do_kern_addr_fault+0x8c/0xa0
>> exc_page_fault+0xd8/0x180
>> asm_exc_page_fault+0x1e/0x30
>> (Removed some logs from here)
>> ? __efi_call+0x28/0x30
>> ? switch_mm+0x20/0x30
>> ? efi_call_rts+0x19a/0x8e0
>> ? process_one_work+0x222/0x3f0
>> ? worker_thread+0x4a/0x3d0
>> ? kthread+0x17a/0x1a0
>> ? process_one_work+0x3f0/0x3f0
>> ? set_kthread_struct+0x40/0x40
>> ? ret_from_fork+0x22/0x30
>> </TASK>
>> ---[ end trace 1f82023595a5927f ]---
>> efi: Froze efi_rts_wq and disabled EFI Runtime Services
>> integrity: Couldn't get size: 0x8000000000000015
>> integrity: MODSIGN: Couldn't get UEFI db list
>> efi: EFI Runtime Services are disabled!
>> integrity: Couldn't get size: 0x8000000000000015
>> integrity: Couldn't get UEFI dbx list
>> integrity: Couldn't get size: 0x8000000000000015
>> integrity: Couldn't get mokx list
>> integrity: Couldn't get size: 0x80000000
>> 
>> This patch prevents querying of these UEFI variables, since these Macs
>> seem to use a non-standard EFI hardware
>> 
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Aditya Garg <gargaditya08@xxxxxxxx>
>> ---
>> v2 :- Reduce code size of the table.
>> V3 :- Close the brackets which were left open by mistake.
>> .../platform_certs/keyring_handler.h | 8 ++++
>> security/integrity/platform_certs/load_uefi.c | 48 +++++++++++++++++++
>> 2 files changed, 56 insertions(+)
>> 
>> diff --git a/security/integrity/platform_certs/keyring_handler.h b/security/integrity/platform_certs/keyring_handler.h
>> index 2462bfa08..cd06bd607 100644
>> --- a/security/integrity/platform_certs/keyring_handler.h
>> +++ b/security/integrity/platform_certs/keyring_handler.h
>> @@ -30,3 +30,11 @@ efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type);
>> efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type);
>> 
>> #endif
>> +
>> +#ifndef UEFI_QUIRK_SKIP_CERT
>> +#define UEFI_QUIRK_SKIP_CERT(vendor, product) \
>> +		 .matches = { \
>> +			DMI_MATCH(DMI_BOARD_VENDOR, vendor), \
>> +			DMI_MATCH(DMI_PRODUCT_NAME, product), \
>> +		},
>> +#endif
>> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
>> index 08b6d12f9..f246c8732 100644
>> --- a/security/integrity/platform_certs/load_uefi.c
>> +++ b/security/integrity/platform_certs/load_uefi.c
>> @@ -3,6 +3,7 @@
>> #include <linux/kernel.h>
>> #include <linux/sched.h>
>> #include <linux/cred.h>
>> +#include <linux/dmi.h>
>> #include <linux/err.h>
>> #include <linux/efi.h>
>> #include <linux/slab.h>
>> @@ -12,6 +13,32 @@
>> #include "../integrity.h"
>> #include "keyring_handler.h"
>> 
>> +/* Apple Macs with T2 Security chip don't support these UEFI variables.
> 
> Please refer to Documentation/process/coding-style.rst for the format
> of multi-line comments.
Done
> 
>> + * The T2 chip manages the Secure Boot and does not allow Linux to boot
>> + * if it is turned on. If turned off, an attempt to get certificates
>> + * causes a crash, so we simply return 0 for them in each function.
>> + */
>> +
> 
> No need for a blank line here.
All blanks removed
> 
>> +static const struct dmi_system_id uefi_skip_cert[] = {
>> +
> No need for a blank here either.
> 
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro15,1") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro15,2") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro15,3") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro15,4") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro16,1") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro16,2") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro16,3") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookPro16,4") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookAir8,1") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookAir8,2") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacBookAir9,1") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacMini8,1") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "MacPro7,1") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "iMac20,1") },
>> +	{ UEFI_QUIRK_SKIP_CERT("Apple Inc.", "iMac20,2") },
>> +	{ }
>> +};
>> +
>> /*
>> * Look to see if a UEFI variable called MokIgnoreDB exists and return true if
>> * it does.
>> @@ -21,12 +48,18 @@
>> * is set, we should ignore the db variable also and the true return indicates
>> * this.
>> */
>> +
> Or here
> 
>> static __init bool uefi_check_ignore_db(void)
>> {
>> 	efi_status_t status;
>> 	unsigned int db = 0;
>> 	unsigned long size = sizeof(db);
>> 	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
>> +	const struct dmi_system_id *dmi_id;
>> +
>> +	dmi_id = dmi_first_match(uefi_skip_cert);
>> +	if (dmi_id)
>> +		return 0;
> 
> The function returns a bool. Return either "true" or "false".
> 
>> 
>> 	status = efi.get_variable(L"MokIgnoreDB", &guid, NULL, &size, &db);
>> 	return status == EFI_SUCCESS;
>> @@ -41,6 +74,11 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
>> 	unsigned long lsize = 4;
>> 	unsigned long tmpdb[4];
>> 	void *db;
>> +	const struct dmi_system_id *dmi_id;
>> +
>> +	dmi_id = dmi_first_match(uefi_skip_cert);
>> +	if (dmi_id)
>> +		return 0;
> 
> The return value here should be NULL.
> 
>> 
>> 	*status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
>> 	if (*status == EFI_NOT_FOUND)
>> @@ -85,6 +123,11 @@ static int __init load_moklist_certs(void)
>> 	unsigned long moksize;
>> 	efi_status_t status;
>> 	int rc;
>> +	const struct dmi_system_id *dmi_id;
>> +
>> +	dmi_id = dmi_first_match(uefi_skip_cert);
>> +	if (dmi_id)
>> +		return 0;
>> 
>> 	/* First try to load certs from the EFI MOKvar config table.
>> 	 * It's not an error if the MOKvar config table doesn't exist
>> @@ -138,6 +181,11 @@ static int __init load_uefi_certs(void)
>> 	unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
>> 	efi_status_t status;
>> 	int rc = 0;
>> +	const struct dmi_system_id *dmi_id;
>> +
>> +	dmi_id = dmi_first_match(uefi_skip_cert);
>> +	if (dmi_id)
>> +		return 0;
> 
> uefi_check_ignore_db(), get_cert_list(), uefi_check_ignore_db(), and
> /load_moklist_certs() are all defined all static and are gated here by
> this dmi_first_match(). There's probably no need for any of the other
> calls to dmi_first_match().
I couldn’t get you here. Could you elaborate?
> 
> Like in all the other cases, there should be some sort of message. At
> minimum, there should be a pr_info().
> 
>> 
>> 	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
>> 		return false;
> 
> thanks,
> 
> Mimi