NetBSD Problem Report #58075

From www@netbsd.org  Sun Mar 24 18:31:58 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 09BF31A9239
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 24 Mar 2024 18:31:58 +0000 (UTC)
Message-Id: <20240324183127.190531A923A@mollari.NetBSD.org>
Date: Sun, 24 Mar 2024 18:31:27 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: efi_bootdp memory corrupted by OpenProtocol on some machines
X-Send-Pr-Version: www-1.0

>Number:         58075
>Category:       kern
>Synopsis:       efi_bootdp memory corrupted by OpenProtocol on some machines
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    riastradh
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Mar 24 18:35:00 +0000 2024
>Closed-Date:    Fri Apr 19 02:31:34 +0000 2024
>Last-Modified:  Fri Apr 19 02:31:34 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The EFINetBSD Foundation
>Environment:
>Description:
The call to EFI_BOOT_SERVICES::OpenProtocol in efi_net_probe invalidates efi_bootdp:

https://nxr.netbsd.org/xref/src/sys/stand/efiboot/efinet.c?r=1.9#380

efi_net_probe (before OpenProtocol): Type=3 SubType=11 Length=37 @ 0xF896D698
  xx xx xx xx xx xx 00 00  00 00 00 00 00 00 00 00
  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  01 03 0C 1B 00

efi_net_probe (after OpenProtocol): Type=24 SubType=214 Length=63638 @ 0xF896D698
  00 00 00 00 B8 13 8A F8  00 00 00 00 00 00 00 00
  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  ...

After this happens, logic later on in efi_block_probe sometimes goes into an infinite loop in efi_device_path_depth trying to operate on the now-invalid efi_bootdp.

In revision 1.7, rin@ changed it to discard efi_bootp after calling OpenProtocol:

> MI efiboot: Stop using efi_bootdp after exclusive open for PXE
> 
> Once boot device is exclusively opened for Simple Network Protocol,
> further access via device path (efi_bootdp) is illegal.
> 
> For some implementations, boot device path gets corrupted by
> exclusive open, and subsequent access by efi_device_path_depth(),
> e.g., causes infinite recursion.
> 
> Fix PXE boot for QEMU/aarch64 with EDK2 on some Linux distributions.
> 
> Thanks yamaguchi@ for comments and tests.

https://nxr.netbsd.org/diff/src/sys/stand/efiboot/efinet.c?r2=%2Fsrc%2Fsys%2Fstand%2Fefiboot%2Fefinet.c%401.7&r1=%2Fsrc%2Fsys%2Fstand%2Fefiboot%2Fefinet.c%401.6

However, it was reverted in 1.9, leaving efi_bootp invalid and regressing to the bug with infinite loop in efi_device_path_depth:

> MI efiboot: Revert "Stop using efi_bootdp after exclusive open for PXE"
> 
> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/stand/efiboot/efinet.c#rev1.7
> 
> Some UEFI implementations pass multiple boot options as boot device path,
> and NULL-clearing it results in boot failures.
> 
> Thanks skrll@ for pointing it out. 

https://nxr.netbsd.org/diff/src/sys/stand/efiboot/efinet.c?r2=%2Fsrc%2Fsys%2Fstand%2Fefiboot%2Fefinet.c%401.9&r1=%2Fsrc%2Fsys%2Fstand%2Fefiboot%2Fefinet.c%401.8
>How-To-Repeat:
boot on a socionext synquacer
>Fix:
Yes, please!

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->rin
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Sun, 24 Mar 2024 18:39:26 +0000
Responsible-Changed-Why:
Can you take a look?  If I revert efinet.c 1.9, the bootloader works
without intervention -- it launches without going into an infinite
loop and it succeeds at netbooting my system.


