From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1bVdVs-0007XS-Tz for mharc-grub-devel@gnu.org; Fri, 05 Aug 2016 07:42:04 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52037) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVdVq-0007XL-Gw for grub-devel@gnu.org; Fri, 05 Aug 2016 07:42:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVdVl-0002tC-Ts for grub-devel@gnu.org; Fri, 05 Aug 2016 07:42:01 -0400 Received: from mailout3.hostsharing.net ([176.9.242.54]:45905) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVdVl-0002so-Kb for grub-devel@gnu.org; Fri, 05 Aug 2016 07:41:57 -0400 Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mailout3.hostsharing.net (Postfix) with ESMTPS id 7FEBD101E6A40; Fri, 5 Aug 2016 13:41:49 +0200 (CEST) Received: from localhost (4-38-90-81.adsl.cmo.de [81.90.38.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by h08.hostsharing.net (Postfix) with ESMTPSA id AC5D26001B9A; Fri, 5 Aug 2016 13:41:47 +0200 (CEST) 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) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 176.9.242.54 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Aug 2016 11:42:03 -0000 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