[v2,5/5] test_vboot.py: include test of fdt_add_pubkey tool

From: Roman Kopytin <Roman.Kopytin@kaspersky.com>

Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 test/py/tests/test_vboot.py | 8 ++++++++
 1 file changed, 8 insertions(+)

Hi Ivan,

On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
>
> Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  test/py/tests/test_vboot.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
> index e3e7ca4b21..956b8fcd43 100644
> --- a/test/py/tests/test_vboot.py
> +++ b/test/py/tests/test_vboot.py
> @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name, sha_algo, padding, sign_options, required,
>
>          util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
>
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')
> +        # Then add the dev key via the fdt_add_pubkey tool
> +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,rsa2048' % sha_algo,
> +                                '-k', tmpdir, '-n', 'dev', '-r', 'conf', dtb])
> +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
> +
>          if full_test:
>              # Make sure that U-Boot checks that the config is in the list of
>              # hashed nodes. If it isn't, a security bypass is possible.
> @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name, sha_algo, padding, sign_options, required,
>      mkimage = cons.config.build_dir + '/tools/mkimage'
>      binman = cons.config.source_dir + '/tools/binman/binman'
>      fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
> +    fdt_add_pubkey = cons.config.build_dir + '/tools/fdt_add_pubkey'
>      dtc_args = '-I dts -O dtb -i %s' % tmpdir
>      dtb = '%ssandbox-u-boot.dtb' % tmpdir
>      sig_node = '/configurations/conf-1/signature'
> --
> 2.39.1
>

Unfortunately this test fails on sandbox:

https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975

I think it would be better to put it in its own test (perhaps in the
same file) so we are not doing it on every test run. Also you could
check (in a very basic way) that it adds the key correctly since we
don't really need another test of the logic of doing that. We are just
checking that your tool calls that logic correctly.

I'll drop this one when applying, for now. Please take a look.

Regards,
Simon

On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> Hi Ivan,
> 
> On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> wrote:
> > 
> > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > 
> > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> >  test/py/tests/test_vboot.py | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/test/py/tests/test_vboot.py
> > b/test/py/tests/test_vboot.py
> > index e3e7ca4b21..956b8fcd43 100644
> > --- a/test/py/tests/test_vboot.py
> > +++ b/test/py/tests/test_vboot.py
> > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> > 
> >          util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > dtb])
> > 
> > +        # Create a fresh .dtb without the public keys
> > +        dtc('sandbox-u-boot.dts')
> > +        # Then add the dev key via the fdt_add_pubkey tool
> > +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,rsa2048'
> > % sha_algo,
> > +                                '-k', tmpdir, '-n', 'dev', '-r',
> > 'conf', dtb])
> > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > dtb])
> > +
> >          if full_test:
> >              # Make sure that U-Boot checks that the config is in
> > the list of
> >              # hashed nodes. If it isn't, a security bypass is
> > possible.
> > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      mkimage = cons.config.build_dir + '/tools/mkimage'
> >      binman = cons.config.source_dir + '/tools/binman/binman'
> >      fit_check_sign = cons.config.build_dir +
> > '/tools/fit_check_sign'
> > +    fdt_add_pubkey = cons.config.build_dir +
> > '/tools/fdt_add_pubkey'
> >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> >      sig_node = '/configurations/conf-1/signature'
> > --
> > 2.39.1
> > 
> 
> Unfortunately this test fails on sandbox:
> 
> https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> 
> I think it would be better to put it in its own test (perhaps in the
> same file) so we are not doing it on every test run. Also you could
> check (in a very basic way) that it adds the key correctly since we
> don't really need another test of the logic of doing that. We are
> just
> checking that your tool calls that logic correctly.
> 
> I'll drop this one when applying, for now. Please take a look.
> 
> Regards,
> Simon

Simon, does it look ok? Test for test_vboot is passed fine.

Thanks.

