search

rhboot / shim

UEFI shim loader
Other
816 stars 284 forks source link

Null-terminate 'arguments' in fallback #611

Open vittyvk opened 10 months ago

vittyvk commented 10 months ago

In case CSV entry contains boot argument (e.g. an image to load for shim) it must be null-terminated. While populate_stanza() makes sure 'arguments' end with '\0', add_boot_option() doesn't account for it in 'size' calculations. E.g. for the following CSV entry:

 shimx64.efi,6.6.0-0.rc0.20230904git708283abf896.6.fc40.x86_64,\EFI\Linux\5f93b3c9cf1c488a99786fb8e99fb840-6.6.0-0.rc0.20230904git708283abf896.6.fc40.x86_64.efi,Comment

the resulting variable after 'fallback' looks like:

 # hexdump /sys/firmware/efi/efivars/Boot0004-8be4df61-93ca-11d2-aa0d-00e098032b8c | tail -3
 0000180 0038 0036 005f 0036 0034 002e 0065 0066
 0000190 0069
 0000192

Add trailing '\0' to 'size' calculations in add_boot_option() when 'arguments' is not empty. The resulting variable looks like:

 # hexdump /sys/firmware/efi/efivars/Boot0004-8be4df61-93ca-11d2-aa0d-00e098032b8c | tail -3
 0000180 0038 0036 005f 0036 0034 002e 0065 0066
 0000190 0069 0000
 0000194

and the specified image is loaded by shim without issues.

vittyvk commented 2 months ago

ping?

julian-klode commented 2 months ago

FWIW, the message is misleading you into thinking it's a single \0 byte, but it's a \0 UCS-2 character (i.e. 2 bytes).

But yes I believe this is useful. It's not strictly necessary: As I'm sure you know, you can always append a space to the entry in the .csv and it works. But it's an ugly workaround for sure.

I can't speak to whether the implementation is otherwise correct as I haven't looked at it in detail, mostly just tracing the reasoning with my work. Generally I'd favor not copying trailing \0s, but append them explicitly again, but I don't have enough context in the code here.

vittyvk commented 2 months ago

The workaround using extra space is indeed known and is already used in the wild, e.g. https://gitlab.com/kraxel/virt-firmware/-/commit/77d3801ccc963da747ba576da0fe586d352615d3 but it probably takes some time to discover it. It'd be nice to have a permanent solution.