Hi Ivan, On Tue, 6 Sept 2022 at 07:27, Ivan Mikhaylov <fr0st6...@gmail.com> wrote: > > On Thu, 2022-04-07 at 01:22 +0300, Alper Nebi Yasak wrote: > > On 06/04/2022 23:28, Ivan Mikhaylov wrote: > > > On Tue, 2022-04-05 at 21:54 +0300, Alper Nebi Yasak wrote: > > > > On 22/03/2022 00:43, Ivan Mikhaylov wrote: > > > > > Introduce proof of concept for binman's new option which > > > > > provides > > > > > sign > > > > > and replace sections in binary images. > > > > > > > > > > Usage as example: > > > > > > > > > > from: > > > > > mkimage -G privateky -r -o sha256,rsa4096 -F fit > > > > > binman replace -i flash.bin -f fit.fit fit > > > > > > > > > > to: > > > > > binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f > > > > > fit.fit > > > > > fit > > > > > > > > This shouldn't need the fit.fit input. It can be extracted from > > > > the > > > > image as you said in the cover letter. But I think instead of > > > > doing > > > > "extract -> sign with mkimage -> replace", signing should be > > > > implemented > > > > in the entry types as a new method. This way we can support other > > > > entry-specific ways to sign things (see vblock for an example). > > > > > > I have both of these things already: > > > 1.extract->sign->replace > > > binman sign -i flash.bin -k privatekey -a sha256,rsa4096 fit > > > 2.sign->replace > > > binman sign -i flash.bin -k privatekey -a sha256,rsa4096 -f > > > fit.fit > > > fit > > > > > > Just waited for review to see in which direction should I move. > > > > > > I've the case when I need only FIT image sign and replace after > > > that. I > > > think they both fit in 'sign' option. Okay about new method, so we > > > talking about just one call like 'SignEntries' without mkimage and > > > other steps with tools? > > > > See below... > > > > > > > > > > > Signed-off-by: Ivan Mikhaylov <ivan.mikhay...@siemens.com> > > > > > --- > > > > > tools/binman/cmdline.py | 13 +++++++++++++ > > > > > tools/binman/control.py | 26 +++++++++++++++++++++++++- > > > > > 2 files changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/tools/binman/cmdline.py b/tools/binman/cmdline.py > > > > > index 0626b850f4..1a25f95ff1 100644 > > > > > --- a/tools/binman/cmdline.py > > > > > +++ b/tools/binman/cmdline.py > > > > > @@ -160,6 +160,19 @@ controlled by a description in the board > > > > > device tree.''' > > > > > replace_parser.add_argument('paths', type=str, nargs='*', > > > > > help='Paths within file to > > > > > replace > > > > > (wildcard)') > > > > > > > > > > + sign_parser = subparsers.add_parser('sign', > > > > > + help='Sign entries > > > > > in > > > > > image') > > > > > + sign_parser.add_argument('-a', '--algo', type=str, > > > > > required=True, > > > > > + help='Hash algorithm e.g. > > > > > sha256,rsa4096') > > > > > + sign_parser.add_argument('-f', '--file', type=str, > > > > > required=True, > > > > > + help='Input filename to sign') > > > > > + sign_parser.add_argument('-i', '--image', type=str, > > > > > required=True, > > > > > + help='Image filename to > > > > > update') > > > > > + sign_parser.add_argument('-k', '--key', type=str, > > > > > required=True, > > > > > + help='Private key file for > > > > > signing') > > > > > + sign_parser.add_argument('paths', type=str, nargs='*', > > > > > + help='Paths within file to > > > > > sign > > > > > (wildcard)') > > > > > + > > > > > > > > If we want to support signing entry types other than FIT, I guess > > > > we > > > > need to base things on "EntryArg"s to make the arguments more > > > > dynamic, > > > > like named-by-arg blobs do. > > > > > > Should we care about such possibility in terms of these patches? > > > > There's already vblock and pre-load entry types where 'sign' is a > > valid > > thing to do. If we add a 'binman sign', it's reasonable to expect it > > to > > work on those as well. But these 'algo' and 'key' arguments don't > > directly apply to vblock, its signing interface is a bit weird and > > needs > > different arguments. > > > > Personally, I'm not sure if I'd use 'binman sign' as I like to > > clean-build things, so the argument mismatch is not that big of a > > concern for me. > > > > > > > > > > > test_parser = subparsers.add_parser('test', help='Run > > > > > tests') > > > > > test_parser.add_argument('-P', '--processes', type=int, > > > > > help='set number of processes to use for running > > > > > tests') > > > > > diff --git a/tools/binman/control.py b/tools/binman/control.py > > > > > index a179f78129..7595ea7776 100644 > > > > > --- a/tools/binman/control.py > > > > > +++ b/tools/binman/control.py > > > > > @@ -19,6 +19,7 @@ from binman import cbfs_util > > > > > from binman import elf > > > > > from patman import command > > > > > from patman import tout > > > > > +from patman import tools > > > > > > > > > > # List of images we plan to create > > > > > # Make this global so that it can be referenced from tests > > > > > @@ -434,6 +435,26 @@ def ReplaceEntries(image_fname, > > > > > input_fname, > > > > > indir, entry_paths, > > > > > AfterReplace(image, allow_resize=allow_resize, > > > > > write_map=write_map) > > > > > return image > > > > > > > > > > +def MkimageSign(privatekey_fname, algo, input_fname): > > > > > + tools.Run('mkimage', '-G', privatekey_fname, '-r', '-o', > > > > > algo, > > > > > '-F', input_fname) > > > > > + > > > > > +def SignEntries(image_fname, input_fname, privatekey_fname, > > > > > algo, > > > > > entry_paths): > > > > > + """Sign and replace the data from one or more entries from > > > > > input files > > > > > + > > > > > + Args: > > > > > + image_fname: Image filename to process > > > > > + input_fname: Single input filename to use if replacing > > > > > one > > > > > file, None > > > > > + otherwise > > > > > + algo: Hashing algorithm > > > > > + privatekey_fname: Private key filename > > > > > + > > > > > + Returns: > > > > > + List of EntryInfo records that were signed and > > > > > replaced > > > > > + """ > > > > > + > > > > > + MkimageSign(privatekey_fname, algo, input_fname) > > > > > + > > > > > + return ReplaceEntries(image_fname, input_fname, None, > > > > > entry_paths) > > > > > > > > I wrote a bit above, but what I mean is the mkimage call would go > > > > into a > > > > new method in etype/fit.py, and SignEntries() would be something > > > > like > > > > ReplaceEntries() but end up calling that method instead of > > > > WriteData(). > > > > > > Yeap, I agree, as I said above about 'how it should be'. Do you > > > think > > > it should be like additional implementation for mkimage in > > > etype/fit.py > > > which should be used in SignEntries inside? Something like: > > > def SignEntries(params): > > > fit = SignFIT(params) > > > return ReplaceEntries(params with fit) > > > > Don't call ReplaceEntries(). Try to copy what it does, but don't call > > entry.WriteData() either. Replacing FIT sections is broken on master > > for > > now, it tries to rebuild itself to the original specification. I'm > > planning to fix it, but the way I'm thinking of is demoting the entry > > to > > 'blob-ext' when replaced. It wouldn't be nice if that happened also > > when > > we're signing an entry. > > > > I think SignEntries would be more like (untested): > > > > def SignEntries(image_fname, entry_paths, ...): > > image_fname = os.path.abspath(image_fname) > > image = Image.FromFile(image_fname) > > state.PrepareFromLoadedData(image) > > image.LoadData() > > > > for entry_path in entry_paths: > > entry = image.FindEntryPath(entry_path) > > entry.UpdateSignatures(...) # TODO > > > > ProcessImage(image, update_fdt=True, write_map=False, > > get_contents=False, allow_resize=False) > > > > Then you'd implement UpdateSignatures in fit.py to configure the > > entry > > to use the given params. I'm not sure how exactly, I thought you'd > > call > > mkimage there, but it might be necessary to edit the underlying > > binman > > control dtb to change the signature nodes. > > Alper, this way works but until 74d3b231(binman: Create FIT subentries > in the FIT section, not its parent) commit. self.data or self.GetData() > is None for entry which need to be signed after this commit. Any ideas > how to get the data of fit entry?
Section data comes from the BuildSectionData() method, so you could try calling that. See also collect_contents_to_file() Regards, Simon