diff mbox series

efi: Do not import certificates from UEFI Secure Boot for T2 Macs

Message ID 9D0C961D-9999-4C41-A44B-22FEAF0DAB7F@live.com
State New
Headers show
Series efi: Do not import certificates from UEFI Secure Boot for T2 Macs | expand

Commit Message

Aditya Garg Feb. 9, 2022, 2:27 p.m. UTC 
From: Aditya Garg <gargaditya08@live.com>

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. 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@vger.kernel.org
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 security/integrity/platform_certs/load_uefi.c | 122 ++++++++++++++++++
 1 file changed, 122 insertions(+)

Comments

Matthew Garrett Feb. 9, 2022, 4:49 p.m. UTC | #1 
On Wed, Feb 09, 2022 at 02:27:51PM +0000, Aditya Garg wrote:
> From: Aditya Garg <gargaditya08@live.com>
> 
> 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. 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

Which specific variable request is triggering the failure? Do any 
runtime variable accesses work on these machines?
Matthew Garrett Feb. 9, 2022, 6:35 p.m. UTC | #2 
On Wed, Feb 09, 2022 at 06:02:34PM +0000, Aditya Garg wrote:
> 
> 
> > On 09-Feb-2022, at 10:19 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > 
> > On Wed, Feb 09, 2022 at 02:27:51PM +0000, Aditya Garg wrote:
> >> From: Aditya Garg <gargaditya08@live.com>
> >> 
> >> 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. 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
> > 
> > Which specific variable request is triggering the failure? Do any 
> > runtime variable accesses work on these machines?
> Commit f5390cd0b43c2e54c7cf5506c7da4a37c5cef746 in Linus’ tree was also added to force EFI v1.1 on these machines, since v2.4, reported by them was causing kernel panics.
> 
> So, EFI 1.1 without import certificates seems to work and have been able to modify the variables, thus the remaining EFI variable accesses seem to work.

The LOAD_UEFI_KEYS code isn't doing anything special here - it's just 
trying to read some variables. If we simply disable that then the 
expectation would be that reading the same variables from userland would 
trigger the same failure. So the question is which of the variables that 
LOAD_UEFI_KEYS accesses is triggering the failure, and what's special 
about that? If it's a specific variable GUID or name that's failing, we 
should block that on Apple hardware in order to avoid issues caused by 
userland performing equivalent accesses.
Aditya Garg Feb. 10, 2022, 4:43 a.m. UTC | #3 
> ie, can you try something like this?
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index f3e54f6616f0..01cbd4811d1e 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -32,6 +32,8 @@
> #include <linux/stringify.h>
> #include <linux/workqueue.h>
> #include <linux/completion.h>
> +#include <linux/ucs2_string.h>
> +#include <linux/slab.h>
> 
> #include <asm/efi.h>
> 
> @@ -203,6 +205,21 @@ static void efi_call_rts(struct work_struct *work)
> 				       (efi_time_t *)arg2);
> 		break;
> 	case EFI_GET_VARIABLE:
> +		unsigned long utf8_name_size;
> +		char *utf8_name;
> +		char guid_str[sizeof(efi_guid_t)+1];
> +
> +		utf8_name_size = ucs2_utf8size((efi_char16_t *)arg1);
> +		utf8_name = kmalloc(utf8_name_size+1, GFP_KERNEL);
> +		if (!utf8_name) {
> +			printk(KERN_INFO "failed to allocate UTF8 buffer\n");
> +			break;
> +		}
> +
> +		ucs2_as_utf8(utf8_name, (efi_char16_t *)arg1, utf8_name_size + 1);
> +		efi_guid_to_str((efi_guid_t *)arg2, guid_str);
> +
> +		printk(KERN_INFO "Reading EFI variable %s-%s\n", utf8_name, guid_str);
> 		status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
> 				       (efi_guid_t *)arg2, (u32 *)arg3,
> 				       (unsigned long *)arg4, (void *)arg5);
> 
Hi Matthew

I haven't tested this yet (Kernel is compiling) but I have found out that this part of the code is causing a crash


