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.