From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Cc: rin@NetBSD.org, jakllsch@NetBSD.org
Subject: Re: kern/58075: efi_bootdp memory corrupted by OpenProtocol on some machines
Date: Sun, 24 Mar 2024 20:06:44 +0000

 This is a multi-part message in MIME format.
 --=_/7SKalmIntflQ5fRRqx18SV1eoaDZ/rT

 The attached patch on efiboot as of HEAD, without backing out efinet.c
 rev. 1.9, seems to make the bootloader work reliably on the Synquacer.
 Thoughts?

 --=_/7SKalmIntflQ5fRRqx18SV1eoaDZ/rT
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr58075-efibootdpcorrupt"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr58075-efibootdpcorrupt.patch"

 From 300bea0f4e29cd53c55edb16e49c03d1727cd0b8 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh@NetBSD.org>
 Date: Sun, 24 Mar 2024 19:18:11 +0000
 Subject: [PATCH] efiboot: Duplicate efi_bootdp before we clobber it in
  efi_net_probe.

 Patch from jakllsch@.  Makes Socionext Synquacer boot considerably
 more reliably.

 PR kern/58075
 ---
  sys/stand/efiboot/efiboot.c | 2 ++
  1 file changed, 2 insertions(+)

 diff --git a/sys/stand/efiboot/efiboot.c b/sys/stand/efiboot/efiboot.c
 index 03af54ae8627..8c444357beee 100644
 --- a/sys/stand/efiboot/efiboot.c
 +++ b/sys/stand/efiboot/efiboot.c
 @@ -86,6 +86,8 @@ efi_main(EFI_HANDLE imageHandle, EFI_SYSTEM_TABLE *system=
 Table)
  	status =3D uefi_call_wrapper(BS->HandleProtocol, 3, efi_li->DeviceHandle,=
  &DevicePathProtocol, (void **)&efi_bootdp);
  	if (EFI_ERROR(status))
  		efi_bootdp =3D NULL;
 +	else
 +		efi_bootdp =3D DuplicateDevicePath(efi_bootdp);
 =20
  #ifdef EFIBOOT_DEBUG
  	Print(L"Loaded image      : 0x%" PRIxEFIPTR "\n", efi_li);

 --=_/7SKalmIntflQ5fRRqx18SV1eoaDZ/rT--

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58075 CVS commit: src/sys/stand/efiboot
Date: Thu, 28 Mar 2024 18:24:57 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Thu Mar 28 18:24:57 UTC 2024

 Modified Files:
 	src/sys/stand/efiboot: efiboot.c

 Log Message:
 efiboot: Duplicate efi_bootdp before we clobber it in efi_net_probe.

 Patch from jakllsch@.  Makes Socionext Synquacer boot considerably
 more reliably.

 PR kern/58075


 To generate a diff of this commit:
 cvs rdiff -u -r1.22 -r1.23 src/sys/stand/efiboot/efiboot.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 29 Mar 2024 02:24:46 +0000
State-Changed-Why:
probably needs pullups, not sure if to 10 or also 9, probably not 8


State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 04 Apr 2024 19:16:03 +0000
State-Changed-Why:
pullup-10 #656
pullup-9 #1828
no aarch64 <9, so not going to bother


Responsible-Changed-From-To: rin->riastradh
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Thu, 04 Apr 2024 19:16:15 +0000
Responsible-Changed-Why:
took


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58075 CVS commit: [netbsd-10] src/sys/stand/efiboot
Date: Thu, 18 Apr 2024 15:32:36 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Apr 18 15:32:36 UTC 2024

 Modified Files:
 	src/sys/stand/efiboot [netbsd-10]: efiboot.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #656):

 	sys/stand/efiboot/efiboot.c: revision 1.23

 efiboot: Duplicate efi_bootdp before we clobber it in efi_net_probe.

 Patch from jakllsch@.  Makes Socionext Synquacer boot considerably
 more reliably.

 PR kern/58075


 To generate a diff of this commit:
 cvs rdiff -u -r1.22 -r1.22.4.1 src/sys/stand/efiboot/efiboot.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58075 CVS commit: [netbsd-9] src/sys/stand/efiboot
Date: Thu, 18 Apr 2024 15:33:44 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Apr 18 15:33:44 UTC 2024

 Modified Files:
 	src/sys/stand/efiboot [netbsd-9]: efiboot.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1828):

 	sys/stand/efiboot/efiboot.c: revision 1.23

 efiboot: Duplicate efi_bootdp before we clobber it in efi_net_probe.

 Patch from jakllsch@.  Makes Socionext Synquacer boot considerably
 more reliably.

 PR kern/58075


 To generate a diff of this commit:
 cvs rdiff -u -r1.16.4.1 -r1.16.4.2 src/sys/stand/efiboot/efiboot.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 19 Apr 2024 02:31:34 +0000
State-Changed-Why:
fixed and pulled up


>Unformatted:

Home	PR Database Search