From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00 2001
From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Date: Thu, 11 Nov 2021 11:15:12 +0300
Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey tool

Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index e3e7ca4b21..5ae622fe21 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
padding, sign_options, required,
         # Check that the boot fails if the global signature is not
provided
         run_bootm(sha_algo, 'global image signature', 'signature is
mandatory', False)
 
+    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
+        """Test fdt_add_pubkey utility with given hash algorithm and
padding.
+
+        This function tests if fdt_add_pubkey utility may add public
keys into dtb.
+
+        Args:
+            sha_algo: Either 'sha1' or 'sha256', to select the
algorithm to use
+            padding: Either '' or '-pss', to select the padding to use
for the
+                    rsa signature algorithm.
+            sign_options: Options to mkimage when signing a fit image.
+        """
+
+        # Create a fresh .dtb without the public keys
+        dtc('sandbox-u-boot.dts')
+        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
+
+        # Sign images with our dev keys
+        sign_fit(sha_algo, sign_options)
+
+        # Create a fresh .dtb without the public keys
+        dtc('sandbox-u-boot.dts')
+
+        cons.log.action('%s: Test fdt_add_pubkey with signed
configuration' % sha_algo)
+        # Then add the dev key via the fdt_add_pubkey tool
+        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
('sha256' if algo_arg else sha_algo, \
+                                'rsa3072' if sha_algo == 'sha384' else
'rsa2048'),
+                                '-k', tmpdir, '-n', 'dev', '-r',
'conf', dtb])
+
+        # Check with fit_check_sign that FIT is signed with key
+        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
+
     cons = u_boot_console
     tmpdir = os.path.join(cons.config.result_dir, name) + '/'
     if not os.path.exists(tmpdir):
@@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
padding, sign_options, required,
     mkimage = cons.config.build_dir + '/tools/mkimage'
     binman = cons.config.source_dir + '/tools/binman/binman'
     fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
+    fdt_add_pubkey = cons.config.build_dir + '/tools/fdt_add_pubkey'
     dtc_args = '-I dts -O dtb -i %s' % tmpdir
     dtb = '%ssandbox-u-boot.dtb' % tmpdir
     sig_node = '/configurations/conf-1/signature'
@@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
padding, sign_options, required,
     with open(evil_kernel, 'wb') as fd:
         fd.write(500 * b'\x01')
 
+    test_fdt_add_pubkey(sha_algo, padding, sign_options)
     try:
         # We need to use our own device tree file. Remember to restore
it
         # afterwards.

Hi Ivan,

On Wed, 15 Mar 2023 at 19:17, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> > Hi Ivan,
> >
> > On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> > wrote:
> > >
> > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > >
> > > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > ---
> > >  test/py/tests/test_vboot.py | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/test/py/tests/test_vboot.py
> > > b/test/py/tests/test_vboot.py
> > > index e3e7ca4b21..956b8fcd43 100644
> > > --- a/test/py/tests/test_vboot.py
> > > +++ b/test/py/tests/test_vboot.py
> > > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >
> > >          util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > > dtb])
> > >
> > > +        # Create a fresh .dtb without the public keys
> > > +        dtc('sandbox-u-boot.dts')
> > > +        # Then add the dev key via the fdt_add_pubkey tool
> > > +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,rsa2048'
> > > % sha_algo,
> > > +                                '-k', tmpdir, '-n', 'dev', '-r',
> > > 'conf', dtb])
> > > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > > dtb])
> > > +
> > >          if full_test:
> > >              # Make sure that U-Boot checks that the config is in
> > > the list of
> > >              # hashed nodes. If it isn't, a security bypass is
> > > possible.
> > > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > >      fit_check_sign = cons.config.build_dir +
> > > '/tools/fit_check_sign'
> > > +    fdt_add_pubkey = cons.config.build_dir +
> > > '/tools/fdt_add_pubkey'
> > >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> > >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> > >      sig_node = '/configurations/conf-1/signature'
> > > --
> > > 2.39.1
> > >
> >
> > Unfortunately this test fails on sandbox:
> >
> > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> >
> > I think it would be better to put it in its own test (perhaps in the
> > same file) so we are not doing it on every test run. Also you could
> > check (in a very basic way) that it adds the key correctly since we
> > don't really need another test of the logic of doing that. We are
> > just
> > checking that your tool calls that logic correctly.
> >
> > I'll drop this one when applying, for now. Please take a look.
> >
> > Regards,
> > Simon
>
> Simon, does it look ok? Test for test_vboot is passed fine.