static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
				  unsigned long *size, efi_status_t *status)
{
	unsigned long lsize = 4;
	unsigned long tmpdb[4];
	void *db;

	*status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
	if (*status == EFI_NOT_FOUND)
		return NULL;

	if (*status != EFI_BUFFER_TOO_SMALL) {
		pr_err("Couldn't get size: 0x%lx\n", *status);
		return NULL;
	}

	db = kmalloc(lsize, GFP_KERNEL);
	if (!db)
		return NULL;

	*status = efi.get_variable(name, guid, NULL, &lsize, db);
	if (*status != EFI_SUCCESS) {
		kfree(db);
		pr_err("Error reading db var: 0x%lx\n", *status);
		return NULL;
	}

	*size = lsize;
	return db;
}

If I remove the return 0 I had added from other 3 left functions, crash doesn't occur.

When the kernel compiles with your patch, I’ll send whatever I get.

Regards
Aditya
Aditya Garg Feb. 10, 2022, 5:49 a.m. UTC | #4 
> ie, can you try something like this?
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index f3e54f6616f0..01cbd4811d1e 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -32,6 +32,8 @@
> #include <linux/stringify.h>
> #include <linux/workqueue.h>
> #include <linux/completion.h>
> +#include <linux/ucs2_string.h>
> +#include <linux/slab.h>
> 
> #include <asm/efi.h>
> 
> @@ -203,6 +205,21 @@ static void efi_call_rts(struct work_struct *work)
> 				       (efi_time_t *)arg2);
> 		break;
> 	case EFI_GET_VARIABLE:
> +		unsigned long utf8_name_size;
> +		char *utf8_name;
> +		char guid_str[sizeof(efi_guid_t)+1];
> +
> +		utf8_name_size = ucs2_utf8size((efi_char16_t *)arg1);
> +		utf8_name = kmalloc(utf8_name_size+1, GFP_KERNEL);
> +		if (!utf8_name) {
> +			printk(KERN_INFO "failed to allocate UTF8 buffer\n");
> +			break;
> +		}
> +
> +		ucs2_as_utf8(utf8_name, (efi_char16_t *)arg1, utf8_name_size + 1);
> +		efi_guid_to_str((efi_guid_t *)arg2, guid_str);
> +
> +		printk(KERN_INFO "Reading EFI variable %s-%s\n", utf8_name, guid_str);
> 		status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
> 				       (efi_guid_t *)arg2, (u32 *)arg3,
> 				       (unsigned long *)arg4, (void *)arg5);

Looks like there is some error in this patch


drivers/firmware/efi/runtime-wrappers.c:208:3: error: a label can only be part of a statement and a declaration is not a statement
  208 |   unsigned long utf8_name_size;
      |   ^~~~~~~~
drivers/firmware/efi/runtime-wrappers.c:209:3: error: expected expression before ‘char’
  209 |   char *utf8_name;
      |   ^~~~
drivers/firmware/efi/runtime-wrappers.c:210:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
  210 |   char guid_str[sizeof(efi_guid_t)+1];
      |   ^~~~
drivers/firmware/efi/runtime-wrappers.c:213:3: error: ‘utf8_name’ undeclared (first use in this function)
  213 |   utf8_name = kmalloc(utf8_name_size+1, GFP_KERNEL);
      |   ^~~~~~~~~
drivers/firmware/efi/runtime-wrappers.c:213:3: note: each undeclared identifier is reported only once for each function it appears in
make[6]: *** [scripts/Makefile.build:287: drivers/firmware/efi/runtime-wrappers.o] Error 1
make[5]: *** [scripts/Makefile.build:549: drivers/firmware/efi] Error 2
make[4]: *** [scripts/Makefile.build:549: drivers/firmware] Error 2
make[4]: *** Waiting for unfinished jobs....
Aditya Garg Feb. 10, 2022, 5:54 a.m. UTC | #5 
> 
> The LOAD_UEFI_KEYS code isn't doing anything special here - it's just 
> trying to read some variables. If we simply disable that then the 
> expectation would be that reading the same variables from userland would 
> trigger the same failure. So the question is which of the variables that 
> LOAD_UEFI_KEYS accesses is triggering the failure, and what's special 
> about that? If it's a specific variable GUID or name that's failing, we 
> should block that on Apple hardware in order to avoid issues caused by 
> userland performing equivalent accesses.

