New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UKI: add support for choosing between multiple device trees #28959
base: main
Are you sure you want to change the base?
Conversation
src/boot/efi/devicetree.c
Outdated
@@ -1,5 +1,7 @@ | |||
/* SPDX-License-Identifier: LGPL-2.1-or-later */ | |||
|
|||
#include <endian.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header is not provided by the compiler and therefore not guaranteed to be safe to use in freedstanding env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thank you for the review, I will use another header file then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, what should the approach here be? Should be32toh() be defined just for this purpose (just on this file) or should it go somewhere in src/fundamental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's the only user, you can keep it in the file and just define it with __builtin_bswap*
directly. UEFI is little endian exclusively, so no point in dealing with the other case.
src/boot/efi/devicetree.c
Outdated
if (be32toh(dt_header->magic) != 0xd00dfeed) | ||
return NULL; | ||
|
||
while (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while (true) { | |
for (;;) { |
src/boot/efi/devicetree.c
Outdated
case FDT_PROP: | ||
len = be32toh(*cursor++); | ||
|
||
if (strcmp8(string_block + be32toh(*cursor++), "compatible") == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (strcmp8(string_block + be32toh(*cursor++), "compatible") == 0) | |
if (streq8(string_block + be32toh(*cursor++), "compatible")) |
src/boot/efi/devicetree.c
Outdated
if (strcmp8(string_block + be32toh(*cursor++), "compatible") == 0) | ||
return (char *)cursor; | ||
|
||
cursor += (len + 3) / 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These numbers are for rounding up the cursor to the next word, since len can be any integer. However I can change how this is done if this is not so clear to see when looking at the code
src/boot/efi/devicetree.c
Outdated
@@ -104,6 +106,65 @@ EFI_STATUS devicetree_install(struct devicetree_state *state, EFI_FILE *root_dir | |||
MAKE_GUID_PTR(EFI_DTB_TABLE), PHYSICAL_ADDRESS_TO_POINTER(state->addr)); | |||
} | |||
|
|||
static char* devicetree_get_compatible(const void *dtb) { | |||
const struct fdt_header *dt_header = dtb; | |||
uint32_t *cursor = (uint32_t *) ((char *)dt_header + be32toh(dt_header->off_dt_struct)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use uint_8*
for raw byte access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will fix
src/boot/efi/devicetree.c
Outdated
if (err != EFI_SUCCESS) | ||
log_error_status(err, "No matching device tree found in UKI."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No matching dt is not an error here. You ought to log about bad dtbs, though: e.g. !IN_SET(EFI_SUCCESS, EFI_NOT_FOUND)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the behavior here should be if(!IN_SET(err, EFI_SUCCESS, EFI_NOT_FOUND)) { log() }
?
#define FDT_NOP 0x00000004 | ||
#define FDT_END 0x00000009 | ||
|
||
struct fdt_header { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally use CamelCase for (new) structs.
src/boot/efi/devicetree.h
Outdated
uint32_t magic; | ||
uint32_t totalsize; | ||
uint32_t off_dt_struct; | ||
uint32_t off_dt_strings; | ||
uint32_t off_mem_rsvmap; | ||
uint32_t version; | ||
uint32_t last_comp_version; | ||
uint32_t boot_cpuid_phys; | ||
uint32_t size_dt_strings; | ||
uint32_t size_dt_struct; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is wrong.
src/boot/efi/devicetree.c
Outdated
} | ||
} | ||
|
||
static EFI_STATUS devicetree_match(const void **dtb_buffer, size_t *dtb_length, const void *fw_dtb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please assert input pointers.
src/ukify/ukify.py
Outdated
for file in dtbs: | ||
dtb = file.read_bytes() | ||
n = len(dtb) | ||
padding = b'\0' * (round_up(n, 8) - n) # pad to 64 bit alignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
padding = b'\0' * (round_up(n, 8) - n) # pad to 64 bit alignment | |
padding = bytes(round_up(n, 8) - n) # pad to 64 bit alignment |
src/boot/efi/devicetree.c
Outdated
@@ -104,6 +106,65 @@ EFI_STATUS devicetree_install(struct devicetree_state *state, EFI_FILE *root_dir | |||
MAKE_GUID_PTR(EFI_DTB_TABLE), PHYSICAL_ADDRESS_TO_POINTER(state->addr)); | |||
} | |||
|
|||
static char* devicetree_get_compatible(const void *dtb) { | |||
const struct fdt_header *dt_header = dtb; | |||
uint32_t *cursor = (uint32_t *) ((char *)dt_header + be32toh(dt_header->off_dt_struct)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, you area dereferencing an offset here that originates in untrusted data, no? that's no ok, you need to validate this against the size of the dtb object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, alignment is weird here, the offset might not be 32bit aligned yet you just dereference the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will add these checks in the updated version
src/boot/efi/devicetree.c
Outdated
char *string_block = (char *) ((char *)dt_header + be32toh(dt_header->off_dt_strings)); | ||
uint32_t len; | ||
|
||
if (be32toh(dt_header->magic) != 0xd00dfeed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use UINT32_C() around the constant, since otherwise the constant is a bit weird, since the values is not expressible as a signed int, yet it is made one, and then converted back to unsigned
|
||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this lacks proper validation of the returned data, i.e. that the returned string is NUL terminated, that all the offsets fit in the dtb blob before we deref them, that we don#t do unaligned accesses and so on.
also this behaviour needs some documentation somwhere, i.e. sd-stub man page or so |
I think we need a more generalized approach. Appending like this might work for DTBs, but I don't think it will work for command lines or other stuff that we want to make selectable among multiple options. We should find a way to use separate sections for each payload I think. @medhefgo does the PE spec/tooling allow multiple sections with the same name? |
Theoretically, yes. |
Is there an advantage to keeping the same section name or would it be possible to name the extra sections .dtb-* as well? |
I think using the same name would be easier, as the definition doesn't need to change, the names are fixed and not dynamic, and appending doesn't need to care about current state |
How big (roughly) is an average dtb and how many would typically be added to an image? |
For the use case that we are targeting it would be something around 5 dtbs, each around 50 to 80 kB |
I'd prefer not to. the section name is such a nice identifier, let's maintain that as a unique handle. We could just say, that if multiple alternatives of specific sections shall be allowed, we define a clear algorithm how to generate section names from the "primary" section name plus some counter. i.e. take the section name, and suffix it with a decimal counter starting from 2. If this would be longer than 8ch then shorten the name before appending the suffix so that it fits. Hence, for .initrd the next alternatives would be .initrd2, .initrd3, .initrd4, …, .initr65, … .init113, … For .cmdline (which already is 8ch long), the alternative names would be .cmdlin2, .cmdlin3, .cmdlin4, …, .cmdli64, …, .cmdl113, … you get the idea. |
That's why I think it's nicer if the name is fixed, so that it's an easy and obvious api, no? It's what we already do for every other setting after all - eg: you add multiple ExecStart=, you don't add ExecStart1= ExecStart2= etc.
This seems quite complex and error prone, and not very nice on the eyes. We'd need to start worrying about clashes when there's enough entries and other unpleasantness. Every producer/parser will need to implement it exactly right. Can't say I like that scheme. |
FWIW I agree with @bluca that keeping the same section name leads to a simpler design. I can implement this and update the pull request |
6e80a8c
to
3e23b8c
Compare
src/boot/efi/devicetree.c
Outdated
@@ -5,6 +5,7 @@ | |||
#include "util.h" | |||
|
|||
#define FDT_V1_SIZE (7*4) | |||
#define be32toh(x) __builtin_bswap32(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for paranoia, can you add change this to:
#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
#define be32toh(x) __builtin_bswap32(x)
#else
#error "Unexpected by order in EFI mode?"
#endif
this is formalized documentation if you so will, explaining a bit why we can just get away with defining be32toh like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
efi.h already asserts little endian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, but that's not visible here. here it's a bit surprising to have this lone definition of a byteswapping helper that hardcodes this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indent the nested defines by two spaces, # define
, # error
, etc.
src/boot/efi/devicetree.c
Outdated
uint32_t strings_off = be32toh(dt_header->OffDTStrings); | ||
uint32_t strings_size = be32toh(dt_header->SizeDTStrings); | ||
|
||
if (struct_off % sizeof(uint32_t) || struct_size % sizeof(uint32_t) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in our codebase we have the rule that for numerical values (i.e. values that are not boolean or boolean-like) we want explicit numernical comparisons for zero/non-zero.
i.e. to match our coding style, please write:
if (struct_off % sizeof(uin32_t) != 0 || …
you get the idea
src/boot/efi/devicetree.c
Outdated
uint32_t strings_size = be32toh(dt_header->SizeDTStrings); | ||
|
||
if (struct_off % sizeof(uint32_t) || struct_size % sizeof(uint32_t) || | ||
strings_off + strings_size > dt_size || struct_off + struct_size > strings_off) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overflow checks missing
src/boot/efi/devicetree.c
Outdated
strings_off + strings_size > dt_size || struct_off + struct_size > strings_off) | ||
return NULL; | ||
|
||
uint32_t *cursor = (uint32_t *) ((uint8_t *) dt_header + struct_off); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are casting the const
away here. please don't
src/boot/efi/devicetree.c
Outdated
return NULL; | ||
|
||
uint32_t *cursor = (uint32_t *) ((uint8_t *) dt_header + struct_off); | ||
char *strings_block = (char *) ((uint8_t *) dt_header + strings_off); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dito
src/boot/efi/devicetree.c
Outdated
uint32_t *cursor = (uint32_t *) ((uint8_t *) dt_header + struct_off); | ||
char *strings_block = (char *) ((uint8_t *) dt_header + strings_off); | ||
|
||
for (uint32_t i = 0; i < struct_size - 2; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that at this point you are indexing memory, and we generally do that with size_t
i'd type i
as size_t here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you didn#t check for underflow of struct_size
here first
@diogo-ivo It'd be great if we could merge this for v255-rc1. Please post an update. |
@keszybz Since we are interested in this feature too, if the original author no longer wishes to work on it, I think I can find some time next week to give this a facelift. |
we must ensure that authenticode calculation is stable, in the face of reordering sections. Or for example, we should restrict the spec that all ".dtb" sections must be one after the other. At this point I am not even sure if this matters, or if this is a red herring. |
Separately thinking about a UKI which has one optional dtb, to be used when booting on real hardware. But not required at all when booting in qemu vm. I hope we have explicit test for this, of not loading dtb at all if it doesn't match at all. I believe currently we always load dtb if present even if it doesn't match the hw. And this is a behaviour change. Unless we simply require for dtbs to match, or like if one wants to force load the dtb, the dtb compatible must have a flag value for sd-boot to recommend always loading it. |
I wouldn't think that was any different than reordering any other section? Ukify processes them in a fixed order. As long as the input is the same, then output should be the same too, unless I'm missing something. |
maybe it makes sense for ukify to write things out in the "canonical order" of sections which we use to measure them, as defined in the UKI spec. And maybe we should order the dtb sections by their binary contents (and maybe even suppress duplicates?) That way, the order in which things are specified doesn't matter anymore, we'll build the same PE files no matter what it only matters that you have the same inputs, not in which order you give them to ukify. |
This definitely makes sense
This seems more difficult, especially to define in a spec for cross-compatibility? Order of input seems easier to me |
Add support for matching the DT contained in a .dtb section of the UKI image against the FW provided FDT.
Add support for selecting from multiple .dtb sections contained in the UKI image by matching against the FW-provided DT.
Document the behaviour of the stub when it finds multiple DTs embedded in the UKI.
Add support for building UKIs with multiple DeviceTrees, each embedded in its own .dtb section.
54ada47
to
e629f67
Compare
Thank you for the input, this version should address the comments made above except for the systemd-measure/ordering part, which I'll add when we reach an agreement on if/how we should order the sections |
|
just do a sort by binary memcmp() (and if one section is a prefix of another, then put the shorter one first). |
sections. The stub will try to match the DeviceTrees embedded in the PE image with the DeviceTree | ||
provided by the device's firmware by comparing the first <literal>compatible</literal> string in each UKI DeviceTree | ||
with the first <literal>compatible</literal> string supplied by the firmware. If no match is found the first | ||
<literal>.dtb</literal> section in the image is selected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no match is found the first
.dtb
section in the image is selected.
worries me. as that will break booting the same uki in a QEMU KVM and hardware. Because one should not override the QEMU KVM provided DTB.
I am also worried about systems that are pure ACPI and thus do not have DeviceTree provided by the firmware -> because then we need to probably extend dtb with things to match on, and likely match on like SMBIOS3 table vendor / laptop SKU. This is to support all the Windows On Arm laptops.
I wonder if we can change this to:
If UKI has only one dtb, it will always be loaded. If UKI has more than one dtb, and none of them match, none of them will be loaded.
As that is both backwards compatible, and requires strict match out of multiple dtbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to require a strict match, if more than one dtb is present in a UKI. And do not load any dtb, if there is no match out of multiple dtbs.
size_t offset = in_memory ? sect->VirtualAddress : sect->PointerToRawData; | ||
size_t size = sect->VirtualSize; | ||
|
||
sizes[j] = offsets[j] ? : size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be sizes[j] = sizes[j] ?: size;
?
Not only that, but this whole logic is written in a rather confusing fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that previously but changed it due to the comment 1. However in the version I'm working on this is no longer an issue since this logic cannot be here as we need to find all .dtb sections to correctly compute digests.
60-ukify.install also needs to be updated:
|
Not strictly related to this patch, but I am working on a revised version of this, and confused about a few things:
|
Because fully and meaningfully testing this stuff with qemu (no real hardware) is next to impossible, so if you find problems, please file PRs to fix them, as only some very basic and minimal validation was done |
@bluca I also only have qemu as a test setup as far as ARM UEFI goes unfortunately, but I will try to fix whatever I can. However the addon dtb case, I am not sure how to fix exactly as the configuration table mechanism simply just doesn't provide way to have multiple configuration tables with the same GUID. Would concatenating the DTBs into one configuration table make downstream consumers of it happy, or does everyone just try to (incorrectly) search for multiple config tables with the same GUID? |
If the actual goal is to only provide a singular dtb to the kernel via a singular configuration table, there would have to be some kind of mechanism to collect all dtb from built-in ones from the image and from addons and somehow select a compatible one (from multiple potential candidates). I can definitely code something from gut feeling, but it would be nice to have some kind of consensus of what everyone considers to be the "right thing". |
@kukrimate we can also provide you both u-boot based & edk2 based hardware (i.e. u-boot / systemready things, and like full edk2 stuff like Windows on Arm laptops). |
In connection with the discussion in 1 this pull request implements a possible solution where further device trees are appended to the .dtb section and the stub tries to match the FW provided device tree with the ones present in the UKI. It also adds the necessary changes to ukify for it to be able to build such UKIs. The specification change can be found in 2.