Please see my message before:

> Unfortunately this test fails on sandbox:
>
> https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
>
> I think it would be better to put it in its own test (perhaps in the
> same file) so we are not doing it on every test run. Also you could
> check (in a very basic way) that it adds the key correctly since we
> don't really need another test of the logic of doing that. We are just
> checking that your tool calls that logic correctly.
>
> I'll drop this one when applying, for now. Please take a look.

I have not applied this patch due to the problem.

Regards,
Simon



>
> Thanks.
>
> From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00 2001
> From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> Date: Thu, 11 Nov 2021 11:15:12 +0300
> Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey tool
>
> Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
> index e3e7ca4b21..5ae622fe21 100644
> --- a/test/py/tests/test_vboot.py
> +++ b/test/py/tests/test_vboot.py
> @@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
> padding, sign_options, required,
>          # Check that the boot fails if the global signature is not
> provided
>          run_bootm(sha_algo, 'global image signature', 'signature is
> mandatory', False)
>
> +    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
> +        """Test fdt_add_pubkey utility with given hash algorithm and
> padding.
> +
> +        This function tests if fdt_add_pubkey utility may add public
> keys into dtb.
> +
> +        Args:
> +            sha_algo: Either 'sha1' or 'sha256', to select the
> algorithm to use
> +            padding: Either '' or '-pss', to select the padding to use
> for the
> +                    rsa signature algorithm.
> +            sign_options: Options to mkimage when signing a fit image.
> +        """
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')
> +        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
> +
> +        # Sign images with our dev keys
> +        sign_fit(sha_algo, sign_options)
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')
> +
> +        cons.log.action('%s: Test fdt_add_pubkey with signed
> configuration' % sha_algo)
> +        # Then add the dev key via the fdt_add_pubkey tool
> +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
> ('sha256' if algo_arg else sha_algo, \
> +                                'rsa3072' if sha_algo == 'sha384' else
> 'rsa2048'),
> +                                '-k', tmpdir, '-n', 'dev', '-r',
> 'conf', dtb])
> +
> +        # Check with fit_check_sign that FIT is signed with key
> +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
> +
>      cons = u_boot_console
>      tmpdir = os.path.join(cons.config.result_dir, name) + '/'
>      if not os.path.exists(tmpdir):
> @@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> padding, sign_options, required,
>      mkimage = cons.config.build_dir + '/tools/mkimage'
>      binman = cons.config.source_dir + '/tools/binman/binman'
>      fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
> +    fdt_add_pubkey = cons.config.build_dir + '/tools/fdt_add_pubkey'
>      dtc_args = '-I dts -O dtb -i %s' % tmpdir
>      dtb = '%ssandbox-u-boot.dtb' % tmpdir
>      sig_node = '/configurations/conf-1/signature'
> @@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> padding, sign_options, required,
>      with open(evil_kernel, 'wb') as fd:
>          fd.write(500 * b'\x01')
>
> +    test_fdt_add_pubkey(sha_algo, padding, sign_options)
>      try:
>          # We need to use our own device tree file. Remember to restore
> it
>          # afterwards.
> --
> 2.39.1
>
>

