R: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs

[Pegorer Massimo]

Hi,

> Da: Simon Glass <sjg at chromium.org>
> Inviato: domenica 4 dicembre 2022 22:17
> 
> ()Hi Sean,
> 
> On Tue, 29 Nov 2022 at 04:45, Sean Anderson <sean.anderson at seco.com>
> wrote:
> >
> > On 11/22/22 21:09, Simon Glass wrote:
> > > Hi Pegorer,
> > >
> > > On Sat, 19 Nov 2022 at 11:01, Pegorer Massimo
> <Massimo.Pegorer at vimar.com> wrote:
> > >>
> > >> Commit 87b0af9317cb4105f3f29cb0a4c28c7cd87ea65f added support for
> signing auto-generated (mkimage -f auto) FIT. Unfortunately, this signs 'images'
> subnodes but not 'configurations' ones. Following patch is a proposal to support
> also 'configurations' signing + 'images' hashing, as an alternative to 'images'
> signing, with 'auto' FIT. For this purpose, a new optional argument is added to
> mkimage '-r' option: any other better idea?
> > >>
> > >> =====
> > >>
> > >> From 8c8c8f421d541cc2eccb50490a57e86b81dc8df2 Mon Sep 17 00:00:00
> > >> 2001
> > >> From: Massimo Pegorer <massimo.pegorer at vimar.com>
> > >> Date: Sat, 19 Nov 2022 16:25:58 +0100
> > >> Subject: [PATCH] mkimage: fit: Support signed conf 'auto' FITs
> > >>
> > >> Extend support for signing in auto-generated FITs. Previously, it
> > >> was possible to sign 'images' subnodes in auto FIT, but not 'configurations'
> > >> subnodes. Consequently, usage with -K and -r options (i.e. write
> > >> keys as 'required' in a .dtb file) resulted in adding a signature
> > >> node with required = "image" property in the dtb.
> > >>
> > >> This patch allows usage of an optional argument with -r option to
> > >> select which subnodes, 'images' ones or 'configurations' ones, have
> > >> to be signed (in the second case 'images' subnodes are hashed): with '-r' or
> '-rimage'
> > >> the firsts are signed, while with '-rconf' the seconds; argument
> > >> values different than 'image' and 'conf' are invalid.
> > >>
> > >> Example to write a key with required = "conf" attribute into a dtb file:
> > >>
> > >> mkimage -f auto -rconf -d /dev/null -K <dtb-file> -o <algo> \
> > >>         -g <key-name-hint> -k <path-to-key-file> <dummy-itb-file>
> > >>
> > >> Signed-off-by: Massimo Pegorer <massimo.pegorer at vimar.com>
> > >> ---
> > >>  tools/fit_image.c | 25 +++++++++++++++++--------
> > >>  tools/mkimage.c   | 18 ++++++++++++++----

[...]

> > >> --
> > >> 2.34.1
> > >>
> > >
> > > I think this is a reasonable feature, but how about using '-f
> > > auto-conf' as the way to select this feature? The '-r' argument is
> > > intended to indicate that the keys are required to be verified.
> >
> > I think that extending -r with an argument is reasonable here. There's
> > no way to specify required = "image" either...
> 
> Note that -F can be used to sign a FIT later, after it is created.
> That option does not allow the creation of new configurations, though, so I
> don't think we need to worry about that angle.
> 
> We should try to support not using -r so that the signatures are added but not
> required. After all, the -r flag actually affects the verification data in U-Boot's
> FDT, not the FIT.
> 
> fit_image_setup_sig() is called with a string arg for require_keys, similar to what
> is used here, but I think that is a different thing.

I agree, '-r' makes sense only with '-K <dtb>'. Therefore, it is preferrable to
allow to specify in a different way what and how to sign in the auto-FIT:
consider someone would like to create signed auto-FIT without the need to
add the key to any FTD.

From a semantic point of view, not using '-r' would be clearer. Otherwise,
we would force an overload of '-r' current meaning, which is "when public
key data are added to the dtb file, include also the "required" property".

The point here is that we have two actions - 1. add hash and/or signatures
to images and configurations in a FIT; 2. add public key data, with or without
"required" property, in a FTD; - which are "independent" but require being
"coordinated" to have a coherent and meaningful final result.

> So overall I think that the image of enabling the feature in this patch is that:
> 
> - a 'signature' subnode is added to each configuration (or image) in
> add_hash_or_sig_node()
> - the crc32 in the image nodes changes to a sha
> - the key may or may not be required
> 
> These sound like things that should only be done during the initial FIT creation,
> with '-f auto', not in later signature addition with -F.
> 
> The current creation of signatures in the image nodes[1] seems a bit odd to me,
> but I suppose it makes sense if the goal is just to sign images. When signing
> configs we want to hash the images, not sign them, so perhaps signing of
> images with '-f auto' should be dropped? I don't mind either way, though.

I think we can keep current behaviour (image signing) when '-f auto' is used
with signing parameters, and the suggested '-f auto-conf' (with mandatory
signing options) to have an auto-FIT with signed configurations. Or swap the
default, if preferred (for the mix and match issue, and not complaining with
backward compatibility):
- '-f auto' + signing params for auto-FIT with signed configurations
- '-f auto-signimg' + signing params for auto-FIT with signed images

> So I do think that '-f auto-conf' is the right thing to do here.
> Alternatively down the road we could add one more flag which controls the
> operation of '-f auto', with respect to image nodes and config
> nodes:
> 
> -S <image>,<config>
> 
> so:
> 
> - (empty) - creates crc nodes in images, no signing of configurations
> - sha256 - creates sha256 nodes in images
> - sha1,rsa2048 - creates sha1 nodes in images, signs configurations with rsa2048
> 
> But I'm not sure how flexible we want this. Keeping it simple along the lines of
> this patch seems good to me.

I would not add more flexibility, as in case people can draw their required FIT
structure writing an ad-hoc ITS and invoking mkimage with '-f <file.its>'. By the
way we are discussing about auto FIT, which is just a single (kernel) image plus
one ore more dtbs plus a ramdisk.

There is one more interesting case: usage of mkimage just to add public key to
a dtb (with or without "required" property), without signing anything. E.g.:

mkimage -f auto -K <dtb> [-r] -d /dev/null -k ... -g ... -o ... etc...

Also for this case the '-f auto-conf' seems fine, without dependency on '-r'.

> Also this patch should have a test.
> 
> Regards,
> Simon
> 
> [1] 87b0af9317c ("mkimage: Support signing 'auto' FITs")

Finally I am going to propose a first patch that will support the following cases:

1. - creates crc nodes in images, no signing of configurations (original behaviour)
	syntax: '-f auto' without signing options
2. - sign images with <algo>, no signing of configurations [1]
	syntax: '-f auto -k ... -g ... -o <algo>'
3. - creates sha1 nodes in images, sign configurations with <algo>
	syntax: '-f auto-conf -k ... -g ... -o <algo>'

Regards,
Massimo