Received-SPF: pass (google.com: domain of linux-kernel+bounces-208954-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 10 Jun 2024 23:53:05 +0200
Precedence: bulk
Mime-Version: 1.0
Message-ID: <20240610215304.2953611-2-ardb+git@google.com>
Subject: [PATCH] efi/x86: Clear BSS when entering in mixed mode via compat entrypoint
From: Ard Biesheuvel <ardb+git@google.com>
To: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Ard Biesheuvel <ardb@kernel.org>, 
	Nicholas Bishop <nicholasbishop@google.com>
Content-Type: text/plain; charset="UTF-8"

From: Ard Biesheuvel <ardb@kernel.org>

Nicholas reports that since commit

  df7ecce842b8 ("x86/efistub: Don't clear BSS twice in mixed mode")

booting in mixed mode via the compat entrypoint is broken on firmware/loader
combinations where the PE/COFF executable is entered with BSS containing
garbage.

The above commit changed the logic because clearing BSS in the 64-bit
entrypoint is too late when booting in mixed mode, as at this point, BSS
variables may have already been modified by the program, and so
reverting it is not an option.

So instead, fix this issue by moving the existing mixed mode BSS
clearing logic to the 32-bit startup routine that is shared between the
different mixed mode boot protocols, so that it is called immediately
after entering from the firmware in all cases.

Fixes: df7ecce842b8 ("x86/efistub: Don't clear BSS twice in mixed mode")
Reported-by: Nicholas Bishop <nicholasbishop@google.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/efi_mixed.S | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/x86/boot/compressed/efi_mixed.S b/arch/x86/boot/compressed/efi_mixed.S
index 876fc6d46a13..a4035c426229 100644
--- a/arch/x86/boot/compressed/efi_mixed.S
+++ b/arch/x86/boot/compressed/efi_mixed.S
@@ -152,16 +152,6 @@ SYM_FUNC_START(efi32_stub_entry)
 	call	1f
 1:	popl	%ecx
 	leal	(efi32_boot_args - 1b)(%ecx), %ebx
-
-	/* Clear BSS */
-	xorl	%eax, %eax
-	leal	(_bss - 1b)(%ecx), %edi
-	leal	(_ebss - 1b)(%ecx), %ecx
-	subl	%edi, %ecx
-	shrl	$2, %ecx
-	cld
-	rep	stosl
-
 	add	$0x4, %esp		/* Discard return address */
 	popl	%ecx
 	popl	%edx
@@ -264,12 +254,24 @@ SYM_FUNC_START_LOCAL(efi32_entry)
 	/* Store firmware stack pointer */
 	movl	%esp, (efi32_boot_sp - 1b)(%ebx)
 
+	/* Take the address of _bss in %edi */
+	movl	$_bss - 1b, %esi
+	leal	(%ebx, %esi), %edi
+
 	/* Store boot arguments */
 	leal	(efi32_boot_args - 1b)(%ebx), %ebx
 	movl	%ecx, 0(%ebx)
 	movl	%edx, 4(%ebx)
 	movb	$0x0, 12(%ebx)          // efi_is64
 
+	/* Clear BSS */
+	xorl	%eax, %eax
+	movl	$_ebss - 1b, %ecx
+	subl	%esi, %ecx
+	shrl	$2, %ecx
+	cld
+	rep	stosl
+
 	/*
 	 * Allocate some memory for a temporary struct boot_params, which only
 	 * needs the minimal pieces that startup_32() relies on.
-- 
2.45.2.505.gda0bf45e8d-goog