On Thu, 2023-03-16 at 07:59 -0600, Simon Glass wrote:
> Hi Ivan,
> 
> On Wed, 15 Mar 2023 at 19:17, Ivan Mikhaylov <fr0st61te@gmail.com>
> wrote:
> > 
> > On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> > > Hi Ivan,
> > > 
> > > On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> > > wrote:
> > > > 
> > > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > > 
> > > > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > > ---
> > > >  test/py/tests/test_vboot.py | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/test/py/tests/test_vboot.py
> > > > b/test/py/tests/test_vboot.py
> > > > index e3e7ca4b21..956b8fcd43 100644
> > > > --- a/test/py/tests/test_vboot.py
> > > > +++ b/test/py/tests/test_vboot.py
> > > > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name,
> > > > sha_algo,
> > > > padding, sign_options, required,
> > > > 
> > > >          util.run_and_log(cons, [fit_check_sign, '-f', fit, '-
> > > > k',
> > > > dtb])
> > > > 
> > > > +        # Create a fresh .dtb without the public keys
> > > > +        dtc('sandbox-u-boot.dts')
> > > > +        # Then add the dev key via the fdt_add_pubkey tool
> > > > +        util.run_and_log(cons, [fdt_add_pubkey, '-a',
> > > > '%s,rsa2048'
> > > > % sha_algo,
> > > > +                                '-k', tmpdir, '-n', 'dev', '-
> > > > r',
> > > > 'conf', dtb])
> > > > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-
> > > > k',
> > > > dtb])
> > > > +
> > > >          if full_test:
> > > >              # Make sure that U-Boot checks that the config is
> > > > in
> > > > the list of
> > > >              # hashed nodes. If it isn't, a security bypass is
> > > > possible.
> > > > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name,
> > > > sha_algo,
> > > > padding, sign_options, required,
> > > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > > >      fit_check_sign = cons.config.build_dir +
> > > > '/tools/fit_check_sign'
> > > > +    fdt_add_pubkey = cons.config.build_dir +
> > > > '/tools/fdt_add_pubkey'
> > > >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> > > >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> > > >      sig_node = '/configurations/conf-1/signature'
> > > > --
> > > > 2.39.1
> > > > 
> > > 
> > > Unfortunately this test fails on sandbox:
> > > 
> > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > > 
> > > I think it would be better to put it in its own test (perhaps in
> > > the
> > > same file) so we are not doing it on every test run. Also you
> > > could
> > > check (in a very basic way) that it adds the key correctly since
> > > we
> > > don't really need another test of the logic of doing that. We are
> > > just
> > > checking that your tool calls that logic correctly.
> > > 
> > > I'll drop this one when applying, for now. Please take a look.
> > > 
> > > Regards,
> > > Simon
> > 
> > Simon, does it look ok? Test for test_vboot is passed fine.
> 
> Please see my message before:
> 
> > Unfortunately this test fails on sandbox:
> > 
> > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > 
> > I think it would be better to put it in its own test (perhaps in
> > the
> > same file) so we are not doing it on every test run. Also you could
> > check (in a very basic way) that it adds the key correctly since we
> > don't really need another test of the logic of doing that. We are
> > just
> > checking that your tool calls that logic correctly.
> > 
> > I'll drop this one when applying, for now. Please take a look.
> 
> I have not applied this patch due to the problem.
> 
> Regards,
> Simon
> 

Simon, but I've changed the test and put it in previous note, maybe you
didn't notice, I did what you asked:
- made own test "test_fdt_add_pubkey"
- simple check that with clear dtb you can add keys with fdt_add_pubkey
  and check with fit_check_sign with signed fit.

I've checked that change with sandbox and it passes test_vboot well.

Need I re-submit that patch or whole series?

Thanks.

