Hi,
> Da: Simon Glass <s...@chromium.org>
> Inviato: domenica 4 dicembre 2022 22:17
>
> ()Hi Sean,
>
> On Tue, 29 Nov 2022 at 04:45, Sean Anderson <sean.ander...@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.pego...@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.pego...@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.pego...@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
