Re: [PATCH 1/3] binman: add sign option for binman

[Ivan Mikhaylov <fr0st61te@gmail.com>]

On Sat, 2023-02-04 at 15:23 -0700, Simon Glass wrote:
> Hi Ivan,
> 
> On Sun, 15 Jan 2023 at 16:54, Ivan Mikhaylov <fr0st61te@gmail.com>
> wrote:
> > 
> > On Fri, 2023-01-13 at 11:00 -0700, Simon Glass wrote:
> > > Hi Ivan,
> > > 
> > > On Sat, 24 Dec 2022 at 15:35, Ivan Mikhaylov
> > > <fr0st61te@gmail.com>
> > > wrote:
> > > > 
> > > > On Sat, 2022-12-17 at 15:02 -0700, Simon Glass wrote:
> > > > > Hi Ivan,
> > > > > 
> > > > > On Tue, 13 Dec 2022 at 11:51, Ivan Mikhaylov
> > > > > <fr0st61te@gmail.com>
> > > > > wrote:
> > > > > > 
> > > > > > On Fri, 2022-11-18 at 13:50 -0700, Simon Glass wrote:
> > > > > > > Hi Ivan,
> > > > > > > 
> > > > > > > On Thu, 15 Sept 2022 at 13:44, Ivan Mikhaylov
> > > > > > > <fr0st61te@gmail.com>
> > > > > > > wrote:
> > > > > > > > 
> > > > > > > > On Wed, 2022-09-07 at 15:10 -0600, Simon Glass wrote:
> > > > > > > > > Hi Ivan,
> > > > > > > > > 
> > > > > > > > > Section data comes from the BuildSectionData()
> > > > > > > > > method, so
> > > > > > > > > you
> > > > > > > > > could
> > > > > > > > > try calling that.
> > > > > > > > > 
> > > > > > > > > See also collect_contents_to_file()
> > > > > > > > > 
> > > > > > > > > Regards,
> > > > > > > > > Simon
> > > > > > > > 
> > > > > > > > Simon, I've tried both these ways and they both don't
> > > > > > > > work
> > > > > > > > to
> > > > > > > > me.
> > > > > > > > What
> > > > > > > > I've got:
> > > > > > > > 
> > > > > > > > def SignEntries(image_fname, input_fname,
> > > > > > > > privatekey_fname,
> > > > > > > > algo,
> > > > > > > > entry_paths):
> > > > > > > >     image_fname = os.path.abspath(image_fname)
> > > > > > > >     image = Image.FromFile(image_fname)
> > > > > > > >     state.PrepareFromLoadedData(image)
> > > > > > > >     image.LoadData()
> > > > > > > > 
> > > > > > > > 1. BuildSectionData
> > > > > > > > 
> > > > > > > >     for entry_path in entry_paths:
> > > > > > > >         entry = image.FindEntryPath(entry_path)
> > > > > > > > 
> > > > > > > >         try:
> > > > > > > >             entry.BuildSectionData(True)
> > > > > > > >         except Exception as e:
> > > > > > > >             logging.error(traceback.format_exc())
> > > > > > > > 
> > > > > > > > 
> > > > > > > > ERROR:root:AttributeError: 'NoneType' object has no
> > > > > > > > attribute
> > > > > > > > 'run'
> > > > > > 
> > > > > > Hi Simon, sorry for long delay.
> > > > > > 
> > > > > > binman: 'NoneType' object has no attribute 'run'
> > > > > > 
> > > > > > Traceback (most recent call last):
> > > > > >   File "/home/fr/upstream_uboot/tools/binman/binman", line
> > > > > > 133,
> > > > > > in
> > > > > > RunBinman
> > > > > >     ret_code = control.Binman(args)
> > > > > >   File "/home/fr/upstream_uboot/tools/binman/control.py",
> > > > > > line
> > > > > > 684,
> > > > > > in
> > > > > > Binman
> > > > > >     SignEntries(args.image, args.file, args.key, args.algo,
> > > > > > args.paths)
> > > > > >   File "/home/fr/upstream_uboot/tools/binman/control.py",
> > > > > > line
> > > > > > 469,
> > > > > > in
> > > > > > SignEntries
> > > > > >     entry.BuildSectionData(True)
> > > > > >   File "/home/fr/upstream_uboot/tools/binman/etype/fit.py",
> > > > > > line
> > > > > > 426,
> > > > > > in BuildSectionData
> > > > > >     if self.mkimage.run(reset_timestamp=True,
> > > > > > output_fname=output_fname,
> > > > > > AttributeError: 'NoneType' object has no attribute 'run'
> > > > > > 
> > > > > 
> > > > > You need to call image.CollectBintolls() like ReadEntry() and
> > > > > other
> > > > > functions similar to yours that read images from a file. This
> > > > > is
> > > > > the
> > > > > only way that the 'mkimage' tool becomes available to fit.py
> > > > > 
> > > > > See fit.AddBintools() which is called by that function and
> > > > > sets
> > > > > 'self.mkimage'
> > > > > > 
> > > > Simon, thanks, now this part works fine but there is still
> > > > issue
> > > > with
> > > > updating of fit section, saw that there exists some functions
> > > > like
> > > > WriteData but for section(etype/fit.py) it is not implemented
> > > > yet.
> > > > 
> > > > ValueError: Node '/fit': Replacing sections is not implemented
> > > > yet
> > > > 
> > > > Also tried SetContents but it doesn't update fit section in
> > > > place.
> > > > Any
> > > > suggestions here?
> > > 
> > > Updating a FIT in the image is not supported, or at least not
> > > tested,
> > > so presumably doesn't work.
> > > 
> > > I obtained fdt_add_pubkey
> > > from
> > > https://patchwork.ozlabs.org/project/uboot/list/?series=271511&state=
> > > *
> > > 
> > > I tried this:
> > > 
> > > binman test testSignSimple
> > > ======================== Running binman tests
> > > ========================
> > > E
> > > =================================================================
> > > ====
> > > =
> > > ERROR: binman.ftest.TestFunctional.testSignSimple
> > > (subunit.RemotedTestCase)
> > > binman.ftest.TestFunctional.testSignSimple
> > > -----------------------------------------------------------------
> > > ----
> > > -
> > > testtools.testresult.real._StringException: ValueError: Error 1
> > > running 'fdt_add_pubkey -a sha256,rsa4096 -k /tmp/binman.1antmyoq
> > > -n
> > > test_key /tmp/binman.1antmyoq/source.dtb': .dtb too small,
> > > increasing
> > > size by 1024 bytes
> > > .dtb too small, increasing size by 1024 bytes
> > > fdt_add_pubkey: Cannot add public key to FIT blob: Unknown error
> > > -56
> > > 
> > > 
> > > During handling of the above exception, another exception
> > > occurred:
> > > 
> > > UnboundLocalError: local variable 'key_dir' referenced before
> > > assignment
> > > 
> > > 
> > > -----------------------------------------------------------------
> > > ----
> > > -
> > > Ran 1 test in 1.658s
> > > 
> > > FAILED (errors=1)
> > > 
> > > [sjg@kea u ((5cf6f1f8e7c...) $)]$ binman test testSignSimpleExact
> > > ======================== Running binman tests
> > > ========================
> > > 
> > > -----------------------------------------------------------------
> > > ----
> > > -
> > > Ran 0 tests in 0.067s
> > > 
> > > OK
> > > 
> > > 
> > > Can you please:
> > > 
> > > - push your tree again
> > > - provide the command line you are using, or test case you are
> > > trying
> > > to make work
> > > - provide the files needed to run it it
> > > 
> > > With that I should be able to figure out what is needed.
> > > 
> > > Regards,
> > > Simon
> > 
> > Simon, sorry, I forgot about fdt_add_pubkey, I've updated and added
> > version on which I'm working into branch which I posted before.
> > There
> > was update in add_verify_data call for rsa at least which sending
> > node
> > number instead of return code because of this you seeing such
> > errors
> > with run of this toolkit. Now you should see something like this:
> > 
> > binman test testSignSimple
> > ======================== Running binman tests
> > ========================
> > E
> > ===================================================================
> > ===
> > ERROR: testSignSimple (binman.ftest.TestFunctional)
> > Test that a FIT container can be signed in image
> > -------------------------------------------------------------------
> > ---
> > ValueError: Node '/fit': Replacing sections is not implemented yet
> > 
> > -------------------------------------------------------------------
> > ---
> > Ran 1 test in 0.480s
> > 
> > FAILED (errors=1)
> > 
> > The command line which I'm using for manual testing:
> > 
> > binman -D sign -i image-updated.bin -k test_key.key -a
> > sha256,rsa4096
> > fit
> 
> I've had a crack at this and sent a patch to allow updating sections
> in toto.
> 
> https://github.com/sjg20/u-boot/tree/try-ivan
> 
> > 
> > Also, as I see fdt_add_pubkey application still not in the u-boot
> > tree.
> > Need I look through and put it in this series or create another
> > series
> > of patches for fdt_add_pubkey?
> 
> Doing it in this series is fine.
> 
> Regards,
> Simon

Simon, thanks a lot, now it's looks like working. I've updated my
branch on https://github.com/fr0st61te/u-boot/commits/signfit,
everything seems ok - fdt_add_pubkey and tests works fine. I want to
check everything with qemu or hw, it'll take some time. I'll get back
with proper patchsets in 2-3 weeks.

Thanks.