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 10:04:39 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Aditya Garg <gargaditya08@...e.com>
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

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.

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

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

Why "mixed_mode"?  Seems like an odd name given "mixed mode"
in EFI context usually means 64-bit OS, but 32-bit EFI.

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

Thanks,

Lukas