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