[PATCH 1/6] efi: Retrieve Apple device properties

[PATCH 1/6] efi: Retrieve Apple device properties

[Lukas Wunner]

Retrieve device properties from EFI using Apple's proprietary protocol
with GUID 91BD12FE-F6C3-44FB-A5B7-5122AB303AE0, which looks like this:

typedef struct {
	unsigned long signature; /* 0x10000 */
	efi_status_t (*get) (
		IN struct apple_properties_protocol *this,
		IN struct efi_generic_dev_path *device,
		IN efi_char16_t *property_name,
		OUT void *buffer,
		IN OUT u32 *buffer_size);
		/* EFI_SUCCESS, EFI_NOT_FOUND, EFI_BUFFER_TOO_SMALL */
	efi_status_t (*set) (
		IN struct apple_properties_protocol *this,
		IN struct efi_generic_dev_path *device,
		IN efi_char16_t *property_name,
		IN void *property_value,
		IN u32 property_value_len);
		/* allocates copies of property name and value */
		/* EFI_SUCCESS, EFI_OUT_OF_RESOURCES */
	efi_status_t (*del) (
		IN struct apple_properties_protocol *this,
		IN struct efi_generic_dev_path *device,
		IN efi_char16_t *property_name);
		/* EFI_SUCCESS, EFI_NOT_FOUND */
	efi_status_t (*get_all) (
		IN struct apple_properties_protocol *this,
		OUT void *buffer,
		IN OUT u32 *buffer_size);
		/* EFI_SUCCESS, EFI_BUFFER_TOO_SMALL */
} apple_properties_protocol;

Apple's EFI driver implementing this protocol, "AAPL,PathProperties",
is a per-device key/value store which is populated by other EFI drivers.
On macOS, these device properties are retrieved by the bootloader
/usr/standalone/i386/boot.efi. The extension AppleACPIPlatform.kext
subsequently merges them into the I/O Kit registry (see ioreg(8)) where
they can be queried by other kernel extensions and user space.

These device properties contain vital information which cannot be
obtained any other way (e.g. Thunderbolt Device ROM). EFI drivers also
use them to communicate the current device state so that OS drivers can
pick up where EFI drivers left (e.g. GPU mode setting).

The get_all() function conveniently fills a buffer with all properties
in marshalled form which can be passed to the kernel as a setup_data
payload. Note that the device properties will only be available if the
kernel is booted with the efistub. Distros should adjust their
installers to always use the efistub on Macs. grub with the "linux"
directive will not work unless the functionality of this commit is
duplicated in grub. (The "linuxefi" directive should work but is not
included upstream as of this writing.)

Thanks to osxreverser for this blog posting which was helpful in reverse
engineering Apple's EFI drivers and bootloader:
https://reverse.put.as/2016/06/25/apple-efi-firmware-passwords-and-the-scbo-myth/

If someone at Apple is reading this, please note there's a memory leak
in your implementation of the del() function as the property struct is
freed but the name and value allocations are not.

Cc: reverser@put.as
Cc: grub-devel@gnu.org
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Andreas Noever <andreas.noever@gmail.com>
Tested-by: Lukas Wunner <lukas@wunner.de> [MacBookPro9,1]
Tested-by: Pierre Moreau <pierre.morrow@free.fr> [MacBookPro11,3]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 arch/x86/boot/compressed/eboot.c      | 55 +++++++++++++++++++++++++++++++++++
 arch/x86/include/uapi/asm/bootparam.h |  1 +
 include/linux/efi.h                   | 17 +++++++++++
 3 files changed, 73 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ff574da..7262ee4 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -571,6 +571,55 @@ free_handle:
 	efi_call_early(free_pool, pci_handle);
 }
 
