linux-kernel - Re: [PATCH] efi: libstub: add support for the apple_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <MA0P287MB021730971A804FB760463A39B8D22@MA0P287MB0217.INDP287.PROD.OUTLOOK.COM>
Date: Sun, 30 Jun 2024 09:13:33 +0000
From: Aditya Garg <gargaditya08@...e.com>
To: Lukas Wunner <lukas@...ner.de>
CC: Ard Biesheuvel <ardb@...nel.org>, Hans de Goede <hdegoede@...hat.com>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>, Linux Kernel Mailing
 List <linux-kernel@...r.kernel.org>, Orlando Chamberlain
	<orlandoch.dev@...il.com>, Kerem Karabay <kekrby@...il.com>
Subject: Re: [PATCH] efi: libstub: add support for the apple_set_os protocol

Hi Lukas

> On 30 Jun 2024, at 1:34 PM, Lukas Wunner <lukas@...ner.de> wrote:
> 
> On Sun, Jun 30, 2024 at 04:42:55AM +0000, Aditya Garg wrote:
>> Commit 0c18184de990 brought support for T2 Macs in apple-gmux. But in order to
> 
> Please run patches through scripts/checkpatch.pl before submission.
> The subject of the commit is missing here and lines should be wrapped
> at 72 or at least 74 chars.
> 
Will fix it in the next revision.
> 
>> Based on this patch for GRUB by Andreas Heider <andreas@...der.io>:
>> https://lists.gnu.org/archive/html/grub-devel/2013-12/msg00442.html
> 
> Please include his Signed-off-by and cc him.
Not sure about this since the patch was send to grub and not lkml, and his work has been used without informing him for this patch solely on the basis of GPL.

I've always been confused in signed-off-by in case of authors whose work has been used without their explicit consent just because the license permits it.

Should I still add his signed-off-by?
> 
> 
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -399,6 +399,8 @@
>>                  useful so that a dump capture kernel won't be
>>                  shot down by NMI
>> 
>> +    apple_set_os    [KNL] Report that macOS is being booted to the firmware
>> +
> 
> Why the kernel parameter?  Why not do this unconditionally?
1. Not all Macs have dual GPU. We don't want to unnecessarily "fool" the firmware in thinking macOS is being booted.
2. apple-gmux is a reverse engineered driver, although upstreamed, not very efficient in switching GPUs. So it's better to make unlocking the GPU optional. + not everyone wants the intel GPU, people are happy working with the dedicated AMD GPU (used by default if apple_set_os isn't enabled).
> 
> 
>> +struct apple_set_os_protocol {
>> +    u64 version;
>> +    efi_status_t (__efiapi *set_os_version) (const char *);
>> +    efi_status_t (__efiapi *set_os_vendor) (const char *);
>> +    struct {
>> +        u32 version;
>> +        u32 set_os_version;
>> +        u32 set_os_vendor;
>> +    } mixed_mode;
>> +};
> 
> How about declaring this __packed, just to be on the safe side?
You mean "struct __attribute__((__packed__)) apple_set_os_protocol {" ?
> 
> Why "mixed_mode"?  Seems like an odd name given "mixed mode"
> in EFI context usually means 64-bit OS, but 32-bit EFI.

EFI firmware on T2 Macs doesn't seem to follow the standard EFI specs (expected from Apple). Earlier it claimed to follow EFI 2.0, but we had to force linux to use EFI 1.1 for it. So as far as Apple is concerned, you'll get to see such strange things.

I guess this strange behaviour is because the T2 security chip handles the EFI.


I'll wait for you replied before sending a v2.