Re: [PATCH] fit: display proper node on error

[Simon Glass <sjg@chromium.org>]

Hi Angelo,

On Mon, 25 Oct 2021 at 15:11, Angelo Dureghello
<angelo.dureghello@timesys.com> wrote:
>
> Hi,
>
> On Sun, Oct 24, 2021 at 9:53 PM Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Alex,
>>
>> On Wed, 6 Oct 2021 at 10:00, Alex G. <mr.nuke.me@gmail.com> wrote:
>> >
>> > + Simon
>> >
>> > On 10/6/21 10:47 AM, Angelo Dureghello wrote:
>> > > Fix final error message from
>> > >
>> > > Verification failed for '<NULL>' hash node in 'conf@1' config node
>> > >
>> > > to
>> > >
>> > > Verification failed for 'signature@1' hash node in 'conf@1' config node
>> > >
>> > > Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
>> > > ---
>> > >   common/image-fit-sig.c | 2 +-
>> > >   1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
>> > > index b979cd2a4b..4f2a6ef214 100644
>> > > --- a/common/image-fit-sig.c
>> > > +++ b/common/image-fit-sig.c
>> > > @@ -166,8 +166,8 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
>> > >                       } else {
>> > >                               puts("+ ");
>> > >                               verified = 1;
>> > > -                             break;
>> > >                       }
>> > > +                     break;
>> >
>> > This would stop checking after the first signature- node. It seems
>> > counter-intuitive, as I would expect all signatures to be checked.
>> >
>>
>> > In my mind, the 'break;' clause should only happen when
>> > fit_image_check_sig() returns an error. I have no idea why it happened
>> > on success. Simon, any thoughts?
>>
>> If you have a 'required' signature you can use the signed-configs
>> approach. Checking the signature of individual images is not actually
>> all that useful.
>>
>> So I think the break is in the right place. It checks all signatures
>> and reports them, but only cares whether at least one was verified.
>>
>> For the error message to be correct, we need to save the noffset of
>> the failed node in a separate variable, I think, so we can report the
>> last error we got.
>>
>
> Oh, looks like i sent a wrong patch also, since the
> error was related on signature check in a config node:
>
> Verification failed for 'signature@1' hash node in 'conf@1' config node
>
> and i patched the image check, this since i couldn't retest on
> the board. But the check mechanism seems the same.
>
> Anyway, fit_image_check_sig() was properly returning an error, and
> issue was related to imx caam driver used for rsa calc. With sw calc
> i have signature verification on conf node passed:
>
> Verifying Hash Integrity ... sha1,rsa2048:dev+ OK
>
> So my understanding is that after an error we want to check for
> further "signature" subnodes inside the same "conf" or image node,
> but i have never seen more than one signature, is it something
> supported/allowed ?

Yes it should check all signatures until it runs out or finds a match.

Regards,
Simon