[PATCH 1/3] binman: add sign option for binman
Simon Glass
sjg at chromium.org
Wed Sep 7 23:10:55 CEST 2022
Hi Ivan,
On Tue, 6 Sept 2022 at 07:27, Ivan Mikhaylov <fr0st61te at 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.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?
Section data comes from the BuildSectionData() method, so you could
try calling that.
See also collect_contents_to_file()
Regards,
Simon
More information about the U-Boot
mailing list