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] [day] [month] [year] [list]
Date: Sun, 30 Jun 2024 19:18:29 +0000
From: Aditya Garg <gargaditya08@...e.com>
To: Ard Biesheuvel <ardb@...nel.org>
CC: Lukas Wunner <lukas@...ner.de>, 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



> On 30 Jun 2024, at 6:29 PM, Ard Biesheuvel <ardb@...nel.org> wrote:
> 
> On Sun, 30 Jun 2024 at 13:56, Aditya Garg <gargaditya08@...e.com> wrote:
>> 
>> 
>> 
>>>> On 30 Jun 2024, at 4:59 PM, Ard Biesheuvel <ardb@...nel.org> wrote:
>>> 
>>> Hello Aditya, Lukas,
>>> 
>>>> On Sun, 30 Jun 2024 at 10:04, 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.
>>>> 
>>>> 
>>>>> 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.
>>>> 
>>>> 
>>> 
>>> No. You cannot simply add a signed-off-by on someone else's behalf,
>>> even if you cc the person.
>>> 
>>> Andreas contributed code to GRUB (which is a GPLv3 project), and had
>>> no involvement whatsoever in creating this patch.
>>> 
>>> A signed-off-by is a statement on the part of the contributor (which
>>> may or may not be the author) that the contribution in question
>>> complies with the requirements imposed by the project in terms of
>>> copyright and licensing. Linux is GPLv2 not v3, so this code should at
>>> least be dual licensed in order to be reused directly.
>>> 
>>> I did not look at the GRUB patch, and IANAL, but this code invokes an
>>> Apple provided protocol (which is proprietary) in a hardcoded way for
>>> interoperability purposes, and so there is not much to
>>> copyright/license anyway. I would be fine with having just your
>>> signoff on this patch, but you could ask Andreas for a GPLv2+3 version
>>> of his GRUB patch if you are not sure.
>>> 
>> 
>> I believe this should be GPL2 compatible since it's simple reverse engineered Apple stuff and there are many kernel drivers with apple specific stuff.
>> 
>>>>> --- 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?
>>>> 
>>>> 
>>> 
>>> Agree that this is suboptimal. If we need a command line param for
>>> this, please make add it to the efi= list
>> 
>> I'll leave this to you. If you are fine with a parameter, I'll add it. If you have to be enforced by default, I'm fine with that.
>> 
>> Or, maybe we add add a parameter that disables this so that in case things break, we can atleast get it done.
>> 
>>>>> +struct apple_set_os_protocol {
>>> 
>>> This should be a union not a struct
>>> 
>>>>> +     u64 version;
>>> 
>>> This should be unsigned long
>>> 
>>>>> +     efi_status_t (__efiapi *set_os_version) (const char *);
>>>>> +     efi_status_t (__efiapi *set_os_vendor) (const char *);
>>>>> +     struct {
>>>>> +             u32 version;
>>> 
>>> ... to match the mixed_mode overlay which is u32. Alternatively, they
>>> could both be u64 but the current arrangement is definitely incorrect.
>>> 
>>>>> +             u32 set_os_version;
>>>>> +             u32 set_os_vendor;
>>>>> +     } mixed_mode;
>>>>> +};
>>>> 
>>>> How about declaring this __packed, just to be on the safe side?
>>>> 
>>> 
>>> I don't think that is necessary. If the mixed_mode overlay is never
>>> used, it doesn't really matter and you can use unsigned long vs u32,
>>> in which case all struct members are native word size so there is no
>>> padding issue.
>>> 
>>>> Why "mixed_mode"?  Seems like an odd name given "mixed mode"
>>>> in EFI context usually means 64-bit OS, but 32-bit EFI.
>>>> 
>>> 
>>> This is how the x86 plumbing works for mixed mode. Every EFI protocol
>>> is a union, with a mixed_mode member describing the 32-bit view. The
>>> accessor macros (efi_bs_call, efi_table_attr) automatically do the
>>> right thing depending on the bitness of the firmware.
>>> 
>>> This means, though, that even protocols that are known not to exist on
>>> 32-bit firmware need to be implemented in the same way, or the code
>>> will not build.
>>> 
>> So should I keep mixed mode or not?
> 
> Yes. To summarize:
> - keep your signoff as-is
> - drop the command line param and handling
> - make the outer protocol struct a union
> - use 'unsigned long' for the version field
> - keep the mixed_mode inner struct
> - reorganize the conditional in setup_quirks() so that there are two
> nested if blocks, where the memcmp() appears in the outer if() and
> apple_set_os() is only called on Apple hardware

That's for the summary Ard.

Sending a v2