Hi, Any news about this regression? If we don't get a fix for this bug asap maybe we should revert and drop this patch since it broke our scarthgap stable LTS
Jose Rogerio Guerra Borin via lists.openembedded.org <rogerio.borin= [email protected]> escreveu (terça, 18/02/2025 à(s) 03:49): > > This message originated from outside your organization > > > > On 2/17/25 11:34 AM, Quentin Schulz wrote: > > > +Cc Marek > > > > Hi, > > Thanks for the quick answer! > > > > > > On 2/16/25 7:36 PM, Rogerio Guerra Borin via lists.openembedded.org > > wrote: > > >> You don't often get email from > > >> [email protected]. Learn why this is > > >> important<https://aka.ms/LearnAboutSenderIdentification > > <https://aka.ms/LearnAboutSenderIdentification>> > > >> Hello, > > >> > > >> At Toradex we're using class "kernel-fitimage" to produce our signed > > >> kernel FIT images and we've found that the recent commit d7bd9c627661 > > >> ("u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE > > >> and UBOOT_ENV enabled") broke our builds. The logs of a failed build > > >> show the following message that I found noteworthy: > > > > Can you mail me the entire log file for completeness ? > > Sure. > > I've added it as attachment (log.do_uboot_assemble_fitimage.841485). > > > >> log.do_uboot_assemble_fitimage.841485: > > >> ... > > >> Couldn't open RSA private key: '/secboot-data/fit-keys/dev2.key': No > > >> such file or directory > > >> > > >> The odd thing was that the build was setting: > > >> > > >> UBOOT_SIGN_KEYNAME = "dev" > > >> UBOOT_SIGN_IMG_KEYNAME = "dev2" > > >> FIT_SIGN_INDIVIDUAL = "0" > > >> FIT_GENERATE_KEYS = "0" > > > > FIT_SIGN_INDIVIDUAL = "0" is correct, you do want to sign > > configurations, not individual images in the fitImage, because signing > > individual images is susceptible to mix-and-match attack. > > > > >> From the above we see the individual images signing is disabled but > > >> the system is trying to access the key file for the individual > signing > > >> (dev2) regardless of that. Checking the code in uboot-sign.bbclass I > > >> found this line: > > >> > > >> https://git.openembedded.org/openembedded-core/tree/meta/classes- > > <https://git.openembedded.org/openembedded-core/tree/meta/classes-> > > >> recipe/uboot-sign.bbclass?h=scarthgap#n116 > > >> > > >> that is responsible for that access (inside function concat_dtb()). > > >> The code is always referencing variable UBOOT_SIGN_IMG_KEYNAME > > >> regardless of FIT_SIGN_INDIVIDUAL being set to "1" or "0" even though > > >> there's a condition a couple lines above L116 where > > >> FIT_SIGN_INDIVIDUAL is tested to decide in which mode to run the > > >> signing tool. > > > > Uh, good find, does the following patch fix your problem ? > > > > """ > > diff --git a/meta/classes-recipe/uboot-sign.bbclass > > b/meta/classes-recipe/uboot-sign.bbclass > > index 96c47ab016..74ef5697ca 100644 > > --- a/meta/classes-recipe/uboot-sign.bbclass > > +++ b/meta/classes-recipe/uboot-sign.bbclass > > @@ -107,13 +107,16 @@ concat_dtb() { > > # makes fitImage susceptible to mix-and-match attack. > > if [ "${FIT_SIGN_INDIVIDUAL}" = "1" ] ; then > > UBOOT_MKIMAGE_MODE="auto" > > + UBOOT_MKIMAGE_KEYNAME=${UBOOT_SIGN_IMG_KEYNAME} > > + else > > + UBOOT_MKIMAGE_KEYNAME=${UBOOT_SIGN_KEYNAME} > > fi > > ${UBOOT_MKIMAGE_SIGN} \ > > ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if > > len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ > > -f $UBOOT_MKIMAGE_MODE \ > > -k "${UBOOT_SIGN_KEYDIR}" \ > > -o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \ > > - -g "${UBOOT_SIGN_IMG_KEYNAME}" \ > > + -g "$UBOOT_MKIMAGE_KEYNAME" \ > > -K "${UBOOT_DTB_BINARY}" \ > > -d /dev/null \ > > -r ${B}/unused.itb \ > > """ > > Looking at the patch I'm pretty sure it would solve my problem, > particularly because we set FIT_SIGN_INDIVIDUAL="0" in our builds. > However, I see it keeps the concept of the signing being either on the > config or on the individual images, which I wanted to further comment > down below. > > > >> To better understand the behavior I created dev2. > > >> {key,crt} files and retried a build which worked. However, checking > > >> the contents of the final u-boot.dtb file I found that the "dev2" key > > >> node was present but the "dev" key wasn't. Then, checking the > contents > > >> of the final fitImage I noticed the configuration nodes were signed > by > > >> the dev key while the individual images were not signed (as > expected). > > >> But that means such a system wouldn't boot since the required "dev" > > >> key is not present in the U-Boot DTB, from what I conclude the FIT > > >> signing is broken at the moment. > > >> > > >> So, first of all I wanted to let you guys know about this issue > > >> (hoping we're not doing anything wrong with our builds). > > > > No, your builds are fine, this is a real bug. > > Thanks for the confirmation. > > > >> As a second point, I wanted to confirm the expected behavior > regarding > > >> variable FIT_SIGN_INDIVIDUAL. According to the documentation: > > >> > > >> https://docs.yoctoproject.org/5.0.6/ref-manual/variables.html#term- > > <https://docs.yoctoproject.org/5.0.6/ref-manual/variables.html#term-> > > >> FIT_SIGN_INDIVIDUAL > > >> > > >> when its value is "1" the individual images will be signed *in > > >> addition* to the FIT image being signed. > > > > That is a bit weird and no, I don't think that is how it is supposed to > > work. > > > > FIT_SIGN_INDIVIDUAL=1 -> only image subnodes in fitImage /images nodes > > are signed AND fitImage is susceptible to mix-and-match attack > > (basically attacker can replace individual signed images in fitImage > > with different individual signed images, e.g. with older versions, to > > achieve a combination of images which allows them to somehow break the > > system). > > > > FIT_SIGN_INDIVIDUAL=0 (recommended) -> images are hashed, configurations > > are hashed, hashes are combined and signed. Because images and > > configurations (which images are to be used together) are signed, > > attacker cannot mix-and-match images into an un-allowed configuration. > > > > >> From that, I understand that > > >> the configurations will be always signed and the individual images > may > > >> or may not be signed depending on FIT_SIGN_INDIVIDUAL. > > > > The other way around. The configurations are signed by default, which is > > good, but this can be disabled if needed (unlikely). > > > > >> I kind of > > >> confirmed this behavior by checking our build artifacts on kirkstone: > > >> with FIT_SIGN_INDIVIDUAL="0" the U-Boot DTB has only the "dev" key > and > > >> the fit image is signed by that same key; with > FIT_SIGN_INDIVIDUAL="1" > > >> both keys are present in the DTB, the configurations are signed by > the > > >> "dev" key and the images by the "dev2" key. I understand this is the > > >> correct behavior, right? > > > > Images shouldn't be signed by different keys than configurations, that > > is a bit odd. > > > > >> I'm asking because looking at the comment at: > > >> > > >> https://git.openembedded.org/openembedded-core/tree/meta/classes- > > <https://git.openembedded.org/openembedded-core/tree/meta/classes-> > > >> recipe/uboot-sign.bbclass?h=scarthgap#n106 > > >> > > >> it gives me the impression that FIT_SIGN_INDIVIDUAL would select > > >> between either signing individual images > > > > Yes > > > > >> or signing the > > >> configurations > > > > Signed configuration also includes hash of the images that the > > configuration references in it, that's what prevents the mix-and-match > > attack. > > > > >> , which doesn't match the documented behavior. I believe > > >> the code in concat_dtb() also seems to assume only one of the keys > > >> (dev/dev2) would need be present in the DTB. So the fix would involve > > >> a review on this as well I imagine. > > IIRC Adrian was looking into improving those docs. > > Regarding this second topic, let me show you some other information I > gathered from a couple of builds that may help with your assessment: > > FSI := FIT_SIGN_INDIVIDUAL > CFG := Whether or not the configurations inside the FIT image were signed > IMG := Whether or not the subimages inside the FIT image were signed > > | Branch | Version | FSI | Keys inside DTB | CFG | IMG | > |-----------+---------------+-----+------------------+-----+-----| > | kirkstone | latest | "0" | {dev} | Y | N | > | kirkstone | latest | "1" | {dev,dev2} | Y | Y | > |-----------+---------------+-----+------------------+-----+-----| > | scarthgap | latest | "0" | Build failed (!) | n/a | n/a | > | | + dummy dev2 | | {dev2} | Y | N | > | scarthgap | latest | "1" | {dev2} (!) | Y | Y | > |-----------+---------------+-----+------------------+-----+-----| > | scarthgap | w/o @d7bd9c62 | "0" | {dev} | Y | N | > | scarthgap | w/o @d7bd9c62 | "1" | {dev,dev2} | Y | Y | > > The first two lines show that on kirkstone when FIT_SIGN_INDIVIDUAL="0" > only the configurations are signed and the DTB contains only the "dev" > key. When FIT_SIGN_INDIVIDUAL="1" both "dev" and "dev2" are present in > the DTB and both the configurations and the images are signed. And > that's the exact same behavior I got on scarthgap after reverting the > commit d7bd9c62. And, as I said, this seems to match the documentation. > I've attached snippets of the fdtdump for the FIT image and U-Boot DTB > from my build on kirkstone to illustrate what I said. > > So, if the expected behavior is according to what you described then the > behavior so far was wrong (for a long time) and the new behavior is > introducing a breaking change for anyone relying on what's documented > for the FIT_SIGN_INDIVIDUAL="1" case, right? Btw, for us at Toradex this > is not a big deal because we don't fall into that case but considering > the general use of the layer the change in behavior could be an issue. > > Another piece of information that might be useful: I made a change to > concat_dtb() to make it add both keys (dev/dev2) to the DTB (by invoking > mkimage twice) and I found the invocation to "uboot-fit_check_sign" > failed afterwards. I confirmed that the DTB had the two keys, but > because the FIT image only had the configurations signed, the tool threw > an error saying the signature of "require key dev2" could not be > verified but the showed that the check of the "dev" key succeeded. > Unfortunately I don't have the logs available for that case, but I could > repeat the tests if needed. Anyway, my point is that the > "uboot-fit_check_sign" tool seems capable of checking both keys as well. > > Interestingly, if I removed the verification step (performed on the > "unused.itb" file), then the image generation succeeded and everything > seemed to work as before both when FIT_SIGN_INDIVIDUAL is "0" or "1". > So, it's not hard to get the old behavior, though we'd need to figure > out how to properly do the verification when mkimage is invoked with -f > "auto" or -f "auto-conf". I could post my unfinished patch if helpful. > > > > -- Best regards, José Quaresma
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#211753): https://lists.openembedded.org/g/openembedded-core/message/211753 Mute This Topic: https://lists.openembedded.org/mt/111218371/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
