[PATCH v3] kernel-fitimage: make signing failure fatal

[PATCH v3] kernel-fitimage: make signing failure fatal

[A. Sverdlin]

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} \
 			${@'-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
 }
 
-- 
2.46.0

Re: [OE-core] [PATCH v3] kernel-fitimage: make signing failure fatal

[Mikko Rapeli]

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

Re: [OE-core] [PATCH v3] kernel-fitimage: make signing failure fatal

[Sverdlin, Alexander]

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

Re: [OE-core] [PATCH v3] kernel-fitimage: make signing failure fatal

[Mikko Rapeli]

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