| Message ID | 20240902161307.1222507-1-alexander.sverdlin@siemens.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v3] kernel-fitimage: make signing failure fatal | expand |
Hi, On Mon, Sep 02, 2024 at 06:13:06PM +0200, A. Sverdlin via lists.openembedded.org wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > mkimage doesn't fail if it is not able to sign FIT nodes. > This may lead to unbootable images in secure boot configurations. > Make signing failures fatal by parsing the mkimage output. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > --- > Changes in v3: > - bbfatag_log -> bberror + bbfatal_log with relevant mkimage output snippets > Changes in v2: > - bbfatal -> bbfatal_log > > meta/classes-recipe/kernel-fitimage.bbclass | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass > index 67c98adb232..ccf848e643f 100644 > --- a/meta/classes-recipe/kernel-fitimage.bbclass > +++ b/meta/classes-recipe/kernel-fitimage.bbclass > @@ -753,11 +753,16 @@ fitimage_assemble() { > # Step 8: Sign the image > # > if [ "x${UBOOT_SIGN_ENABLE}" = "x1" ] ; then > - ${UBOOT_MKIMAGE_SIGN} \ > + output=$(${UBOOT_MKIMAGE_SIGN} \ Will this subshell return errors as before or is "set -e" propagated there? > ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ > -F -k "${UBOOT_SIGN_KEYDIR}" \ > -r ${KERNEL_OUTPUT_DIR}/$2 \ > - ${UBOOT_MKIMAGE_SIGN_ARGS} > + ${UBOOT_MKIMAGE_SIGN_ARGS}) > + echo "$output" > + if err=$(echo "$output" | grep -C9 -E "Sign value:\s*unavailable"); then > + bberror "${UBOOT_MKIMAGE_SIGN} failed to provide signatures for these images:" > + bbfatal_log "\n$err" Is the problem really in mkimage since it does not return errors when signing fails? Cheers, -Mikko
Hi Mikko! On Tue, 2024-09-03 at 09:03 +0300, Mikko Rapeli wrote: > > mkimage doesn't fail if it is not able to sign FIT nodes. > > This may lead to unbootable images in secure boot configurations. > > Make signing failures fatal by parsing the mkimage output. > > > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > --- > > Changes in v3: > > - bbfatag_log -> bberror + bbfatal_log with relevant mkimage output snippets > > Changes in v2: > > - bbfatal -> bbfatal_log > > > > meta/classes-recipe/kernel-fitimage.bbclass | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass > > index 67c98adb232..ccf848e643f 100644 > > --- a/meta/classes-recipe/kernel-fitimage.bbclass > > +++ b/meta/classes-recipe/kernel-fitimage.bbclass > > @@ -753,11 +753,16 @@ fitimage_assemble() { > > # Step 8: Sign the image > > # > > if [ "x${UBOOT_SIGN_ENABLE}" = "x1" ] ; then > > - ${UBOOT_MKIMAGE_SIGN} \ > > + output=$(${UBOOT_MKIMAGE_SIGN} \ > > Will this subshell return errors as before or is "set -e" propagated there? Good point, I need to test if I'm not masking real errors here... > > ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ > > -F -k "${UBOOT_SIGN_KEYDIR}" \ > > -r ${KERNEL_OUTPUT_DIR}/$2 \ > > - ${UBOOT_MKIMAGE_SIGN_ARGS} > > + ${UBOOT_MKIMAGE_SIGN_ARGS}) > > + echo "$output" > > + if err=$(echo "$output" | grep -C9 -E "Sign value:\s*unavailable"); then > > + bberror "${UBOOT_MKIMAGE_SIGN} failed to provide signatures for these images:" > > + bbfatal_log "\n$err" > > Is the problem really in mkimage since it does not return errors when signing fails? I'd say yes, but it's explicitly implemented as best effort approach: https://github.com/u-boot/u-boot/commit/56518e71041f#diff-b0d9a26d538f8dc3aff2e1b518534e9e2026713b1f4204e2680d8a84244e3408R234 But how many years would it take to propagate new mkimage in practice?.. -- Alexander Sverdlin Siemens AG www.siemens.com
Hi, On Tue, Sep 03, 2024 at 07:24:31AM +0000, Sverdlin, Alexander wrote: > Hi Mikko! > > On Tue, 2024-09-03 at 09:03 +0300, Mikko Rapeli wrote: > > > mkimage doesn't fail if it is not able to sign FIT nodes. > > > This may lead to unbootable images in secure boot configurations. > > > Make signing failures fatal by parsing the mkimage output. > > > > > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > > --- > > > Changes in v3: > > > - bbfatag_log -> bberror + bbfatal_log with relevant mkimage output snippets > > > Changes in v2: > > > - bbfatal -> bbfatal_log > > > > > > � meta/classes-recipe/kernel-fitimage.bbclass | 9 +++++++-- > > > � 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass > > > index 67c98adb232..ccf848e643f 100644 > > > --- a/meta/classes-recipe/kernel-fitimage.bbclass > > > +++ b/meta/classes-recipe/kernel-fitimage.bbclass > > > @@ -753,11 +753,16 @@ fitimage_assemble() { > > > �� # Step 8: Sign the image > > > �� # > > > �� if [ "x${UBOOT_SIGN_ENABLE}" = "x1" ] ; then > > > - ${UBOOT_MKIMAGE_SIGN} \ > > > + output=$(${UBOOT_MKIMAGE_SIGN} \ > > > > Will this subshell return errors as before or is "set -e" propagated there? > > Good point, I need to test if I'm not masking real errors here... > > > > �� ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ > > > �� -F -k "${UBOOT_SIGN_KEYDIR}" \ > > > �� -r ${KERNEL_OUTPUT_DIR}/$2 \ > > > - ${UBOOT_MKIMAGE_SIGN_ARGS} > > > + ${UBOOT_MKIMAGE_SIGN_ARGS}) > > > + echo "$output" > > > + if err=$(echo "$output" | grep -C9 -E "Sign value:\s*unavailable"); then > > > + bberror "${UBOOT_MKIMAGE_SIGN} failed to provide signatures for these images:" > > > + bbfatal_log "\n$err" > > > > Is the problem really in mkimage since it does not return errors when signing fails? > > I'd say yes, but it's explicitly implemented as best effort approach: > https://github.com/u-boot/u-boot/commit/56518e71041f#diff-b0d9a26d538f8dc3aff2e1b518534e9e2026713b1f4204e2680d8a84244e3408R234 > > But how many years would it take to propagate new mkimage in practice?.. I think this should be challenged with u-boot upstream. If the tool is explicitly used to sign images then failure to do so should be captured and error value returned. Patching this in should be straight forward. Working around the issue by grep'ing logs is not good in the long run. What other failure modes may exist? Cheers, -Mikko
diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass index 67c98adb232..ccf848e643f 100644 --- a/meta/classes-recipe/kernel-fitimage.bbclass +++ b/meta/classes-recipe/kernel-fitimage.bbclass @@ -753,11 +753,16 @@ fitimage_assemble() { # Step 8: Sign the image # if [ "x${UBOOT_SIGN_ENABLE}" = "x1" ] ; then - ${UBOOT_MKIMAGE_SIGN} \ + output=$(${UBOOT_MKIMAGE_SIGN} \ ${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \ -F -k "${UBOOT_SIGN_KEYDIR}" \ -r ${KERNEL_OUTPUT_DIR}/$2 \ - ${UBOOT_MKIMAGE_SIGN_ARGS} + ${UBOOT_MKIMAGE_SIGN_ARGS}) + echo "$output" + if err=$(echo "$output" | grep -C9 -E "Sign value:\s*unavailable"); then + bberror "${UBOOT_MKIMAGE_SIGN} failed to provide signatures for these images:" + bbfatal_log "\n$err" + fi fi }