diff mbox series

Revert "uboot-sign: fix U-Boot binary with public key"

Message ID 20241206210917.31123-1-reatmon@ti.com
State Accepted, archived
Commit 5e82d33451b5662df1e7fe2518a50644d18aa70d
Headers show
Series Revert "uboot-sign: fix U-Boot binary with public key" | expand

Commit Message

Ryan Eatmon Dec. 6, 2024, 9:09 p.m. UTC
This reverts commit 0d14e99aa18ee38293df63d585fafc270a4538be.

The patch removed logic required for correct handling of
UBOOT_SUFFIX=img or UBOOT_SUFFIX=rom.  We need to find a better way to
handle the fix for [YOCTO #15649].

Signed-off-by:  Ryan Eatmon <reatmon@ti.com>
---
 meta/classes-recipe/uboot-sign.bbclass | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Richard Purdie Dec. 8, 2024, 10:22 p.m. UTC | #1
On Fri, 2024-12-06 at 15:09 -0600, Ryan Eatmon via lists.openembedded.org wrote:
> This reverts commit 0d14e99aa18ee38293df63d585fafc270a4538be.
> 
> The patch removed logic required for correct handling of
> UBOOT_SUFFIX=img or UBOOT_SUFFIX=rom.  We need to find a better way to
> handle the fix for [YOCTO #15649].
> 
> Signed-off-by:  Ryan Eatmon <reatmon@ti.com>
> ---
>  meta/classes-recipe/uboot-sign.bbclass | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass
> index 7ee73b872a..a17be745ce 100644
> --- a/meta/classes-recipe/uboot-sign.bbclass
> +++ b/meta/classes-recipe/uboot-sign.bbclass
> @@ -122,7 +122,13 @@ concat_dtb() {
>  	# If we're not using a signed u-boot fit, concatenate SPL w/o DTB & U-Boot DTB
>  	# with public key (otherwise U-Boot will be packaged by uboot_fitimage_assemble)
>  	if [ "${SPL_SIGN_ENABLE}" != "1" ] ; then
> -		if [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
> +		if [ "x${UBOOT_SUFFIX}" = "ximg" -o "x${UBOOT_SUFFIX}" = "xrom" ] && \
> +			[ -e "${UBOOT_DTB_BINARY}" ]; then
> +			oe_runmake EXT_DTB="${UBOOT_DTB_SIGNED}" ${UBOOT_MAKE_TARGET}
> +			if [ -n "${binary}" ]; then
> +				cp ${binary} ${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}
> +			fi
> +		elif [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
>  			if [ -n "${binary}" ]; then
>  				cat ${UBOOT_NODTB_BINARY} ${UBOOT_DTB_SIGNED} | tee ${binary} > \
>  					${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}
> 

I'm in two minds about whether to take this or not. The code is clearly
needed for some platforms however the selftests don't cover it and I
doubt it is documented either :/

If I take this, can we add in some better testing please?

Cheers,

Richard
Ryan Eatmon Dec. 9, 2024, 8:40 p.m. UTC | #2
On 12/8/2024 4:22 PM, Richard Purdie wrote:
> On Fri, 2024-12-06 at 15:09 -0600, Ryan Eatmon via lists.openembedded.org wrote:
>> This reverts commit 0d14e99aa18ee38293df63d585fafc270a4538be.
>>
>> The patch removed logic required for correct handling of
>> UBOOT_SUFFIX=img or UBOOT_SUFFIX=rom.  We need to find a better way to
>> handle the fix for [YOCTO #15649].
>>
>> Signed-off-by:  Ryan Eatmon <reatmon@ti.com>
>> ---
>>   meta/classes-recipe/uboot-sign.bbclass | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass
>> index 7ee73b872a..a17be745ce 100644
>> --- a/meta/classes-recipe/uboot-sign.bbclass
>> +++ b/meta/classes-recipe/uboot-sign.bbclass
>> @@ -122,7 +122,13 @@ concat_dtb() {
>>   	# If we're not using a signed u-boot fit, concatenate SPL w/o DTB & U-Boot DTB
>>   	# with public key (otherwise U-Boot will be packaged by uboot_fitimage_assemble)
>>   	if [ "${SPL_SIGN_ENABLE}" != "1" ] ; then
>> -		if [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
>> +		if [ "x${UBOOT_SUFFIX}" = "ximg" -o "x${UBOOT_SUFFIX}" = "xrom" ] && \
>> +			[ -e "${UBOOT_DTB_BINARY}" ]; then
>> +			oe_runmake EXT_DTB="${UBOOT_DTB_SIGNED}" ${UBOOT_MAKE_TARGET}
>> +			if [ -n "${binary}" ]; then
>> +				cp ${binary} ${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}
>> +			fi
>> +		elif [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
>>   			if [ -n "${binary}" ]; then
>>   				cat ${UBOOT_NODTB_BINARY} ${UBOOT_DTB_SIGNED} | tee ${binary} > \
>>   					${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}
>>
> 
> I'm in two minds about whether to take this or not. The code is clearly
> needed for some platforms however the selftests don't cover it and I
> doubt it is documented either :/
> 
> If I take this, can we add in some better testing please?

The initial commit that starts the img ball rolling came from 2016:

https://git.openembedded.org/openembedded-core/commit/meta/classes/uboot-sign.bbclass?id=4afee787e455ce1d4c002cd5c003182f1fc50028

The logic has morphed over time, but there is where it started.  So it's 
been in there for a number of years.

I'm willing to take a stab at the selftest.  Is there any documentation 
to help with that broadly means/entails?  I'll also talk to Denys in our 
call tomorrow if he knows and can point me in a good direction for this.


> Cheers,
> 
> Richard
>
Richard Purdie Dec. 16, 2024, 5:26 p.m. UTC | #3
On Mon, 2024-12-09 at 14:40 -0600, Ryan Eatmon wrote:
> 
> 
> On 12/8/2024 4:22 PM, Richard Purdie wrote:
> > On Fri, 2024-12-06 at 15:09 -0600, Ryan Eatmon via lists.openembedded.org wrote:
> > > This reverts commit 0d14e99aa18ee38293df63d585fafc270a4538be.
> > > 
> > > The patch removed logic required for correct handling of
> > > UBOOT_SUFFIX=img or UBOOT_SUFFIX=rom.  We need to find a better way to
> > > handle the fix for [YOCTO #15649].
> > > 
> > > Signed-off-by:  Ryan Eatmon <reatmon@ti.com>
> > > ---
> > >   meta/classes-recipe/uboot-sign.bbclass | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass
> > > index 7ee73b872a..a17be745ce 100644
> > > --- a/meta/classes-recipe/uboot-sign.bbclass
> > > +++ b/meta/classes-recipe/uboot-sign.bbclass
> > > @@ -122,7 +122,13 @@ concat_dtb() {
> > >   	# If we're not using a signed u-boot fit, concatenate SPL w/o DTB & U-Boot DTB
> > >   	# with public key (otherwise U-Boot will be packaged by uboot_fitimage_assemble)
> > >   	if [ "${SPL_SIGN_ENABLE}" != "1" ] ; then
> > > -		if [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
> > > +		if [ "x${UBOOT_SUFFIX}" = "ximg" -o "x${UBOOT_SUFFIX}" = "xrom" ] && \
> > > +			[ -e "${UBOOT_DTB_BINARY}" ]; then
> > > +			oe_runmake EXT_DTB="${UBOOT_DTB_SIGNED}" ${UBOOT_MAKE_TARGET}
> > > +			if [ -n "${binary}" ]; then
> > > +				cp ${binary} ${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}
> > > +			fi
> > > +		elif [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
> > >   			if [ -n "${binary}" ]; then
> > >   				cat ${UBOOT_NODTB_BINARY} ${UBOOT_DTB_SIGNED} | tee ${binary} > \
> > >   					${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}
> > > 
> > 
> > I'm in two minds about whether to take this or not. The code is clearly
> > needed for some platforms however the selftests don't cover it and I
> > doubt it is documented either :/
> > 
> > If I take this, can we add in some better testing please?
> 
> The initial commit that starts the img ball rolling came from 2016:
> 
> https://git.openembedded.org/openembedded-core/commit/meta/classes/uboot-sign.bbclass?id=4afee787e455ce1d4c002cd5c003182f1fc50028
> 
> The logic has morphed over time, but there is where it started.  So it's 
> been in there for a number of years.
> 
> I'm willing to take a stab at the selftest.  Is there any documentation 
> to help with that broadly means/entails?  I'll also talk to Denys in our 
> call tomorrow if he knows and can point me in a good direction for this.

You can run a subset of selftest with "oe-selftest -r uboot" wich would
run the test cases in meta/lib/oeqa/selftest/cases/uboot.py. You can
narrow it to a specific class of tests within that file or a specific
test too, e.g.: "oe-selftest -r uboot.UBootTest.test_boot_uboot".

The aim is to have test cases for key workflows we need to ensure work.

There is some information in the manual:
https://docs.yoctoproject.org/test-manual/index.html
but it probably doesn't go into the level of detail you're looking for
about writing individual tests. That is something we've wanted to aim
to add but we're probably not there yet.

Cheers,

Richard
diff mbox series

Patch

diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass
index 7ee73b872a..a17be745ce 100644
--- a/meta/classes-recipe/uboot-sign.bbclass
+++ b/meta/classes-recipe/uboot-sign.bbclass
@@ -122,7 +122,13 @@  concat_dtb() {
 	# If we're not using a signed u-boot fit, concatenate SPL w/o DTB & U-Boot DTB
 	# with public key (otherwise U-Boot will be packaged by uboot_fitimage_assemble)
 	if [ "${SPL_SIGN_ENABLE}" != "1" ] ; then
-		if [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
+		if [ "x${UBOOT_SUFFIX}" = "ximg" -o "x${UBOOT_SUFFIX}" = "xrom" ] && \
+			[ -e "${UBOOT_DTB_BINARY}" ]; then
+			oe_runmake EXT_DTB="${UBOOT_DTB_SIGNED}" ${UBOOT_MAKE_TARGET}
+			if [ -n "${binary}" ]; then
+				cp ${binary} ${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}
+			fi
+		elif [ -e "${UBOOT_NODTB_BINARY}" -a -e "${UBOOT_DTB_BINARY}" ]; then
 			if [ -n "${binary}" ]; then
 				cat ${UBOOT_NODTB_BINARY} ${UBOOT_DTB_SIGNED} | tee ${binary} > \
 					${UBOOT_BINARYNAME}-${type}.${UBOOT_SUFFIX}