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]

Date: Sun, 30 Jun 2024 13:29:15 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Lukas Wunner <lukas@...ner.de>
Cc: Aditya Garg <gargaditya08@...e.com>, 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

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.

> > --- 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

> > +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.

>
> > +static void apple_set_os(void)
> > +{
> > +     efi_guid_t guid = APPLE_SET_OS_PROTOCOL_GUID;
>
> My recollection is that if you don't declare this static const,
> gcc generates suboptimal code.  (It constructs the GUID on the
> stack at runtime instead of storing it in .rodata.)
> Maybe it's become smarter in the meantime, but I doubt it.
>

I don't remember the details but it looks like we stopped doing this a
while ago. I don't mind keeping it like this.