+static void retrieve_apple_device_properties(struct boot_params *params)
+{
+	efi_guid_t guid = APPLE_PROPERTIES_PROTOCOL_GUID;
+	struct setup_data *data, *new;
+	efi_status_t status;
+	void *properties;
+	u32 size = 0;
+
+	status = efi_early->call(
+			(unsigned long)sys_table->boottime->locate_protocol,
+			&guid, NULL, &properties);
+	if (status != EFI_SUCCESS)
+		return;
+
+	do {
+		status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+			size + sizeof(struct setup_data), &new);
+		if (status != EFI_SUCCESS) {
+			efi_printk(sys_table,
+				   "Failed to alloc mem for properties\n");
+			return;
+		}
+		status = efi_early->call(efi_early->is64 ?
+			((apple_properties_protocol_64 *)properties)->get_all :
+			((apple_properties_protocol_32 *)properties)->get_all,
+			properties, new->data, &size);
+		if (status == EFI_BUFFER_TOO_SMALL)
+			efi_call_early(free_pool, new);
+	} while (status == EFI_BUFFER_TOO_SMALL);
+
+	if (!size) {
+		efi_call_early(free_pool, new);
+		return;
+	}
+
+	new->type = SETUP_APPLE_PROPERTIES;
+	new->len  = size;
+	new->next = 0;
+
+	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
+	if (!data)
+		params->hdr.setup_data = (unsigned long)new;
+	else {
+		while (data->next)
+			data = (struct setup_data *)(unsigned long)data->next;
+		data->next = (unsigned long)new;
+	}
+}
+
 static efi_status_t
 setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 {
@@ -1098,6 +1147,7 @@ free_mem_map:
 struct boot_params *efi_main(struct efi_config *c,
 			     struct boot_params *boot_params)
 {
+	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
 	struct desc_ptr *gdt = NULL;
 	efi_loaded_image_t *image;
 	struct setup_header *hdr = &boot_params->hdr;
@@ -1128,6 +1178,11 @@ struct boot_params *efi_main(struct efi_config *c,
 
 	setup_efi_pci(boot_params);
 
+	if (!memcmp((void *)sys_table->fw_vendor, apple, sizeof(apple))) {
+		if (IS_ENABLED(CONFIG_APPLE_PROPERTIES))
+			retrieve_apple_device_properties(boot_params);
+	}
+
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
 				sizeof(*gdt), (void **)&gdt);
 	if (status != EFI_SUCCESS) {
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c18ce67..b10bf31 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -7,6 +7,7 @@
 #define SETUP_DTB			2
 #define SETUP_PCI			3
 #define SETUP_EFI			4
+#define SETUP_APPLE_PROPERTIES		5
 
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f80a75..e53b4b2 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -432,6 +432,22 @@ typedef struct {
 #define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
 #define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
 
+typedef struct {
+	u32 signature;
+	u32 get;
+	u32 set;
+	u32 del;
+	u32 get_all;
+} apple_properties_protocol_32;
+
+typedef struct {
+	u64 signature;
+	u64 get;
+	u64 set;
+	u64 del;
+	u64 get_all;
+} apple_properties_protocol_64;
+
 /*
  * Types and defines for EFI ResetSystem
  */
@@ -580,6 +596,7 @@ void efi_native_runtime_setup(void);
 #define EFI_RNG_PROTOCOL_GUID			EFI_GUID(0x3152bca5, 0xeade, 0x433d,  0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
 #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
 #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
+#define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
 
 /*
  * This GUID is used to pass to the kernel proper the struct screen_info
-- 
2.8.1

[PATCH 0/6] Apple device properties

[Lukas Wunner]

Apple EFI drivers supply device properties which are needed to support
Macs optimally.

This series extends the efistub to retrieve the device properties before
ExitBootServices is called (patch [1/6]). They are assigned to devices
in an fs_initcall (patch [5/6]). As a first use case, the Thunderbolt
driver is amended to take advantage of the Device ROM supplied by EFI
(patch [6/6]).

A by-product is a parser for EFI Device Paths which finds the struct
device corresponding to a given path. This is needed to assign
properties to their devices (patch [3/6]).


I've pushed these patches to GitHub where they can be reviewed more
comfortably with green/red highlighting:
https://github.com/l1k/linux/commits/apple_properties_v1


It would be good if one of the resident EFI experts could look over
patch [1/6] to see if I've made any mistakes that might prevent this
from working on 32 bit. It was only tested on 64 bit, I don't know
anyone with an older Mac who could test this.

Specifically, is the following okay:
efi_early->call((unsigned long)sys_table->boottime->locate_protocol, ...)

It would be convenient to have LocateProtocol or LocateHandleBuffer in
struct efi_config so that they can be called with efi_call_early().
Would a patch to add those be entertained? Right now we only offer
LocateHandle and HandleProtocol, which is somewhat cumbersome and
needs more code as the setup_pci() functions show.

Thanks,

Lukas


Lukas Wunner (6):
  efi: Retrieve Apple device properties
  ACPI / bus: Make acpi_get_first_physical_node() public
  efi: Add device path parser
  driver core: Don't leak secondary fwnode on device removal
  efi: Assign Apple device properties
  thunderbolt: Use Device ROM retrieved from EFI

 Documentation/kernel-parameters.txt     |   5 +
 arch/x86/boot/compressed/eboot.c        |  55 ++++++++
 arch/x86/include/uapi/asm/bootparam.h   |   1 +
 drivers/acpi/internal.h                 |   1 -
 drivers/base/core.c                     |   1 +
 drivers/firmware/efi/Kconfig            |  16 +++
 drivers/firmware/efi/Makefile           |   2 +
 drivers/firmware/efi/apple-properties.c | 219 ++++++++++++++++++++++++++++++++
 drivers/firmware/efi/dev-path-parser.c  | 186 +++++++++++++++++++++++++++
 drivers/thunderbolt/Kconfig             |   1 +
 drivers/thunderbolt/eeprom.c            |  42 ++++++
 drivers/thunderbolt/switch.c            |   2 +-
 include/linux/acpi.h                    |   7 +
 include/linux/efi.h                     |  38 ++++++
 14 files changed, 574 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/efi/apple-properties.c
 create mode 100644 drivers/firmware/efi/dev-path-parser.c

-- 
2.8.1

Re: [PATCH 0/6] Apple device properties

[Rafael J. Wysocki]

On Wednesday, July 27, 2016 01:20:41 PM Lukas Wunner wrote:
> Apple EFI drivers supply device properties which are needed to support
> Macs optimally.
> 
> This series extends the efistub to retrieve the device properties before
> ExitBootServices is called (patch [1/6]). They are assigned to devices
> in an fs_initcall (patch [5/6]). As a first use case, the Thunderbolt
> driver is amended to take advantage of the Device ROM supplied by EFI
> (patch [6/6]).

Can you please CC the whole series to linux-acpi for easier/better review?

Thanks,
Rafael

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Andrei Borzenkov]

27.07.2016 14:20, Lukas Wunner пишет:
> Retrieve device properties from EFI using Apple's proprietary protocol
> with GUID 91BD12FE-F6C3-44FB-A5B7-5122AB303AE0, which looks like this:
> 
> typedef struct {
> 	unsigned long signature; /* 0x10000 */
> 	efi_status_t (*get) (
> 		IN struct apple_properties_protocol *this,
> 		IN struct efi_generic_dev_path *device,
> 		IN efi_char16_t *property_name,
> 		OUT void *buffer,
> 		IN OUT u32 *buffer_size);
> 		/* EFI_SUCCESS, EFI_NOT_FOUND, EFI_BUFFER_TOO_SMALL */
> 	efi_status_t (*set) (
> 		IN struct apple_properties_protocol *this,
> 		IN struct efi_generic_dev_path *device,
> 		IN efi_char16_t *property_name,
> 		IN void *property_value,
> 		IN u32 property_value_len);
> 		/* allocates copies of property name and value */
> 		/* EFI_SUCCESS, EFI_OUT_OF_RESOURCES */
> 	efi_status_t (*del) (
> 		IN struct apple_properties_protocol *this,
> 		IN struct efi_generic_dev_path *device,
> 		IN efi_char16_t *property_name);
> 		/* EFI_SUCCESS, EFI_NOT_FOUND */
> 	efi_status_t (*get_all) (
> 		IN struct apple_properties_protocol *this,
> 		OUT void *buffer,
> 		IN OUT u32 *buffer_size);
> 		/* EFI_SUCCESS, EFI_BUFFER_TOO_SMALL */
> } apple_properties_protocol;
> 
> Apple's EFI driver implementing this protocol, "AAPL,PathProperties",
> is a per-device key/value store which is populated by other EFI drivers.
> On macOS, these device properties are retrieved by the bootloader
> /usr/standalone/i386/boot.efi. The extension AppleACPIPlatform.kext
> subsequently merges them into the I/O Kit registry (see ioreg(8)) where
> they can be queried by other kernel extensions and user space.
> 
> These device properties contain vital information which cannot be
> obtained any other way (e.g. Thunderbolt Device ROM). EFI drivers also
> use them to communicate the current device state so that OS drivers can
> pick up where EFI drivers left (e.g. GPU mode setting).
> 
> The get_all() function conveniently fills a buffer with all properties
> in marshalled form which can be passed to the kernel as a setup_data
> payload. Note that the device properties will only be available if the
> kernel is booted with the efistub. Distros should adjust their
> installers to always use the efistub on Macs. grub with the "linux"
> directive will not work unless the functionality of this commit is
> duplicated in grub. (The "linuxefi" directive should work but is not
> included upstream as of this writing.)
> 

Intention was to partially merge linuxefi functionality into "linux" by
autodetecting and preferring EFI stub support. Peter, any progress on this?

> Thanks to osxreverser for this blog posting which was helpful in reverse
> engineering Apple's EFI drivers and bootloader:
> https://reverse.put.as/2016/06/25/apple-efi-firmware-passwords-and-the-scbo-myth/
> 
> If someone at Apple is reading this, please note there's a memory leak
> in your implementation of the del() function as the property struct is
> freed but the name and value allocations are not.
> 
> Cc: reverser@put.as
> Cc: grub-devel@gnu.org
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Andreas Noever <andreas.noever@gmail.com>
> Tested-by: Lukas Wunner <lukas@wunner.de> [MacBookPro9,1]
> Tested-by: Pierre Moreau <pierre.morrow@free.fr> [MacBookPro11,3]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  arch/x86/boot/compressed/eboot.c      | 55 +++++++++++++++++++++++++++++++++++
>  arch/x86/include/uapi/asm/bootparam.h |  1 +
>  include/linux/efi.h                   | 17 +++++++++++
>  3 files changed, 73 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index ff574da..7262ee4 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -571,6 +571,55 @@ free_handle:
>  	efi_call_early(free_pool, pci_handle);
>  }
>  
> +static void retrieve_apple_device_properties(struct boot_params *params)
> +{
> +	efi_guid_t guid = APPLE_PROPERTIES_PROTOCOL_GUID;
> +	struct setup_data *data, *new;
> +	efi_status_t status;
> +	void *properties;
> +	u32 size = 0;
> +
> +	status = efi_early->call(
> +			(unsigned long)sys_table->boottime->locate_protocol,
> +			&guid, NULL, &properties);
> +	if (status != EFI_SUCCESS)
> +		return;
> +
> +	do {
> +		status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> +			size + sizeof(struct setup_data), &new);
> +		if (status != EFI_SUCCESS) {
> +			efi_printk(sys_table,
> +				   "Failed to alloc mem for properties\n");
> +			return;
> +		}
> +		status = efi_early->call(efi_early->is64 ?
> +			((apple_properties_protocol_64 *)properties)->get_all :
> +			((apple_properties_protocol_32 *)properties)->get_all,
> +			properties, new->data, &size);
> +		if (status == EFI_BUFFER_TOO_SMALL)
> +			efi_call_early(free_pool, new);
> +	} while (status == EFI_BUFFER_TOO_SMALL);
> +
> +	if (!size) {
> +		efi_call_early(free_pool, new);
> +		return;
> +	}
> +
> +	new->type = SETUP_APPLE_PROPERTIES;
> +	new->len  = size;
> +	new->next = 0;
> +
> +	data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
> +	if (!data)
> +		params->hdr.setup_data = (unsigned long)new;
> +	else {
> +		while (data->next)
> +			data = (struct setup_data *)(unsigned long)data->next;
> +		data->next = (unsigned long)new;
> +	}
> +}
> +
>  static efi_status_t
>  setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
>  {
> @@ -1098,6 +1147,7 @@ free_mem_map:
>  struct boot_params *efi_main(struct efi_config *c,
>  			     struct boot_params *boot_params)
>  {
> +	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>  	struct desc_ptr *gdt = NULL;
>  	efi_loaded_image_t *image;
>  	struct setup_header *hdr = &boot_params->hdr;
> @@ -1128,6 +1178,11 @@ struct boot_params *efi_main(struct efi_config *c,
>  
>  	setup_efi_pci(boot_params);
>  
> +	if (!memcmp((void *)sys_table->fw_vendor, apple, sizeof(apple))) {
> +		if (IS_ENABLED(CONFIG_APPLE_PROPERTIES))
> +			retrieve_apple_device_properties(boot_params);
> +	}
> +
>  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>  				sizeof(*gdt), (void **)&gdt);
>  	if (status != EFI_SUCCESS) {
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index c18ce67..b10bf31 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -7,6 +7,7 @@
>  #define SETUP_DTB			2
>  #define SETUP_PCI			3
>  #define SETUP_EFI			4
> +#define SETUP_APPLE_PROPERTIES		5
>  
>  /* ram_size flags */
>  #define RAMDISK_IMAGE_START_MASK	0x07FF
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 7f80a75..e53b4b2 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -432,6 +432,22 @@ typedef struct {
>  #define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
>  #define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
>  
> +typedef struct {
> +	u32 signature;
> +	u32 get;
> +	u32 set;
> +	u32 del;
> +	u32 get_all;
> +} apple_properties_protocol_32;
> +
> +typedef struct {
> +	u64 signature;
> +	u64 get;
> +	u64 set;
> +	u64 del;
> +	u64 get_all;
> +} apple_properties_protocol_64;
> +
>  /*
>   * Types and defines for EFI ResetSystem
>   */
> @@ -580,6 +596,7 @@ void efi_native_runtime_setup(void);
>  #define EFI_RNG_PROTOCOL_GUID			EFI_GUID(0x3152bca5, 0xeade, 0x433d,  0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
>  #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
>  #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
> +#define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
>  
>  /*
>   * This GUID is used to pass to the kernel proper the struct screen_info
> 

Re: [PATCH 0/6] Apple device properties

[Matt Fleming]

On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> Apple EFI drivers supply device properties which are needed to support
> Macs optimally.
> 
> This series extends the efistub to retrieve the device properties before
> ExitBootServices is called (patch [1/6]). They are assigned to devices
> in an fs_initcall (patch [5/6]). As a first use case, the Thunderbolt
> driver is amended to take advantage of the Device ROM supplied by EFI
> (patch [6/6]).
> 
> A by-product is a parser for EFI Device Paths which finds the struct
> device corresponding to a given path. This is needed to assign
> properties to their devices (patch [3/6]).
> 
> 
> I've pushed these patches to GitHub where they can be reviewed more
> comfortably with green/red highlighting:
> https://github.com/l1k/linux/commits/apple_properties_v1
> 
> 
> It would be good if one of the resident EFI experts could look over
> patch [1/6] to see if I've made any mistakes that might prevent this
> from working on 32 bit. It was only tested on 64 bit, I don't know
> anyone with an older Mac who could test this.
> 
> Specifically, is the following okay:
> efi_early->call((unsigned long)sys_table->boottime->locate_protocol, ...)

This probably isn't going to work with EFI mixed mode because you
can't jump through pointers at runtime - that's the whole point of the
setup_boot_services*bits() code.

> It would be convenient to have LocateProtocol or LocateHandleBuffer in
> struct efi_config so that they can be called with efi_call_early().
> Would a patch to add those be entertained? Right now we only offer
> LocateHandle and HandleProtocol, which is somewhat cumbersome and
> needs more code as the setup_pci() functions show.

Yes, go for it. Doing this would make it work with EFI mixed mode too.

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Matt Fleming]

On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index ff574da..7262ee4 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -571,6 +571,55 @@ free_handle:
>  	efi_call_early(free_pool, pci_handle);
>  }
>  
> +static void retrieve_apple_device_properties(struct boot_params *params)
> +{
> +	efi_guid_t guid = APPLE_PROPERTIES_PROTOCOL_GUID;
> +	struct setup_data *data, *new;
> +	efi_status_t status;
> +	void *properties;
> +	u32 size = 0;
> +
> +	status = efi_early->call(
> +			(unsigned long)sys_table->boottime->locate_protocol,
> +			&guid, NULL, &properties);
> +	if (status != EFI_SUCCESS)
> +		return;
> +
> +	do {
> +		status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> +			size + sizeof(struct setup_data), &new);
> +		if (status != EFI_SUCCESS) {
> +			efi_printk(sys_table,
> +				   "Failed to alloc mem for properties\n");
> +			return;
> +		}
> +		status = efi_early->call(efi_early->is64 ?
> +			((apple_properties_protocol_64 *)properties)->get_all :
> +			((apple_properties_protocol_32 *)properties)->get_all,
> +			properties, new->data, &size);
> +		if (status == EFI_BUFFER_TOO_SMALL)
> +			efi_call_early(free_pool, new);
> +	} while (status == EFI_BUFFER_TOO_SMALL);

Is this looping really required? Do we not know ahead of time what we
expect the size to be? Writing this as a potentially infinite loop (if
broken firmware always returns EFI_BUFFER_TOO_SMALL) is a bad idea.

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Lukas Wunner]

On Thu, Aug 04, 2016 at 04:13:45PM +0100, Matt Fleming wrote:
> On Thu, 28 Jul, at 02:25:41AM, Lukas Wunner wrote:
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index ff574da..7262ee4 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -571,6 +571,55 @@ free_handle:
> >  	efi_call_early(free_pool, pci_handle);
> >  }
> >  
> > +static void retrieve_apple_device_properties(struct boot_params *params)
> > +{
> > +	efi_guid_t guid = APPLE_PROPERTIES_PROTOCOL_GUID;
> > +	struct setup_data *data, *new;
> > +	efi_status_t status;
> > +	void *properties;
> > +	u32 size = 0;
> > +
> > +	status = efi_early->call(
> > +			(unsigned long)sys_table->boottime->locate_protocol,
> > +			&guid, NULL, &properties);
> > +	if (status != EFI_SUCCESS)
> > +		return;
> > +
> > +	do {
> > +		status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> > +			size + sizeof(struct setup_data), &new);
> > +		if (status != EFI_SUCCESS) {
> > +			efi_printk(sys_table,
> > +				   "Failed to alloc mem for properties\n");
> > +			return;
> > +		}
> > +		status = efi_early->call(efi_early->is64 ?
> > +			((apple_properties_protocol_64 *)properties)->get_all :
> > +			((apple_properties_protocol_32 *)properties)->get_all,
> > +			properties, new->data, &size);
> > +		if (status == EFI_BUFFER_TOO_SMALL)
> > +			efi_call_early(free_pool, new);
> > +	} while (status == EFI_BUFFER_TOO_SMALL);
> 
> Is this looping really required? Do we not know ahead of time what we
> expect the size to be? Writing this as a potentially infinite loop (if
> broken firmware always returns EFI_BUFFER_TOO_SMALL) is a bad idea.

macOS' bootloader does exactly the same. So if the firmware was broken
in this way, macOS wouldn't boot and it's unlikely that Apple would
ship it. The code is not executed on non-Macs (due to the memcmp for
sys_table->fw_vendor[] == u"Apple" in efi_main()).

Looks like this in /usr/standalone/i386/boot.efi:
58b9         mov        rbx, 0x8000000000000005		; EFI_BUFFER_TOO_SMALL
...
58e6         mov        rcx, qword [ss:rbp+var_38]	; properties protocol
58ea         mov        rdx, rdi			; properties buffer
58ed         mov        r8, rsi				; buffer len
58f0         call       qword [ds:rcx+0x20]		; properties->get_all
58f3         cmp        rax, rbx
58f6         je         0x58c5				; infinite loop

And the code in the corresponding ->get_all function in the EFI driver
is such that it only returns either EFI_SUCCESS or EFI_BUFFER_TOO_SMALL.

So I could cap the number of loop iterations but it would be pointless.

I also checked the bootloader they shipped with OS X 10.6 (2009), they
used Universal EFI binaries back then (x86_64 + i386) in order to support
the very first Intel Macs of 2006. Found the same infinite loop there.

The reason for the loop is that the number of device properties is
dynamic. E.g. each attached Thunderbolt device is assigned 3 properties.
If a Thunderbolt device is plugged in between a first loop iteration
(to obtain the size) and a second loop iteration (to fill the buffer),
EFI_BUFFER_TOO_SMALL is returned and a third loop iteration is needed.

Thanks,

Lukas

Re: [PATCH 1/6] efi: Retrieve Apple device properties

[Matt Fleming]

On Fri, 05 Aug, at 01:42:19PM, Lukas Wunner wrote:
> 
> So I could cap the number of loop iterations but it would be pointless.

OK, thanks for the explanation.