https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=297643
Bug ID: 297643
Summary: libefivar: efibootmgr -v dies with SIGBUS on a boot
entry containing a USB WWID device path
Product: Base System
Version: 15.1-RELEASE
Hardware: Any
OS: Any
Status: New
Severity: Affects Some People
Priority: ---
Component: bin
Assignee: [email protected]
Reporter: [email protected]
Created attachment 273880
--> https://bugs.freebsd.org/bugzilla/attachment.cgi?id=273880&action=edit
The first line is the whole story: snprintf returns -1 for the UCS-2 data, and
UefiDevicePathLibCatPrint() assigns that to a UINTN
I am a FreeBSD user, not a developer. The observations below are from my
machine
and I have tried to check each one; the reading of the source was done with AI
assistance, so please treat my interpretation with caution — I may well have
misunderstood the intent of the code. The measurements I am more confident
about, because they are reproducible.
## What happens
On FreeBSD 15.1-RELEASE-p2 amd64:
```
$ efibootmgr # works, exit 0
$ efibootmgr -v
Bus error (core dumped)
```
Under lldb the crash is in `libefivar.so.1`, at
`efidp_format_device_path + 342`, on the instruction
```
cmpb $0x2c, (%rcx,%rax)
```
with `%rcx = 0x0000000000000000` and `%rax = 0xffff...` (a very large value).
## Narrowing it down
Only one of my boot entries triggers it:
```
$ for n in Boot0000 Boot2001 Boot2002 Boot2003 BootOrder BootCurrent; do
printf '%-12s ' "$n"
efivar --load-option --name "8be4df61-93ca-11d2-aa0d-00e098032b8c-$n" \
>/dev/null 2>&1 && echo ok || echo "rc=$?"
done
Boot0000 rc=138
Boot2001 ok
Boot2002 ok
Boot2003 ok
BootOrder ok
BootCurrent ok
```
`138` is `128 + SIGBUS`. `Boot0000` is a firmware-generated entry, 224 bytes:
```
0000: 01 00 00 00 55 00 45 00 46 00 49 00 20 00 55 00
0010: 53 00 42 00 20 00 44 00 65 00 76 00 69 00 63 00
0020: 65 00 20 00 28 00 47 00 65 00 6e 00 65 00 72 00
0030: 69 00 63 00 20 00 4d 00 61 00 73 00 73 00 53 00
0040: 74 00 6f 00 72 00 61 00 67 00 65 00 43 00 6c 00
0050: 61 00 73 00 73 00 29 00 00 00 03 10 22 00 00 00
0060: e3 05 64 07 30 00 30 00 30 00 30 00 30 00 30 00
0070: 30 00 30 00 32 00 39 00 36 00 34 00 03 11 05 00
0080: 01 04 01 2a 00 01 00 00 00 00 08 00 00 00 00 00
0090: 00 00 00 08 00 00 00 00 00 9b 21 bc ba 92 95 f1
00a0: 11 90 68 d4 93 90 57 9f a5 02 02 7f ff 04 00 52
00b0: 43
```
As far as I can tell that decodes as:
- `Attributes = 1`, `FilePathListLength = 0x55`
- Description `"EFI USB Device (Generic MassStorageClass)"`
- first device path node at `0x5A`: type `0x03`, subtype `0x10`
(`MSG_USB_WWID_DP`), length `0x22` = 34
- `InterfaceNumber = 0`, `VendorId = 0x05e3`, `ProductId = 0x0764`
- `SerialNumber`: the remaining 24 bytes = 12 `CHAR16`, `"000000002964"`
The other entries have no USB WWID node, which is consistent with only this one
failing.
## What I think is going on
Here I am least sure, so I have tried to test the part that can be tested
without UEFI at all.
`DevPathToTextUsbWWID()` in `lib/libefivar/efivar-dp-format.c:1076` formats the
serial number with `%S`:
```c
"UsbWwid(0x%x,0x%x,0x%x,\"%S\")",
```
`%S` on FreeBSD takes a `wchar_t *`, and `wchar_t` is 32 bits here, whereas
`CHAR16` is 16. A short program with the exact bytes from `Boot0000`:
```c
uint16_t serial[] = { '0','0','0','0','0','0','0','0','2','9','6','4',
0x1103, 0x0005, 0x0401, 0x2a01, 0x0100, 0x0000 };
setlocale(LC_ALL, "C.UTF-8");
n = snprintf(NULL, 0, "UsbWwid(0x%x,0x%x,0x%x,\"%S\")",
0, 0x05e3, 0x0764, (wchar_t *)serial);
```
prints
```
sizeof(wchar_t) = 4
snprintf mit UCS-2 an %S : n = -1, errno = 86 (Illegal byte sequence)
snprintf mit echtem wchar_t: n = 14, errno = 0
```
(Full source in the attachment.) Read as 32-bit values, `"00"` becomes
`0x00300030` = 3145776, which is beyond `0x10FFFF`, so the conversion reports
`EILSEQ` and `snprintf` returns `-1`. My locale is `C.UTF-8`; prefixing
`LC_ALL=C.UTF-8` changes nothing, so I do not think this is a locale issue.
The comment at `efivar-dp-format.c:54` says
```
* %s -> %S in spots (where it is still UCS-2)
```
which is what made me look here. If `%S` were expected to handle UCS-2, that
would explain the choice — but on FreeBSD it appears not to. Am I reading this
correctly?
If `snprintf` does return `-1` there, then `UefiDevicePathLibCatPrint()` in the
same file seems to carry it forward:
```c
UINTN Count;
...
Count = vsnprintf (NULL, 0, Fmt, Args);
...
if ((Str->Count + (Count + 1)) > Str->Capacity) {
Str->Capacity = (Str->Count + (Count + 1) * 2);
Str->Str = reallocf (Str->Str, Str->Capacity);
ASSERT (Str->Str != NULL);
}
vsnprintf (Str->Str + Str->Count, Str->Capacity - Str->Count, Fmt, Args);
Str->Count += Count;
```
`Count` is `UINTN`, i.e. unsigned, so `-1` becomes `SIZE_MAX`. Then `Count + 1`
is `0`, the capacity test is `Str->Count + 0 > Str->Capacity`, which for a
freshly
zeroed `POOL_PRINT` is `0 > 0` and therefore false — so nothing is allocated
and
`Str->Str` stays `NULL`. Afterwards `Str->Count += SIZE_MAX`.
The caller (`UefiDevicePathLibConvertDevicePathToText`, around line 2553) then
does, on the next node:
```c
if ((Str.Count != 0) && (ToText != DevPathToTextEndInstance)) {
if (Str.Str[Str.Count] != ',') {
```
`Str.Count` is not zero, and `Str.Str` is `NULL`, so this dereferences
`NULL + SIZE_MAX`. That matches the faulting instruction and both register
values above, which is why I believe this is the path — though I would not be
surprised to be wrong about some step.
## A second difference from the EDK2 original
Since this file derives from EDK2's `UefiDevicePathLib`, I compared it against
`MdePkg/Library/UefiDevicePathLib/DevicePathToText.c` in current EDK2 master.
One line in `DevPathToTextUsbWWID()` differs, and I do not think the difference
is intentional:
```c
/* EDK2 master */
SerialNumberStr = (CHAR16 *)((UINT8 *)UsbWWId + sizeof (USB_WWID_DEVICE_PATH));
/* FreeBSD lib/libefivar/efivar-dp-format.c */
SerialNumberStr = (CHAR16 *)(&UsbWWId + 1);
```
`UsbWWId` is a local variable of type `USB_WWID_DEVICE_PATH *`. `&UsbWWId` is
therefore the address of that local, and `+ 1` advances by the size of a
pointer — so `SerialNumberStr` appears to point into the stack frame rather
than
at the serial number inside the device path node. `Length` is still computed
correctly from `DevicePathNodeLength()`, so the following
```c
if ((Length >= 1) && (SerialNumberStr[Length - 1] != 0)) {
```
would be reading stack memory, and whatever is handed to the format call would
be stack contents.
I am not certain this is FreeBSD-introduced. The form with the ampersand looks
like it may have come from an older EDK2 snapshot, and I have not traced when
EDK2 changed it — that seemed better left to someone who knows the import
history.
I withdraw a point I had in an earlier draft, that the `SerialNumber` field is
not NUL-terminated and would be read past: it is true that the field is not
terminated (12 `CHAR16` fill the node length exactly), but the code already
handles that — it copies into a new buffer and terminates it. So that is not a
defect, and with the pointer above pointing elsewhere it would not help anyway.
## Two smaller things, possibly deliberate
1. `Str.Str[Str.Count]` indexes one past the last character. After
`UefiDevicePathLibCatPrint()` returns, `Str->Count` is the string length, so
that position holds the NUL terminator rather than the final character. If
the intent is "did the previous node already end with a comma", `Count - 1`
would seem to be meant; as written the comparison looks like it can never be
true, and a `/` is always appended.
This one is **not** FreeBSD-specific: EDK2 master has the identical
`if (Str.Str[Str.Count] != L',')`. So it is presumably inherited, and if it
is
a defect it is one to raise upstream rather than here.
2. `ASSERT()` is defined empty in `lib/libefivar/uefi-dplib.h:524`, so
`ASSERT (Str->Str != NULL)` after `reallocf()` does not check anything.
`reallocf` frees on failure and returns `NULL`, while `Str->Count` keeps its
old value — which would reach the same dereference as above. EDK2 added an
explicit `if (NewStr == NULL) { return; }` next to its ASSERTs in this
function; the FreeBSD copy does not have that.
## Which parts look FreeBSD-specific
For whatever it is worth, comparing with EDK2 master:
| | EDK2
| FreeBSD |
|-----------------------|-----------------------------------------------------------------------------|----------------------------------------------------|
| `CatPrint` formatting | `SPrintLength()` / `UnicodeVSPrint()`, `CHAR16`
throughout, no error return | `vsnprintf()`, whose `-1` is assigned to a
`UINTN` |
| serial number format | `%s` with `CHAR16 *`
| `%S`, which takes `wchar_t *` here |
| `SerialNumberStr` | `(UINT8 *)UsbWWId + sizeof (...)`
| `&UsbWWId + 1` |
| separator check | `Str.Str[Str.Count]`
| identical |
So the crash itself seems to come from the parts that were adapted, and the
separator check appears to be inherited. I could easily be wrong about the
history.
## Workaround
`efibootmgr` without `-v` is unaffected, and the other subcommands appear to
work. I have not tried deleting `Boot0000`; it is firmware-generated, so I
expect it to come back.
## What I can do
I am happy to test a patch, provide more of the backtrace, or dump any other
variable. I cannot judge which of the three points above is the right place to
fix, or whether some of this is inherited from the EDK2 sources this file
derives from — I have not compared against them.
--
You are receiving this mail because:
You are the assignee for the bug.
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.