regression: CopyMem() in ad8692e copies out of bounds #725
Conversation
|
I'm not 100% sure that we can really count on the string being null terminated since other places also seem to just copy the first 100 char16s. |
The CopyMem() introduced in "ad8692e avoid EFIv2 runtime services on Apple x86 machines" copies 100 CHAR16s no matter what. NX enabled firmware catches this and the boot breaks on those systems when the value is smaller than that and it's up against a page boundary with a page that's not mapped as readable. https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html says that FirmwareVendor is a pointer to a NUL terminated string that identifies the vendor that produces the system firmware for the platform. Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
|
Was there a reason not to just StrnCmp in place as in earlier commit of this PR? Possible (lack of) alignment?
|
| * So right now we're only copying ten bytes, because Apple is the | ||
| * only vendor we're testing against. | ||
| */ | ||
| CopyMem(vendorbuf, vendor, 10); |
There was a problem hiding this comment.
Apologies if I'm missing something, but vendorbuf is not used after this CopyMem, but rather vendor, which looks like a mistake.
There was a problem hiding this comment.
Ping @vathpela - I'm not sure if closed issues and merged PRs are monitored in this repo, i.e. should I create a new Issue?
There was a problem hiding this comment.
I guess also ping @jsetje
The CopyMem() introduced in
ad8692e avoid EFIv2 runtime services on Apple x86 machines copies 100 CHAR16s no matter what. NX enabled firmware catches this and the boot breaks on those systems.
https://uefi.org/specs/UEFI/2.10/04_EFI_System_Table.html says that FirmwareVendor is a pointer to a null terminated string that identifies the vendor that produces the system firmware for the platform. If we can rely on it being null terminated then we don't need to copy the string at all.