Attention is currently required from: Benjamin Doron, Felix Held, Lean Sheng Tan, Matt DeVillier, Maximilian Brune, Philipp Hug, ron minnich.
Julius Werner has posted comments on this change by Benjamin Doron. ( https://review.coreboot.org/c/coreboot/+/84796?usp=email )
Change subject: lib/{fit,fit_payload}.c: Enhance support for FIT images ......................................................................
Patch Set 9:
(12 comments)
File payloads/Kconfig:
https://review.coreboot.org/c/coreboot/+/84796/comment/3e6638da_4e27fa40?usp... : PS9, Line 186: default y if PAYLOAD_LINUX && (ARCH_ARM64 || ARCH_RISCV || ARCH_ARM) nit: unclear why these shift around?
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/92b79025_3d7d3db0?usp... : PS6, Line 26: 64
It's more maintainable, sure. Done. […]
Checking the return value would ensure that somebody will notice if you overrun the buffer for some reason (e.g. if the pattern string changes in the future). Otherwise you'll just have a half-written string in the device tree which might cause all sorts of tricky and hard to detect problems down the line.
https://review.coreboot.org/c/coreboot/+/84796/comment/d1512912_c8b6c62f?usp... : PS6, Line 31: dt_add_reg_prop(image_node, &addr, &size, 1, 2, 1);
I realised this (about the cells) after carefully looking through the UPL FDT, and I've now implemen […]
My understanding is that every node that has children with `reg` properties represents a bus, and the toplevel bus is memory. `ranges` (without value) tells the kernel that the child node uses the same bus (e.g. memory) as the parent node, while `ranges = <...>` can be used to define a translation between child bus and parent bus in some way. Not having any `ranges` on a node that has children with `reg` is illegal and the kernel will complain about it, because it thinks that you forgot to set up the correct translation. Putting `ranges` without a value is basically a way to say "yes, I've looked at the child bus and parent bus here and confirmed they are the same".
https://review.coreboot.org/c/coreboot/+/84796/comment/05a56d51_b2e3345e?usp... : PS6, Line 116: secondaries.size = kernel->size - primary_image->size;
Done. […]
You could underflow in the size if `region_size(kernel)` is smaller than `primary_image->size` for whatever reason, and `_untrusted()` could catch that. Just another opportunity for defensive programming, I agree that we don't need to treat the payload as potentially malicious if we're about to execute code from it anyway, but there might still be mistakes made somewhere.
https://review.coreboot.org/c/coreboot/+/84796/comment/dc6fedbb_759e5d1b?usp... : PS6, Line 131: continue;
Secondary images aren't necessarily crucial for the boot, and there are other reasons why we might f […]
Hmm... well, you're the expert on this boot mode but in general that seems fishy to me. If `require-fit` means the bootloader needs to load all images, but you don't manage to load a particular image, that's a failed boot. If you want the user to be able to recover from failed boots I would suggest using something like vboot's recovery mode feature, rather than just trying to boot half an image and hope for the best. And it's not something that should really fail randomly at runtime (so either you notice it immediately the first time you test a new image, and then you can easily fix it, or it should continue to keep working on an image that has managed to boot at least once). If the user wants to boot without that volume it's easy to fdtput it out of the image and reflash, so I'd just let them do that and not try to second-guess their intentions.
Especially if you ever plan to boot a system providing any sorts of security guarantees with this, I would very strongly suggest to always fail hard and fast rather than risk booting in an untested and unintended way that may be used as an attack vector.
https://review.coreboot.org/c/coreboot/+/84796/comment/7aefc0e3_0989fef1?usp... : PS6, Line 271: code.size += image_chain->image->size;
How is this not a concern for the primary image? We read in the data size and compression type the s […]
In the kernel case `fit_payload_arch()` determines the uncompressed size and and modifies the `kernel` region's size field before placing it in bootmem. That was one of the reasons we have to call out into arch code here, because the Linux kernel header formats are different for each architecture and we need to decompress and parse a little piece of it to figure out the uncompressed size. Your case would be easier there if `uncomp-size` is available for all non-kernel images.
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/1eca08ee_98474ed7?usp... : PS9, Line 22: printk(BIOS_CRIT, "WEAK: Generic code called to load a FIT kernel!\n"); nit: to make it a bit more clearer what it means to see this I'd maybe print something like "Booting Linux kernel FIT payloads not implemented for this architecture!\n" instead?
https://review.coreboot.org/c/coreboot/+/84796/comment/8918e4ee_096cf174?usp... : PS9, Line 29: printk(BIOS_ERR, "WEAK: %s:%s() called to add %s to FDT\n", __FILE__, __func__, name); Is this an error? If so, why does it need to be a weak function at all, rather than just a normal function that causes a compile error if not implemented?
I still don't quite get why you want to make this look like a platform hook tbh, it looks like this is a UPL thing, but UPL isn't really platform-specific. Isn't all this "firmware payload" stuff UPL-specific anyway? What else would it be used for? Why not just call the UPL function directly? Or do something like ``` if (CONFIG(HANDOFF_UPL_DEVICETREE)) upl_fdt_add_secondary(struct device_tree *tree, const char *name, struct region *secondary); ```
https://review.coreboot.org/c/coreboot/+/84796/comment/e7099fde_f63d981c?usp... : PS9, Line 250: if (!bootmem_walk(fit_place_mem, firmware)) You shouldn't need to walk if you only want one specific address, you can just use `bootmem_region_targets_type()`.
https://review.coreboot.org/c/coreboot/+/84796/comment/6ebb4e1d_93b17f8a?usp... : PS9, Line 307: if (secondary_image->load_address && secondary_image->load_address != region_offset(&secondary_region)) { Same here, I'd suggest to first check if the load address is given, and if it is just check `bootmem_region_targets_type()` and place it, if not you can walk (or just use `bootmem_allocate_buffer()` if the placement truly doesn't matter).
https://review.coreboot.org/c/coreboot/+/84796/comment/bdc23be3_c6656415?usp... : PS9, Line 401: bool status = false; FWIW, the use of a boolean to report the result of an "action" type function here was always wrong according to the code style (https://doc.coreboot.org/contributing/coding_style.html#function-return-valu...). This might be a good opportunity to change it to return cb_err instead.
https://review.coreboot.org/c/coreboot/+/84796/comment/f2603d6e_98e25616?usp... : PS9, Line 403: &fdt, &initrd Do either of these make sense for the firmware case? FDT is always going to be the runtime-generated coreboot FDT instead, and initrd is a very Linux-kernel-specific thing.
-
Julius Werner (Code Review)