The size log seems to be extra when crash occurs

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
Aditya Garg Feb. 10, 2022, 10:43 a.m. UTC | #6 
> On 09-Feb-2022, at 9:09 PM, David Laight <David.Laight@ACULAB.COM> wrote:
> 
> From: Aditya Garg
>> Sent: 09 February 2022 14:28
>> 
>> 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. 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
>> 
> ..
>> +static const struct dmi_system_id uefi_apple_ignore[] = {
>> +	{
>> +		 .matches = {
>> +			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,1"),
>> +		},
> 
> I think I'd use:
> #define xxx(vendor, product) \
> 		 .matches = {
> 			DMI_MATCH(DMI_BOARD_VENDOR, vendor), \
> 			DMI_MATCH(DMI_PRODUCT_NAME, product), \
> 		}
> somewhere with a suitable name (bikeshed blue) to reduce
> the code size of this table.
> 
Alright, I’ll send a v2 with this addressed.
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
Ard Biesheuvel Feb. 10, 2022, 10:47 a.m. UTC | #7 
On Thu, 10 Feb 2022 at 11:45, Aditya Garg <gargaditya08@live.com> wrote:
>
> From: Aditya Garg <gargaditya08@live.com>
>
> 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. 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@vger.kernel.org
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
> v2 :- Reduce code size of the table.

NAK. As Matthew pointed out, other reads of the same variables may
still trigger the same issue.


>  .../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.
> + * 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.
> + */
> +
> +static const struct dmi_system_id uefi_skip_cert[] = {
> +
> +       { 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.
>   */
> +
>  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;
>
>         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;
>
>         *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;
>
>         if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
>                 return false;
> --
> 2.25.1
>
>
Aditya Garg Feb. 10, 2022, 10:53 a.m. UTC | #8 
> 
> NAK. As Matthew pointed out, other reads of the same variables may
> still trigger the same issue.
> 

Ohk. I just sent a v2 in order to fix the issue point out by David. We can go ahead with Matthew’s point of view, which makes sense, in further versions.
Matthew Garrett Feb. 10, 2022, 6:09 p.m. UTC | #9 
On Thu, Feb 10, 2022 at 05:49:50AM +0000, Aditya Garg wrote:

> Looks like there is some error in this patch

I'm sorry, I'd build tested it here but clearly screwed that up. Try 
this one?

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index f3e54f6616f0..18b2ab40bee5 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -32,6 +32,8 @@
 #include <linux/stringify.h>
 #include <linux/workqueue.h>
 #include <linux/completion.h>
+#include <linux/ucs2_string.h>
+#include <linux/slab.h>
 
 #include <asm/efi.h>
 
@@ -179,6 +181,9 @@ static void efi_call_rts(struct work_struct *work)
 {
 	void *arg1, *arg2, *arg3, *arg4, *arg5;
 	efi_status_t status = EFI_NOT_FOUND;
+	unsigned long utf8_name_size;
+	char *utf8_name;
+	char guid_str[sizeof(efi_guid_t)+1];
 
 	arg1 = efi_rts_work.arg1;
 	arg2 = efi_rts_work.arg2;
@@ -203,6 +208,17 @@ static void efi_call_rts(struct work_struct *work)
 				       (efi_time_t *)arg2);
 		break;
 	case EFI_GET_VARIABLE:
+		utf8_name_size = ucs2_utf8size((efi_char16_t *)arg1);
+		utf8_name = kmalloc(utf8_name_size+1, GFP_KERNEL);
+		if (!utf8_name) {
+			printk(KERN_INFO "failed to allocate UTF8 buffer\n");
+			break;
+		}
+
+		ucs2_as_utf8(utf8_name, (efi_char16_t *)arg1, utf8_name_size + 1);
+		efi_guid_to_str((efi_guid_t *)arg2, guid_str);
+
+		printk(KERN_INFO "Reading EFI variable %s-%s\n", utf8_name, guid_str);
 		status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
 				       (efi_guid_t *)arg2, (u32 *)arg3,
 				       (unsigned long *)arg4, (void *)arg5);
Aditya Garg Feb. 11, 2022, 4:51 a.m. UTC | #10 
> 
> I'm sorry, I'd build tested it here but clearly screwed that up. Try 
> this one?
> 
> 
With this patch, I built 2 kernels, one with CONFIG_LOAD_UEFI_KEYS=y and other with CONFIG_LOAD_UEFI_KEYS=n. I have got different variables causing panics in both cases. The logs couldn't get saved in journalctl so, I clicked a picture of the same. The kernel anyways was refusing to boot after these logs.

With CONFIG_LOAD_UEFI_KEYS=y, this variable seems to be causing panics

MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23

The link of the logs :- https://gist.githubusercontent.com/AdityaGarg8/8e820c2724a65fb4bbb5deae2b358dc8/raw/2d3ef24c2b5025d500c5bebd418db5c185a47328/CONFIG_LOAD_UEFI_KEYS=y.jpeg

With CONFIG_LOAD_UEFI_KEYS=n, this variable seems to be causing panics

AppleSecureBootPolicy-94b73556-2197-4702-82a8-3e1337dafbfb

The link of the logs :- https://gist.githubusercontent.com/AdityaGarg8/8e820c2724a65fb4bbb5deae2b358dc8/raw/2d3ef24c2b5025d500c5bebd418db5c185a47328/CONFIG_LOAD_UEFI_KEYS=n.jpeg
Matthew Garrett Feb. 11, 2022, 4:28 p.m. UTC | #11 
On Fri, Feb 11, 2022 at 04:51:22AM +0000, Aditya Garg wrote:

> With this patch, I built 2 kernels, one with CONFIG_LOAD_UEFI_KEYS=y and other with CONFIG_LOAD_UEFI_KEYS=n. I have got different variables causing panics in both cases. The logs couldn't get saved in journalctl so, I clicked a picture of the same. The kernel anyways was refusing to boot after these logs.

Oh gosh I'm an idiot. Please try this one instead.

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index f3e54f6616f0..79a003c2f7b6 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -32,6 +32,8 @@
 #include <linux/stringify.h>
 #include <linux/workqueue.h>
 #include <linux/completion.h>
+#include <linux/ucs2_string.h>
+#include <linux/slab.h>
 
 #include <asm/efi.h>
 
@@ -179,6 +181,9 @@ static void efi_call_rts(struct work_struct *work)
 {
 	void *arg1, *arg2, *arg3, *arg4, *arg5;
 	efi_status_t status = EFI_NOT_FOUND;
+	unsigned long utf8_name_size;
+	char *utf8_name;
+	char guid_str[UUID_STRING_LEN + 1];
 
 	arg1 = efi_rts_work.arg1;
 	arg2 = efi_rts_work.arg2;
@@ -203,6 +208,17 @@ static void efi_call_rts(struct work_struct *work)
 				       (efi_time_t *)arg2);
 		break;
 	case EFI_GET_VARIABLE:
+		utf8_name_size = ucs2_utf8size((efi_char16_t *)arg1);
+		utf8_name = kmalloc(utf8_name_size+1, GFP_KERNEL);
+		if (!utf8_name) {
+			printk(KERN_INFO "failed to allocate UTF8 buffer\n");
+			break;
+		}
+
+		ucs2_as_utf8(utf8_name, (efi_char16_t *)arg1, utf8_name_size + 1);
+		efi_guid_to_str((efi_guid_t *)arg2, guid_str);
+
+		printk(KERN_INFO "Reading EFI variable %s-%s\n", utf8_name, guid_str);
 		status = efi_call_virt(get_variable, (efi_char16_t *)arg1,
 				       (efi_guid_t *)arg2, (u32 *)arg3,
 				       (unsigned long *)arg4, (void *)arg5);
Aditya Garg Feb. 12, 2022, 5:53 a.m. UTC | #12 
> 
> Oh gosh I'm an idiot. Please try this one instead.
> 

Looks like EFI variable MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23 and EFI variable db-d719b2cb-3d3a-4596-a3bc-dad00e67656f are causing issues

Feb 12 11:01:52 MacBook kernel: Reading EFI variable MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23
Feb 12 11:01:52 MacBook kernel: Reading EFI variable db-d719b2cb-3d3a-4596-a3bc-dad00e67656f
Feb 12 11:01:52 MacBook kernel: ------------[ cut here ]------------
Feb 12 11:01:52 MacBook kernel: [Firmware Bug]: Page fault caused by firmware at PA: 0xffffbc34c0068000
Feb 12 11:01:52 MacBook kernel: WARNING: CPU: 7 PID: 98 at arch/x86/platform/efi/quirks.c:735 efi_crash_gracefully_on_page_fault+0x50/0xf0
Feb 12 11:01:52 MacBook kernel: Modules linked in:
Feb 12 11:01:52 MacBook kernel: CPU: 7 PID: 98 Comm: kworker/u24:1 Not tainted 5.16.8-t2 #2
Feb 12 11:01:52 MacBook kernel: Hardware name: Apple Inc. MacBookPro16,1/Mac-E1008331FDC96864, BIOS 1715.81.2.0.0 (iBridge: 19.16.10744.0.0,0) 01/06/2022
Feb 12 11:01:52 MacBook kernel: Workqueue: efi_rts_wq efi_call_rts
Feb 12 11:01:52 MacBook kernel: RIP: 0010:efi_crash_gracefully_on_page_fault+0x50/0xf0
Feb 12 11:01:52 MacBook kernel: Code: fc e8 44 30 03 00 49 81 fc ff 0f 00 00 76 08 48 3d 50 04 3c 9b 74 04 41 5c 5d c3 4c 89 e6 48 c7 c7 c0 00 7c 9a e8 69 71 be 00 <0f> 0b 83 3d 87 62 11 02 0a 0f 84 0a 62 be 00 e8 bc 1b 00 00 48 8b
Feb 12 11:01:52 MacBook kernel: RSP: 0000:ffffbc34c03f29d8 EFLAGS: 00010086
Feb 12 11:01:52 MacBook kernel: RAX: 0000000000000000 RBX: ffffbc34c03f2a18 RCX: 0000000000000000
Feb 12 11:01:52 MacBook kernel: RDX: 0000000000000003 RSI: ffffbc34c03f2820 RDI: 00000000ffffffff
Feb 12 11:01:52 MacBook kernel: RBP: ffffbc34c03f29e0 R08: 0000000000000003 R09: 0000000000000001
Feb 12 11:01:52 MacBook kernel: R10: ffff9f6801685640 R11: 0000000000000001 R12: ffffbc34c0068000
Feb 12 11:01:52 MacBook kernel: R13: 0000000000000000 R14: ffffbc34c03f2b68 R15: ffff9f6801233380
Feb 12 11:01:52 MacBook kernel: FS:  0000000000000000(0000) GS:ffff9f6b6ebc0000(0000) knlGS:0000000000000000
Feb 12 11:01:52 MacBook kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Feb 12 11:01:52 MacBook kernel: CR2: ffffbc34c0068000 CR3: 00000001001ec002 CR4: 00000000003706e0
Feb 12 11:01:52 MacBook kernel: Call Trace:
Feb 12 11:01:52 MacBook kernel:  <TASK>
Feb 12 11:01:52 MacBook kernel:  page_fault_oops+0x4f/0x2c0
Feb 12 11:01:52 MacBook kernel:  ? search_bpf_extables+0x6b/0x80
Feb 12 11:01:52 MacBook kernel:  ? search_module_extables+0x50/0x80
Feb 12 11:01:52 MacBook kernel:  ? search_exception_tables+0x5b/0x60
Feb 12 11:01:52 MacBook kernel:  kernelmode_fixup_or_oops+0x9e/0x110
Feb 12 11:01:52 MacBook kernel:  __bad_area_nosemaphore+0x155/0x190
Feb 12 11:01:52 MacBook kernel:  bad_area_nosemaphore+0x16/0x20
Feb 12 11:01:52 MacBook kernel:  do_kern_addr_fault+0x8c/0xa0
Feb 12 11:01:52 MacBook kernel:  exc_page_fault+0xd8/0x180
Feb 12 11:01:52 MacBook kernel:  asm_exc_page_fault+0x1e/0x30
Feb 12 11:01:52 MacBook kernel: RIP: 0010:0xfffffffeefc440c5
Feb 12 11:01:52 MacBook kernel: Code: 31 c9 48 29 f9 48 83 e1 0f 74 0c 4c 39 c1 49 0f 47 c8 49 29 c8 f3 a4 4c 89 c1 49 83 e0 0f 48 c1 e9 04 74 2c f3 0f 7f 44 24 18 <f3> 0f 6f 06 66 0f e7 07 48 83 c6 10 48 83 c7 10 e2 ee 0f ae f0 66
Feb 12 11:01:52 MacBook kernel: RSP: 0000:ffffbc34c03f2c18 EFLAGS: 00010286
Feb 12 11:01:52 MacBook kernel: RAX: fffffffeefc9205a RBX: ffffffff9a81787a RCX: 0000000000000033
Feb 12 11:01:52 MacBook kernel: RDX: ffffbc34c0067d20 RSI: ffffbc34c0067ff6 RDI: fffffffeefc92330
Feb 12 11:01:52 MacBook kernel: RBP: ffffbc34c03f2ca0 R08: 0000000000000001 R09: ffffbc34c0068326
Feb 12 11:01:52 MacBook kernel: R10: fffffffeefc8e018 R11: 000000000019f2a5 R12: 0000000000000000
Feb 12 11:01:52 MacBook kernel: R13: ffffbc34c0067db0 R14: ffffbc34c0067d01 R15: 0000000000000607
Feb 12 11:01:52 MacBook kernel:  ? __efi_call+0x28/0x30
Feb 12 11:01:52 MacBook kernel:  ? switch_mm+0x20/0x30
Feb 12 11:01:52 MacBook kernel:  ? efi_call_rts+0x20d/0x970
Feb 12 11:01:52 MacBook kernel:  ? process_one_work+0x222/0x3f0
Feb 12 11:01:52 MacBook kernel:  ? worker_thread+0x4a/0x3d0
Feb 12 11:01:52 MacBook kernel:  ? kthread+0x17a/0x1a0
Feb 12 11:01:52 MacBook kernel:  ? process_one_work+0x3f0/0x3f0
Feb 12 11:01:52 MacBook kernel:  ? set_kthread_struct+0x40/0x40
Feb 12 11:01:52 MacBook kernel:  ? ret_from_fork+0x22/0x30
Feb 12 11:01:52 MacBook kernel:  </TASK>
Feb 12 11:01:52 MacBook kernel: ---[ end trace c81d89c8fdfe87c4 ]---
Feb 12 11:01:52 MacBook kernel: efi: Froze efi_rts_wq and disabled EFI Runtime Services
Feb 12 11:01:52 MacBook kernel: integrity: Couldn't get size: 0x8000000000000015
Feb 12 11:01:52 MacBook kernel: integrity: MODSIGN: Couldn't get UEFI db list
Feb 12 11:01:52 MacBook kernel: efi: EFI Runtime Services are disabled!
Feb 12 11:01:52 MacBook kernel: integrity: Couldn't get size: 0x8000000000000015
Feb 12 11:01:52 MacBook kernel: integrity: Couldn't get UEFI dbx list
Feb 12 11:01:52 MacBook kernel: integrity: Couldn't get size: 0x8000000000000015
Feb 12 11:01:52 MacBook kernel: integrity: Couldn't get mokx list
Feb 12 11:01:52 MacBook kernel: integrity: Couldn't get size: 0x8000000000000015
Feb 12 11:01:52 MacBook kernel: integrity: Couldn't get UEFI MokListRT


The full journalctl log is here on https://gist.githubusercontent.com/AdityaGarg8/8e820c2724a65fb4bbb5deae2b358dc8/raw/c71e13036426211cbd590fdfabe5f88531bac645/log.log


With CONFIG_LOAD_UEFI_KEYS=n, these variables are not seen and EFI Runtime Services work.
Matthew Garrett Feb. 12, 2022, 7:42 p.m. UTC | #13 
On Sat, Feb 12, 2022 at 05:53:47AM +0000, Aditya Garg wrote:

> Feb 12 11:01:52 MacBook kernel: Reading EFI variable db-d719b2cb-3d3a-4596-a3bc-dad00e67656f

Ok. With CONFIG_LOAD_UEFI_KEYS=n, can you run:

cat /sys/firmware/efi/efivars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f

and see whether it generates the same failure? If so then my (handwavy) 
guess is that something's going wrong with a firmware codepath for the 
d719b2cb-3d3a-4596-a3bc-dad00e67656f GUID. Someone could potentially 
then figure out whether the same happens under Windows, but the easiest 
thing is probably to just return a failure on Apple hardware when 
someone tries to access anything with that GUID.
Aditya Garg Feb. 13, 2022, 8:22 a.m. UTC | #14 
> 
> Ok. With CONFIG_LOAD_UEFI_KEYS=n, can you run:
> 
> cat /sys/firmware/efi/efivars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f
> 
> and see whether it generates the same failure? If so then my (handwavy) 
> guess is that something's going wrong with a firmware codepath for the 
> d719b2cb-3d3a-4596-a3bc-dad00e67656f GUID. Someone could potentially 
> then figure out whether the same happens under Windows, but the easiest 
> thing is probably to just return a failure on Apple hardware when 
> someone tries to access anything with that GUID.

Surprisingly it didn’t cause a crash. The logs are at https://gist.githubusercontent.com/AdityaGarg8/8e820c2724a65fb4bbb5deae2b358dc8/raw/2a003ef43ae06dbe2bcc22b34ba7ccbb03898a21/log2.log

I also tried cat /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23, but it doesn’t exist

aditya@MacBook:~$ cat /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23
cat: /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23: No such file or directory
Matthew Garrett Feb. 13, 2022, 9:33 p.m. UTC | #15 
On Sun, Feb 13, 2022 at 08:22:32AM +0000, Aditya Garg wrote:

> Surprisingly it didn’t cause a crash. The logs are at https://gist.githubusercontent.com/AdityaGarg8/8e820c2724a65fb4bbb5deae2b358dc8/raw/2a003ef43ae06dbe2bcc22b34ba7ccbb03898a21/log2.log

Interesting. Ok, so there's something else going on here. I'll have 
access to a T2 system next week, so I'll take a look then. Is this 
something that started happening recently, or has it always happened if 
this config option is set on these platforms?
Aditya Garg Feb. 14, 2022, 12:33 a.m. UTC | #16 
> 
> Interesting. Ok, so there's something else going on here. I'll have 
> access to a T2 system next week, so I'll take a look then. Is this 
> something that started happening recently, or has it always happened if 
> this config option is set on these platforms?

Before https://github.com/torvalds/linux/commit/f5390cd0b43c2e54c7cf5506c7da4a37c5cef746, we had to add efi=noruntime to get Linux working on these machines. Now, its this config option only which is the issue.

You may want to follow https://wiki.t2linux.org/ to test on a T2 Mac, since many T2 patches haven't been upstreamed yet.
diff mbox series

Patch

diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 08b6d12f9..668b2a713 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,106 @@ 
 #include "../integrity.h"
 #include "keyring_handler.h"
 
+/* Apple Macs with T2 Security chip don't support these UEFI variables.
+ * 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.
+ */
+
+static const struct dmi_system_id uefi_apple_ignore[] = {
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,1"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,2"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,3"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro15,4"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,2"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,3"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir8,1"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir8,2"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir9,1"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacMini8,1"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacPro7,1"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "iMac20,1"),
+		},
+	},
+	{
+		 .matches = {
+			DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "iMac20,2"),
+		},
+	},
+	{ }
+};
+
 /*
  * Look to see if a UEFI variable called MokIgnoreDB exists and return true if
  * it does.
@@ -21,12 +122,18 @@ 
  * is set, we should ignore the db variable also and the true return indicates
  * this.
  */
+
 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_apple_ignore);
+	if (dmi_id)
+		return 0;
 
 	status = efi.get_variable(L"MokIgnoreDB", &guid, NULL, &size, &db);
 	return status == EFI_SUCCESS;
@@ -41,6 +148,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_apple_ignore);
+	if (dmi_id)
+		return 0;
 
 	*status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
 	if (*status == EFI_NOT_FOUND)
@@ -85,6 +197,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_apple_ignore);
+	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 +255,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_apple_ignore);
+	if (dmi_id)
+		return 0;
 
 	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
 		return false;