> 
> 
> > 
> > Thanks.
> > 
> > From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00
> > 2001
> > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > Date: Thu, 11 Nov 2021 11:15:12 +0300
> > Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey
> > tool
> > 
> > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > ---
> >  test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/test/py/tests/test_vboot.py
> > b/test/py/tests/test_vboot.py
> > index e3e7ca4b21..5ae622fe21 100644
> > --- a/test/py/tests/test_vboot.py
> > +++ b/test/py/tests/test_vboot.py
> > @@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >          # Check that the boot fails if the global signature is not
> > provided
> >          run_bootm(sha_algo, 'global image signature', 'signature
> > is
> > mandatory', False)
> > 
> > +    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
> > +        """Test fdt_add_pubkey utility with given hash algorithm
> > and
> > padding.
> > +
> > +        This function tests if fdt_add_pubkey utility may add
> > public
> > keys into dtb.
> > +
> > +        Args:
> > +            sha_algo: Either 'sha1' or 'sha256', to select the
> > algorithm to use
> > +            padding: Either '' or '-pss', to select the padding to
> > use
> > for the
> > +                    rsa signature algorithm.
> > +            sign_options: Options to mkimage when signing a fit
> > image.
> > +        """
> > +
> > +        # Create a fresh .dtb without the public keys
> > +        dtc('sandbox-u-boot.dts')
> > +        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
> > +
> > +        # Sign images with our dev keys
> > +        sign_fit(sha_algo, sign_options)
> > +
> > +        # Create a fresh .dtb without the public keys
> > +        dtc('sandbox-u-boot.dts')
> > +
> > +        cons.log.action('%s: Test fdt_add_pubkey with signed
> > configuration' % sha_algo)
> > +        # Then add the dev key via the fdt_add_pubkey tool
> > +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
> > ('sha256' if algo_arg else sha_algo, \
> > +                                'rsa3072' if sha_algo == 'sha384'
> > else
> > 'rsa2048'),
> > +                                '-k', tmpdir, '-n', 'dev', '-r',
> > 'conf', dtb])
> > +
> > +        # Check with fit_check_sign that FIT is signed with key
> > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > dtb])
> > +
> >      cons = u_boot_console
> >      tmpdir = os.path.join(cons.config.result_dir, name) + '/'
> >      if not os.path.exists(tmpdir):
> > @@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      mkimage = cons.config.build_dir + '/tools/mkimage'
> >      binman = cons.config.source_dir + '/tools/binman/binman'
> >      fit_check_sign = cons.config.build_dir +
> > '/tools/fit_check_sign'
> > +    fdt_add_pubkey = cons.config.build_dir +
> > '/tools/fdt_add_pubkey'
> >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> >      sig_node = '/configurations/conf-1/signature'
> > @@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > padding, sign_options, required,
> >      with open(evil_kernel, 'wb') as fd:
> >          fd.write(500 * b'\x01')
> > 
> > +    test_fdt_add_pubkey(sha_algo, padding, sign_options)
> >      try:
> >          # We need to use our own device tree file. Remember to
> > restore
> > it
> >          # afterwards.
> > --
> > 2.39.1
> > 
> >

Hi Ivan,

