From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161261AbcHELlz (ORCPT ); Fri, 5 Aug 2016 07:41:55 -0400 Received: from mailout3.hostsharing.net ([176.9.242.54]:33445 "EHLO mailout3.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759594AbcHELlx (ORCPT ); Fri, 5 Aug 2016 07:41:53 -0400 Date: Fri, 5 Aug 2016 13:42:19 +0200 From: Lukas Wunner To: Matt Fleming Cc: linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, Andreas Noever , Pierre Moreau , reverser@put.as, grub-devel@gnu.org, x86@kernel.org, linux-acpi@vger.kernel.org Subject: Re: [PATCH 1/6] efi: Retrieve Apple device properties Message-ID: <20160805114219.GA4952@wunner.de> References: <833d193b8a18b0afe168c515e9e56a857ece4bd1.1469616641.git.lukas@wunner.de> <20160804151345.GM3636@codeblueprint.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160804151345.GM3636@codeblueprint.co.uk> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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