[PATCH 1/3] binman: add sign option for binman
Ivan Mikhaylov
fr0st61te at gmail.com
Tue Sep 6 18:27:21 CEST 2022
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.mikhaylov at 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?
Thanks.
>
> >
> > Anyways, waiting for Simon's review.
> >
> > Thanks.
More information about the U-Boot
mailing list