On Thu, 16 Mar 2023 at 08:45, Ivan Mikhaylov <fr0st61te@gmail.com> wrote:
>
> On Thu, 2023-03-16 at 07:59 -0600, Simon Glass wrote:
> > Hi Ivan,
> >
> > On Wed, 15 Mar 2023 at 19:17, Ivan Mikhaylov <fr0st61te@gmail.com>
> > wrote:
> > >
> > > On Fri, 2023-03-10 at 17:46 -0800, Simon Glass wrote:
> > > > Hi Ivan,
> > > >
> > > > On Tue, 7 Mar 2023 at 14:13, Ivan Mikhaylov <fr0st61te@gmail.com>
> > > > wrote:
> > > > >
> > > > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > > >
> > > > > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > > > ---
> > > > >  test/py/tests/test_vboot.py | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/test/py/tests/test_vboot.py
> > > > > b/test/py/tests/test_vboot.py
> > > > > index e3e7ca4b21..956b8fcd43 100644
> > > > > --- a/test/py/tests/test_vboot.py
> > > > > +++ b/test/py/tests/test_vboot.py
> > > > > @@ -313,6 +313,13 @@ def test_vboot(u_boot_console, name,
> > > > > sha_algo,
> > > > > padding, sign_options, required,
> > > > >
> > > > >          util.run_and_log(cons, [fit_check_sign, '-f', fit, '-
> > > > > k',
> > > > > dtb])
> > > > >
> > > > > +        # Create a fresh .dtb without the public keys
> > > > > +        dtc('sandbox-u-boot.dts')
> > > > > +        # Then add the dev key via the fdt_add_pubkey tool
> > > > > +        util.run_and_log(cons, [fdt_add_pubkey, '-a',
> > > > > '%s,rsa2048'
> > > > > % sha_algo,
> > > > > +                                '-k', tmpdir, '-n', 'dev', '-
> > > > > r',
> > > > > 'conf', dtb])
> > > > > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-
> > > > > k',
> > > > > dtb])
> > > > > +
> > > > >          if full_test:
> > > > >              # Make sure that U-Boot checks that the config is
> > > > > in
> > > > > the list of
> > > > >              # hashed nodes. If it isn't, a security bypass is
> > > > > possible.
> > > > > @@ -500,6 +507,7 @@ def test_vboot(u_boot_console, name,
> > > > > sha_algo,
> > > > > padding, sign_options, required,
> > > > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > > > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > > > >      fit_check_sign = cons.config.build_dir +
> > > > > '/tools/fit_check_sign'
> > > > > +    fdt_add_pubkey = cons.config.build_dir +
> > > > > '/tools/fdt_add_pubkey'
> > > > >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> > > > >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> > > > >      sig_node = '/configurations/conf-1/signature'
> > > > > --
> > > > > 2.39.1
> > > > >
> > > >
> > > > Unfortunately this test fails on sandbox:
> > > >
> > > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > > >
> > > > I think it would be better to put it in its own test (perhaps in
> > > > the
> > > > same file) so we are not doing it on every test run. Also you
> > > > could
> > > > check (in a very basic way) that it adds the key correctly since
> > > > we
> > > > don't really need another test of the logic of doing that. We are
> > > > just
> > > > checking that your tool calls that logic correctly.
> > > >
> > > > I'll drop this one when applying, for now. Please take a look.
> > > >
> > > > Regards,
> > > > Simon
> > >
> > > Simon, does it look ok? Test for test_vboot is passed fine.
> >
> > Please see my message before:
> >
> > > Unfortunately this test fails on sandbox:
> > >
> > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/591975
> > >
> > > I think it would be better to put it in its own test (perhaps in
> > > the
> > > same file) so we are not doing it on every test run. Also you could
> > > check (in a very basic way) that it adds the key correctly since we
> > > don't really need another test of the logic of doing that. We are
> > > just
> > > checking that your tool calls that logic correctly.
> > >
> > > I'll drop this one when applying, for now. Please take a look.
> >
> > I have not applied this patch due to the problem.
> >
> > Regards,
> > Simon
> >
>
> Simon, but I've changed the test and put it in previous note, maybe you
> didn't notice, I did what you asked:
> - made own test "test_fdt_add_pubkey"
> - simple check that with clear dtb you can add keys with fdt_add_pubkey
>   and check with fit_check_sign with signed fit.
>
> I've checked that change with sandbox and it passes test_vboot well.
>
> Need I re-submit that patch or whole series?

OK, great. Yes please send that patch by itself (rebased to
u-boot-dm/next) so it is picked up by patchwork. You'll find the rest
of your patches in dm/next but they have not made it upstream yet.

Regards,
Simon


>
> Thanks.
>
> >
> >
> > >
> > > Thanks.
> > >
> > > From 5484d525d4950b064adf1204f5bf055229c942ac Mon Sep 17 00:00:00
> > > 2001
> > > From: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > Date: Thu, 11 Nov 2021 11:15:12 +0300
> > > Subject: [PATCH v3] test_vboot.py: include test of fdt_add_pubkey
> > > tool
> > >
> > > Signed-off-by: Roman Kopytin <Roman.Kopytin@kaspersky.com>
> > > Signed-off-by: Ivan Mikhaylov <fr0st61te@gmail.com>
> > > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > > ---
> > >  test/py/tests/test_vboot.py | 33 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > >
> > > diff --git a/test/py/tests/test_vboot.py
> > > b/test/py/tests/test_vboot.py
> > > index e3e7ca4b21..5ae622fe21 100644
> > > --- a/test/py/tests/test_vboot.py
> > > +++ b/test/py/tests/test_vboot.py
> > > @@ -491,6 +491,37 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >          # Check that the boot fails if the global signature is not
> > > provided
> > >          run_bootm(sha_algo, 'global image signature', 'signature
> > > is
> > > mandatory', False)
> > >
> > > +    def test_fdt_add_pubkey(sha_algo, padding, sign_options):
> > > +        """Test fdt_add_pubkey utility with given hash algorithm
> > > and
> > > padding.
> > > +
> > > +        This function tests if fdt_add_pubkey utility may add
> > > public
> > > keys into dtb.
> > > +
> > > +        Args:
> > > +            sha_algo: Either 'sha1' or 'sha256', to select the
> > > algorithm to use
> > > +            padding: Either '' or '-pss', to select the padding to
> > > use
> > > for the
> > > +                    rsa signature algorithm.
> > > +            sign_options: Options to mkimage when signing a fit
> > > image.
> > > +        """
> > > +
> > > +        # Create a fresh .dtb without the public keys
> > > +        dtc('sandbox-u-boot.dts')
> > > +        make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
> > > +
> > > +        # Sign images with our dev keys
> > > +        sign_fit(sha_algo, sign_options)
> > > +
> > > +        # Create a fresh .dtb without the public keys
> > > +        dtc('sandbox-u-boot.dts')
> > > +
> > > +        cons.log.action('%s: Test fdt_add_pubkey with signed
> > > configuration' % sha_algo)
> > > +        # Then add the dev key via the fdt_add_pubkey tool
> > > +        util.run_and_log(cons, [fdt_add_pubkey, '-a', '%s,%s' %
> > > ('sha256' if algo_arg else sha_algo, \
> > > +                                'rsa3072' if sha_algo == 'sha384'
> > > else
> > > 'rsa2048'),
> > > +                                '-k', tmpdir, '-n', 'dev', '-r',
> > > 'conf', dtb])
> > > +
> > > +        # Check with fit_check_sign that FIT is signed with key
> > > +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k',
> > > dtb])
> > > +
> > >      cons = u_boot_console
> > >      tmpdir = os.path.join(cons.config.result_dir, name) + '/'
> > >      if not os.path.exists(tmpdir):
> > > @@ -500,6 +531,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >      mkimage = cons.config.build_dir + '/tools/mkimage'
> > >      binman = cons.config.source_dir + '/tools/binman/binman'
> > >      fit_check_sign = cons.config.build_dir +
> > > '/tools/fit_check_sign'
> > > +    fdt_add_pubkey = cons.config.build_dir +
> > > '/tools/fdt_add_pubkey'
> > >      dtc_args = '-I dts -O dtb -i %s' % tmpdir
> > >      dtb = '%ssandbox-u-boot.dtb' % tmpdir
> > >      sig_node = '/configurations/conf-1/signature'
> > > @@ -516,6 +548,7 @@ def test_vboot(u_boot_console, name, sha_algo,
> > > padding, sign_options, required,
> > >      with open(evil_kernel, 'wb') as fd:
> > >          fd.write(500 * b'\x01')
> > >
> > > +    test_fdt_add_pubkey(sha_algo, padding, sign_options)
> > >      try:
> > >          # We need to use our own device tree file. Remember to
> > > restore
> > > it
> > >          # afterwards.
> > > --
> > > 2.39.1
> > >
> > >
>