On 2/20/25 18:04, Marek Vasut wrote:
On 2/18/25 4:49 AM, Rogerio Guerra Borin wrote:
[...]
>> 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.
That is correct, the signing is supposed to operate either on signed
configurations (where the configuration signature includes the hash of all the
images referenced by the configuration), or separate images (which is
susceptible to mix and match attack).
[...]
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.
This last part is a bit odd. The behavior you describe would imply that
FIT_SIGN_INDIVIDUAL="1" is some sort of setup which signs both configurations
(which include signature of referenced images) and images themselves, which would be
redundant.
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.
See the U-Boot documentation:
https://docs.u-boot.org/en/latest/usage/fit/signature.html
And especially this section:
https://docs.u-boot.org/en/latest/usage/fit/signature.html#signed-configurations
That is why I find the signing of both images and configurations odd.
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?
I believe FIT_SIGN_INDIVIDUAL="1" did not work correctly, yes. I also think
FIT_SIGN_INDIVIDUAL="1" should not be used, because of the mix and match attack, please
see U-Boot documentation above.
+CC Sean to get second opinion here.
(disclaimer: I have not been following the discussion closely)
IMO image signing should not be used, and really should come with a big fat
warning
or something. Ideally it should be removed in a future release (if possible;
not sure what
the OE perspective is on compatibility in this area). It remains in U-Boot only
for
compatibility; it probably should get some warnings there too so people don't
accidentally
use it for new boards.
--Sean
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.
I am not convinced adding the old behavior is the right thing to do, please see
above.
But I am also unsure what to do about people depending on the old behavior,
which I think is odd. Thoughts ?
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#211800):
https://lists.openembedded.org/g/openembedded-core/message/211800
Mute This Topic: https://lists.openembedded.org/mt/111218371/21656
Group Owner: openembedded-core+ow...@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
