| Message ID | 20250220144012.27057-1-l.anderweit@phytec.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] uboot-sign: Fix u-boot dtb signatures | expand |
Hi Leonard,
I've tested your patch and I wanted to let you know it worked fine for me both when FIT_SIGN_INDIVIDUAL="1" or "0". I've checked the contents of the u-boot dtb (for the presence of the required pubkeys) and the fitImage (for the signatures) and the results match what we had before commit d7bd9c62766 ("u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV enabled").
As for the patch, since the understanding is that when FIT_SIGN_INDIVIDUAL="1" the individual images will be signed besides the signing of the configurations then I'd say that sentence in the comment "Signing individual images is not recommended as that makes fitImage susceptible to mix-and-match attack" seems unnecessary/misleading to me since it gives the impression that one would get either images or configurations signed.
As for the check performed at build time by the "fit_check_sign" tool, the fact that now the check is done only on the configuration doesn't seem like a big loss to me. Though I imagine the ideal solution would be to have that check on the final fitImage rather than on a temporary one (unused.itb) in order to provide stronger guarantees that the image is correctly signed. However, this would likely complicate things which may make it not worth the effort...
Regards
Hi Leonard, I tested it on a couple of machines on my side and it fixes the regression. Tested-by: Jose Quaresma <jose.quaresma@foundries.io> Thanks for working on the solution. Jose Rogerio Guerra Borin via lists.openembedded.org <rogerio.borin= toradex.com@lists.openembedded.org> escreveu (quinta, 20/02/2025 à(s) 19:22): > Hi Leonard, > > I've tested your patch and I wanted to let you know it worked fine for me > both when FIT_SIGN_INDIVIDUAL="1" or "0". I've checked the contents of the > u-boot dtb (for the presence of the required pubkeys) and the fitImage (for > the signatures) and the results match what we had before commit d7bd9c62766 > ("u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and > UBOOT_ENV enabled"). > > As for the patch, since the understanding is that when > FIT_SIGN_INDIVIDUAL="1" the individual images will be signed besides the > signing of the configurations then I'd say that sentence in the comment > "Signing individual images is not recommended as that makes fitImage > susceptible to mix-and-match attack" seems unnecessary/misleading to me > since it gives the impression that one would get either images or > configurations signed. > > As for the check performed at build time by the "fit_check_sign" tool, the > fact that now the check is done only on the configuration doesn't seem like > a big loss to me. Though I imagine the ideal solution would be to have that > check on the final fitImage rather than on a temporary one (unused.itb) in > order to provide stronger guarantees that the image is correctly signed. > However, this would likely complicate things which may make it not worth > the effort... > > Regards > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#211775): > https://lists.openembedded.org/g/openembedded-core/message/211775 > Mute This Topic: https://lists.openembedded.org/mt/111289801/5052612 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [ > quaresma.jose@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- > >
diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass index 96c47ab01651..b2fcb5a31546 100644 --- a/meta/classes-recipe/uboot-sign.bbclass +++ b/meta/classes-recipe/uboot-sign.bbclass @@ -102,26 +102,36 @@ concat_dtb() { if [ -e "${UBOOT_DTB_BINARY}" ]; then # Re-sign the kernel in order to add the keys to our dtb - UBOOT_MKIMAGE_MODE="auto-conf" - # Signing individual images is not recommended as that - # makes fitImage susceptible to mix-and-match attack. - if [ "${FIT_SIGN_INDIVIDUAL}" = "1" ] ; then - UBOOT_MKIMAGE_MODE="auto" - fi ${UBOOT_MKIMAGE_SIGN} \ ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ - -f $UBOOT_MKIMAGE_MODE \ + -f auto-conf \ -k "${UBOOT_SIGN_KEYDIR}" \ -o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \ - -g "${UBOOT_SIGN_IMG_KEYNAME}" \ + -g "${UBOOT_SIGN_KEYNAME}" \ -K "${UBOOT_DTB_BINARY}" \ -d /dev/null \ -r ${B}/unused.itb \ ${UBOOT_MKIMAGE_SIGN_ARGS} + # Verify the kernel image and u-boot dtb ${UBOOT_FIT_CHECK_SIGN} \ -k "${UBOOT_DTB_BINARY}" \ -f ${B}/unused.itb + + # Signing individual images is not recommended as that + # makes fitImage susceptible to mix-and-match attack. + if [ "${FIT_SIGN_INDIVIDUAL}" = "1" ] ; then + ${UBOOT_MKIMAGE_SIGN} \ + ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ + -f auto \ + -k "${UBOOT_SIGN_KEYDIR}" \ + -o "${FIT_HASH_ALG},${FIT_SIGN_ALG}" \ + -g "${UBOOT_SIGN_IMG_KEYNAME}" \ + -K "${UBOOT_DTB_BINARY}" \ + -d /dev/null \ + -r ${B}/unused.itb \ + ${UBOOT_MKIMAGE_SIGN_ARGS} + fi cp ${UBOOT_DTB_BINARY} ${UBOOT_DTB_SIGNED} fi
With UBOOT_SIGN_ENABLE enabled commit 3fb215a3af24 (u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV enabled) always adds the signature of UBOOT_SIGN_IMG_KEYNAME to the u-boot dtb, independent of FIT_SIGN_INDIVIDUAL. The kernel fitimage configuration node is signed with UBOOT_SIGN_KEYNAME but the u-boot dtb contains the signature of UBOOT_SIGN_IMG_KEYNAME. U-boot is therefore unable to verify the signed kernel fitimage. Before that commit the signature of all keys used in the kernel fitimage would be added to the u-boot dtb. To fix this, always add the signature of UBOOT_SIGN_KEYNAME for configuration nodes to the u-boot dtb. If FIT_SIGN_INDIVIDUAL is 1 also add the signature of UBOOT_SIGN_IMG_KEYNAME for individual images. This has one drawback at the moment: The signing of individual images is not tested with fit_check_sign during concat_dtb. Fixes: 3fb215a3af24 (u-boot: kernel-fitimage: Fix dependency loop if UBOOT_SIGN_ENABLE and UBOOT_ENV enabled) Reported-by: Rogerio Guerra Borin <rogerio.borin@toradex.com> Signed-off-by: Leonard Anderweit <l.anderweit@phytec.de> --- Link to bug report: https://lists.openembedded.org/g/openembedded-core/topic/111218371#msg211507 --- meta/classes-recipe/uboot-sign.bbclass | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)