diff mbox series

[3/5] binman: capsule: Use dumped capsule header contents for verification

Message ID 20231004112722.416877-4-sughosh.ganu@linaro.org
State New
Headers show
Series Support for dumping capsule headers and empty capsules | expand

Commit Message

Sughosh Ganu Oct. 4, 2023, 11:27 a.m. UTC
The various fields of a generated capsule are currently verified
through hard-coded offsets. Use the dump-capsule feature for dumping
the capsule header contents and use those for capsule verification.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 tools/binman/ftest.py | 95 ++++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 41 deletions(-)

Comments

Simon Glass Oct. 7, 2023, 11:09 p.m. UTC | #1
Hi Sughosh,

On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The various fields of a generated capsule are currently verified
> through hard-coded offsets. Use the dump-capsule feature for dumping
> the capsule header contents and use those for capsule verification.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  tools/binman/ftest.py | 95 ++++++++++++++++++++++++-------------------
>  1 file changed, 54 insertions(+), 41 deletions(-)

This looks good apart from a few nits below. However, the tests fail for me.

>
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 8e419645a6..2b8871ab7e 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -121,9 +121,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
>  TEE_ADDR = 0x5678
>
>  # Firmware Management Protocol(FMP) GUID
> -FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
> +FW_MGMT_GUID = '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A'
>  # Image GUID specified in the DTS
> -CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
> +CAPSULE_IMAGE_GUID = '09D7CF52-0720-4710-91D1-08469B7FE9C8'
> +# Windows cert GUID
> +WIN_CERT_TYPE_EFI_GUID = '4AAFD29D-68DF-49EE-8AA9-347D375665A7'

Please use lower-case hex

>
>  class TestFunctional(unittest.TestCase):
>      """Functional tests for binman
> @@ -7223,52 +7225,63 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>          self.assertRegex(err,
>                           "Image 'image'.*missing bintools.*: bootgen")
>
> +    def _GetCapsuleHeaders(self, data):

This should have a function comment so it is clear what it is doing, returning.

> +        capsule_file = os.path.join(self._indir, 'test.capsule')
> +        tools.write_file(capsule_file, data)
> +
> +        out = tools.run('mkeficapsule', '--dump-capsule', capsule_file)
> +        lines = out.splitlines()
> +
> +        re_line = re.compile(r'^([^:\-\t]*)(?:\t*\s*:\s*(.*))?$')
> +        vals = collections.defaultdict(list)
> +        for line in lines:
> +            mat = re_line.match(line)
> +            if mat:
> +                vals[mat.group(1)] = mat.group(2)
> +
> +        return vals
> +
[..]

Regards,
Simon
Sughosh Ganu Oct. 9, 2023, 7:46 a.m. UTC | #2
hi Simon,

On Sun, 8 Oct 2023 at 04:41, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The various fields of a generated capsule are currently verified
> > through hard-coded offsets. Use the dump-capsule feature for dumping
> > the capsule header contents and use those for capsule verification.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  tools/binman/ftest.py | 95 ++++++++++++++++++++++++-------------------
> >  1 file changed, 54 insertions(+), 41 deletions(-)
>
> This looks good apart from a few nits below. However, the tests fail for me.

Can you please specify which tests fail, and the way to reproduce the
failures? I ran the tests before sending the patches, and they ran
fine, including the coverage which is 100%. Ci too did not complain.

>
> >
> > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > index 8e419645a6..2b8871ab7e 100644
> > --- a/tools/binman/ftest.py
> > +++ b/tools/binman/ftest.py
> > @@ -121,9 +121,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
> >  TEE_ADDR = 0x5678
> >
> >  # Firmware Management Protocol(FMP) GUID
> > -FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
> > +FW_MGMT_GUID = '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A'
> >  # Image GUID specified in the DTS
> > -CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
> > +CAPSULE_IMAGE_GUID = '09D7CF52-0720-4710-91D1-08469B7FE9C8'
> > +# Windows cert GUID
> > +WIN_CERT_TYPE_EFI_GUID = '4AAFD29D-68DF-49EE-8AA9-347D375665A7'
>
> Please use lower-case hex

Okay

>
> >
> >  class TestFunctional(unittest.TestCase):
> >      """Functional tests for binman
> > @@ -7223,52 +7225,63 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> >          self.assertRegex(err,
> >                           "Image 'image'.*missing bintools.*: bootgen")
> >
> > +    def _GetCapsuleHeaders(self, data):
>
> This should have a function comment so it is clear what it is doing, returning.

Will add

-sughosh

>
> > +        capsule_file = os.path.join(self._indir, 'test.capsule')
> > +        tools.write_file(capsule_file, data)
> > +
> > +        out = tools.run('mkeficapsule', '--dump-capsule', capsule_file)
> > +        lines = out.splitlines()
> > +
> > +        re_line = re.compile(r'^([^:\-\t]*)(?:\t*\s*:\s*(.*))?$')
> > +        vals = collections.defaultdict(list)
> > +        for line in lines:
> > +            mat = re_line.match(line)
> > +            if mat:
> > +                vals[mat.group(1)] = mat.group(2)
> > +
> > +        return vals
> > +
> [..]
>
> Regards,
> Simon
Simon Glass Oct. 9, 2023, 5:56 p.m. UTC | #3
Hi Sughosh,

On Mon, 9 Oct 2023 at 01:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sun, 8 Oct 2023 at 04:41, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.ganu@linaro.org>
wrote:
> > >
> > > The various fields of a generated capsule are currently verified
> > > through hard-coded offsets. Use the dump-capsule feature for dumping
> > > the capsule header contents and use those for capsule verification.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >  tools/binman/ftest.py | 95
++++++++++++++++++++++++-------------------
> > >  1 file changed, 54 insertions(+), 41 deletions(-)
> >
> > This looks good apart from a few nits below. However, the tests fail
for me.
>
> Can you please specify which tests fail, and the way to reproduce the
> failures? I ran the tests before sending the patches, and they ran
> fine, including the coverage which is 100%. Ci too did not complain.

Sure, for example:

$ binman test testCapsuleGen
======================== Running binman tests ========================
/usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering
(buffering=1) isn't supported in binary mode, the default buffer size will
be used
  return io.open(fd, mode, buffering, encoding, *args, **kwargs)
/usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering
(buffering=1) isn't supported in binary mode, the default buffer size will
be used
  return io.open(fd, mode, buffering, encoding, *args, **kwargs)
F
======================================================================
FAIL: binman.ftest.TestFunctional.testCapsuleGen (subunit.RemotedTestCase)
binman.ftest.TestFunctional.testCapsuleGen
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call
last):
AssertionError: '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A' != []


----------------------------------------------------------------------
Ran 1 test in 0.147s

FAILED (failures=1)


>
> >
> > >
> > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > > index 8e419645a6..2b8871ab7e 100644
> > > --- a/tools/binman/ftest.py
> > > +++ b/tools/binman/ftest.py
> > > @@ -121,9 +121,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4',
'lzma_alone', 'lzop', 'xz', 'zstd']
> > >  TEE_ADDR = 0x5678
> > >
> > >  # Firmware Management Protocol(FMP) GUID
> > > -FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
> > > +FW_MGMT_GUID = '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A'
> > >  # Image GUID specified in the DTS
> > > -CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
> > > +CAPSULE_IMAGE_GUID = '09D7CF52-0720-4710-91D1-08469B7FE9C8'
> > > +# Windows cert GUID
> > > +WIN_CERT_TYPE_EFI_GUID = '4AAFD29D-68DF-49EE-8AA9-347D375665A7'
> >
> > Please use lower-case hex
>
> Okay
>
> >
> > >
> > >  class TestFunctional(unittest.TestCase):
> > >      """Functional tests for binman
> > > @@ -7223,52 +7225,63 @@ fdt         fdtmap                Extract the
devicetree blob from the fdtmap
> > >          self.assertRegex(err,
> > >                           "Image 'image'.*missing bintools.*:
bootgen")
> > >
> > > +    def _GetCapsuleHeaders(self, data):
> >
> > This should have a function comment so it is clear what it is doing,
returning.
>
> Will add
>
> -sughosh
>
> >
> > > +        capsule_file = os.path.join(self._indir, 'test.capsule')
> > > +        tools.write_file(capsule_file, data)
> > > +
> > > +        out = tools.run('mkeficapsule', '--dump-capsule',
capsule_file)
> > > +        lines = out.splitlines()
> > > +
> > > +        re_line = re.compile(r'^([^:\-\t]*)(?:\t*\s*:\s*(.*))?$')
> > > +        vals = collections.defaultdict(list)
> > > +        for line in lines:
> > > +            mat = re_line.match(line)
> > > +            if mat:
> > > +                vals[mat.group(1)] = mat.group(2)
> > > +
> > > +        return vals
> > > +
> > [..]
> >
> > Regards,
> > Simon

Regards,
SImon
Sughosh Ganu Oct. 9, 2023, 7:41 p.m. UTC | #4
hi Simon,

On Mon, 9 Oct 2023 at 23:27, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 9 Oct 2023 at 01:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sun, 8 Oct 2023 at 04:41, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > The various fields of a generated capsule are currently verified
> > > > through hard-coded offsets. Use the dump-capsule feature for dumping
> > > > the capsule header contents and use those for capsule verification.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >  tools/binman/ftest.py | 95 ++++++++++++++++++++++++-------------------
> > > >  1 file changed, 54 insertions(+), 41 deletions(-)
> > >
> > > This looks good apart from a few nits below. However, the tests fail for me.
> >
> > Can you please specify which tests fail, and the way to reproduce the
> > failures? I ran the tests before sending the patches, and they ran
> > fine, including the coverage which is 100%. Ci too did not complain.
>
> Sure, for example:
>
> $ binman test testCapsuleGen
> ======================== Running binman tests ========================
> /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
>   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
>   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> F
> ======================================================================
> FAIL: binman.ftest.TestFunctional.testCapsuleGen (subunit.RemotedTestCase)
> binman.ftest.TestFunctional.testCapsuleGen
> ----------------------------------------------------------------------
> testtools.testresult.real._StringException: Traceback (most recent call last):
> AssertionError: '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A' != []
>
>
> ----------------------------------------------------------------------
> Ran 1 test in 0.147s
>
> FAILED (failures=1)

That is interesting. When I run the tests, they run just fine. I had
tested them before sending the patches. For e.g.

./tools/binman/binman test testCapsuleGen
======================== Running binman tests ========================
.
----------------------------------------------------------------------
Ran 1 test in 0.375s

OK

-sughosh

>
>
> >
> > >
> > > >
> > > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > > > index 8e419645a6..2b8871ab7e 100644
> > > > --- a/tools/binman/ftest.py
> > > > +++ b/tools/binman/ftest.py
> > > > @@ -121,9 +121,11 @@ COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
> > > >  TEE_ADDR = 0x5678
> > > >
> > > >  # Firmware Management Protocol(FMP) GUID
> > > > -FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
> > > > +FW_MGMT_GUID = '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A'
> > > >  # Image GUID specified in the DTS
> > > > -CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
> > > > +CAPSULE_IMAGE_GUID = '09D7CF52-0720-4710-91D1-08469B7FE9C8'
> > > > +# Windows cert GUID
> > > > +WIN_CERT_TYPE_EFI_GUID = '4AAFD29D-68DF-49EE-8AA9-347D375665A7'
> > >
> > > Please use lower-case hex
> >
> > Okay
> >
> > >
> > > >
> > > >  class TestFunctional(unittest.TestCase):
> > > >      """Functional tests for binman
> > > > @@ -7223,52 +7225,63 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> > > >          self.assertRegex(err,
> > > >                           "Image 'image'.*missing bintools.*: bootgen")
> > > >
> > > > +    def _GetCapsuleHeaders(self, data):
> > >
> > > This should have a function comment so it is clear what it is doing, returning.
> >
> > Will add
> >
> > -sughosh
> >
> > >
> > > > +        capsule_file = os.path.join(self._indir, 'test.capsule')
> > > > +        tools.write_file(capsule_file, data)
> > > > +
> > > > +        out = tools.run('mkeficapsule', '--dump-capsule', capsule_file)
> > > > +        lines = out.splitlines()
> > > > +
> > > > +        re_line = re.compile(r'^([^:\-\t]*)(?:\t*\s*:\s*(.*))?$')
> > > > +        vals = collections.defaultdict(list)
> > > > +        for line in lines:
> > > > +            mat = re_line.match(line)
> > > > +            if mat:
> > > > +                vals[mat.group(1)] = mat.group(2)
> > > > +
> > > > +        return vals
> > > > +
> > > [..]
> > >
> > > Regards,
> > > Simon
>
> Regards,
> SImon
Simon Glass Oct. 10, 2023, 2:57 p.m. UTC | #5
Hi Sughosh,

On Mon, 9 Oct 2023 at 13:41, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Mon, 9 Oct 2023 at 23:27, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 9 Oct 2023 at 01:46, Sughosh Ganu <sughosh.ganu@linaro.org>
wrote:
> > >
> > > hi Simon,
> > >
> > > On Sun, 8 Oct 2023 at 04:41, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.ganu@linaro.org>
wrote:
> > > > >
> > > > > The various fields of a generated capsule are currently verified
> > > > > through hard-coded offsets. Use the dump-capsule feature for
dumping
> > > > > the capsule header contents and use those for capsule
verification.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >  tools/binman/ftest.py | 95
++++++++++++++++++++++++-------------------
> > > > >  1 file changed, 54 insertions(+), 41 deletions(-)
> > > >
> > > > This looks good apart from a few nits below. However, the tests
fail for me.
> > >
> > > Can you please specify which tests fail, and the way to reproduce the
> > > failures? I ran the tests before sending the patches, and they ran
> > > fine, including the coverage which is 100%. Ci too did not complain.
> >
> > Sure, for example:
> >
> > $ binman test testCapsuleGen
> > ======================== Running binman tests ========================
> > /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering
(buffering=1) isn't supported in binary mode, the default buffer size will
be used
> >   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> > /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering
(buffering=1) isn't supported in binary mode, the default buffer size will
be used
> >   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> > F
> > ======================================================================
> > FAIL: binman.ftest.TestFunctional.testCapsuleGen
(subunit.RemotedTestCase)
> > binman.ftest.TestFunctional.testCapsuleGen
> > ----------------------------------------------------------------------
> > testtools.testresult.real._StringException: Traceback (most recent call
last):
> > AssertionError: '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A' != []
> >
> >
> > ----------------------------------------------------------------------
> > Ran 1 test in 0.147s
> >
> > FAILED (failures=1)
>
> That is interesting. When I run the tests, they run just fine. I had
> tested them before sending the patches. For e.g.
>
> ./tools/binman/binman test testCapsuleGen
> ======================== Running binman tests ========================
> .
> ----------------------------------------------------------------------
> Ran 1 test in 0.375s
>
> OK

Yes, I'm not sure what that is. Perhaps you have a required tool in your
path? But in that case I would expect to get some sort of error.

[..]

Regards,
Simon
Sughosh Ganu Oct. 10, 2023, 3 p.m. UTC | #6
hi Simon,

On Tue, 10 Oct 2023 at 20:27, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 9 Oct 2023 at 13:41, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Mon, 9 Oct 2023 at 23:27, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Mon, 9 Oct 2023 at 01:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Sun, 8 Oct 2023 at 04:41, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > The various fields of a generated capsule are currently verified
> > > > > > through hard-coded offsets. Use the dump-capsule feature for dumping
> > > > > > the capsule header contents and use those for capsule verification.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > >  tools/binman/ftest.py | 95 ++++++++++++++++++++++++-------------------
> > > > > >  1 file changed, 54 insertions(+), 41 deletions(-)
> > > > >
> > > > > This looks good apart from a few nits below. However, the tests fail for me.
> > > >
> > > > Can you please specify which tests fail, and the way to reproduce the
> > > > failures? I ran the tests before sending the patches, and they ran
> > > > fine, including the coverage which is 100%. Ci too did not complain.
> > >
> > > Sure, for example:
> > >
> > > $ binman test testCapsuleGen
> > > ======================== Running binman tests ========================
> > > /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
> > >   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> > > /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
> > >   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> > > F
> > > ======================================================================
> > > FAIL: binman.ftest.TestFunctional.testCapsuleGen (subunit.RemotedTestCase)
> > > binman.ftest.TestFunctional.testCapsuleGen
> > > ----------------------------------------------------------------------
> > > testtools.testresult.real._StringException: Traceback (most recent call last):
> > > AssertionError: '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A' != []
> > >
> > >
> > > ----------------------------------------------------------------------
> > > Ran 1 test in 0.147s
> > >
> > > FAILED (failures=1)
> >
> > That is interesting. When I run the tests, they run just fine. I had
> > tested them before sending the patches. For e.g.
> >
> > ./tools/binman/binman test testCapsuleGen
> > ======================== Running binman tests ========================
> > .
> > ----------------------------------------------------------------------
> > Ran 1 test in 0.375s
> >
> > OK
>
> Yes, I'm not sure what that is. Perhaps you have a required tool in your path? But in that case I would expect to get some sort of error.

I don't have any special tool in my path. Just that I build tools
before invoking the tests, since the mkeficapsule tool needs to have
been present. Moreover, the tests are also passing in the CI run. So
it seems to be something specific in your setup I guess.

-sughosh
Simon Glass Oct. 10, 2023, 3:14 p.m. UTC | #7
Hi Sughosh,

On Tue, 10 Oct 2023 at 09:01, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 10 Oct 2023 at 20:27, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 9 Oct 2023 at 13:41, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Mon, 9 Oct 2023 at 23:27, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 9 Oct 2023 at 01:46, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Sun, 8 Oct 2023 at 04:41, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 4 Oct 2023 at 05:27, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > The various fields of a generated capsule are currently verified
> > > > > > > through hard-coded offsets. Use the dump-capsule feature for dumping
> > > > > > > the capsule header contents and use those for capsule verification.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > ---
> > > > > > >  tools/binman/ftest.py | 95 ++++++++++++++++++++++++-------------------
> > > > > > >  1 file changed, 54 insertions(+), 41 deletions(-)
> > > > > >
> > > > > > This looks good apart from a few nits below. However, the tests fail for me.
> > > > >
> > > > > Can you please specify which tests fail, and the way to reproduce the
> > > > > failures? I ran the tests before sending the patches, and they ran
> > > > > fine, including the coverage which is 100%. Ci too did not complain.
> > > >
> > > > Sure, for example:
> > > >
> > > > $ binman test testCapsuleGen
> > > > ======================== Running binman tests ========================
> > > > /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
> > > >   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> > > > /usr/lib/python3.10/os.py:1030: RuntimeWarning: line buffering (buffering=1) isn't supported in binary mode, the default buffer size will be used
> > > >   return io.open(fd, mode, buffering, encoding, *args, **kwargs)
> > > > F
> > > > ======================================================================
> > > > FAIL: binman.ftest.TestFunctional.testCapsuleGen (subunit.RemotedTestCase)
> > > > binman.ftest.TestFunctional.testCapsuleGen
> > > > ----------------------------------------------------------------------
> > > > testtools.testresult.real._StringException: Traceback (most recent call last):
> > > > AssertionError: '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A' != []
> > > >
> > > >
> > > > ----------------------------------------------------------------------
> > > > Ran 1 test in 0.147s
> > > >
> > > > FAILED (failures=1)
> > >
> > > That is interesting. When I run the tests, they run just fine. I had
> > > tested them before sending the patches. For e.g.
> > >
> > > ./tools/binman/binman test testCapsuleGen
> > > ======================== Running binman tests ========================
> > > .
> > > ----------------------------------------------------------------------
> > > Ran 1 test in 0.375s
> > >
> > > OK
> >
> > Yes, I'm not sure what that is. Perhaps you have a required tool in your path? But in that case I would expect to get some sort of error.
>
> I don't have any special tool in my path. Just that I build tools
> before invoking the tests, since the mkeficapsule tool needs to have
> been present. Moreover, the tests are also passing in the CI run. So
> it seems to be something specific in your setup I guess.
>

Yes, I suppose so...I'll try the new version and try to figure it out.

Regards,
Simon
diff mbox series

Patch

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 8e419645a6..2b8871ab7e 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -121,9 +121,11 @@  COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', 'zstd']
 TEE_ADDR = 0x5678
 
 # Firmware Management Protocol(FMP) GUID
-FW_MGMT_GUID = 'edd5cb6d2de8444cbda17194199ad92a'
+FW_MGMT_GUID = '6DCBD5ED-E82D-4C44-BDA1-7194199AD92A'
 # Image GUID specified in the DTS
-CAPSULE_IMAGE_GUID = '52cfd7092007104791d108469b7fe9c8'
+CAPSULE_IMAGE_GUID = '09D7CF52-0720-4710-91D1-08469B7FE9C8'
+# Windows cert GUID
+WIN_CERT_TYPE_EFI_GUID = '4AAFD29D-68DF-49EE-8AA9-347D375665A7'
 
 class TestFunctional(unittest.TestCase):
     """Functional tests for binman
@@ -7223,52 +7225,63 @@  fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertRegex(err,
                          "Image 'image'.*missing bintools.*: bootgen")
 
+    def _GetCapsuleHeaders(self, data):
+        capsule_file = os.path.join(self._indir, 'test.capsule')
+        tools.write_file(capsule_file, data)
+
+        out = tools.run('mkeficapsule', '--dump-capsule', capsule_file)
+        lines = out.splitlines()
+
+        re_line = re.compile(r'^([^:\-\t]*)(?:\t*\s*:\s*(.*))?$')
+        vals = collections.defaultdict(list)
+        for line in lines:
+            mat = re_line.match(line)
+            if mat:
+                vals[mat.group(1)] = mat.group(2)
+
+        return vals
+
     def _CheckCapsule(self, data, signed_capsule=False, version_check=False,
                       capoemflags=False):
-        fmp_signature = "4d535331" # 'M', 'S', 'S', '1'
-        fmp_size = "10"
-        fmp_fw_version = "02"
-        oemflag = "0080"
+        fmp_signature = "3153534D" # 'M', 'S', 'S', '1'
+        fmp_size = "00000010"
+        fmp_fw_version = "00000002"
+        capsule_image_index = "00000001"
+        oemflag = "00018000"
+        auth_hdr_revision = "00000200"
+        auth_hdr_cert_type = "00000EF1"
 
-        payload_data = EFI_CAPSULE_DATA
+        payload_data_len = len(EFI_CAPSULE_DATA)
 
-        # TODO - Currently, these offsets for capsule fields are hardcoded.
-        # There are plans to add support to the mkeficapsule tool to dump
-        # the capsule contents which can then be used for capsule
-        # verification.
+        hdr = self._GetCapsuleHeaders(data)
 
-        # Firmware Management Protocol(FMP) GUID - offset(0 - 32)
-        self.assertEqual(FW_MGMT_GUID, data.hex()[:32])
-        # Image GUID - offset(96 - 128)
-        self.assertEqual(CAPSULE_IMAGE_GUID, data.hex()[96:128])
+        self.assertEqual(FW_MGMT_GUID, hdr['EFI_CAPSULE_HDR.CAPSULE_GUID'])
+
+        self.assertEqual(CAPSULE_IMAGE_GUID,
+                         hdr['FMP_CAPSULE_IMAGE_HDR.UPDATE_IMAGE_TYPE_ID'])
+        self.assertEqual(capsule_image_index,
+                         hdr['FMP_CAPSULE_IMAGE_HDR.UPDATE_IMAGE_INDEX'])
 
         if capoemflags:
-            # OEM Flags - offset(40 - 44)
-            self.assertEqual(oemflag, data.hex()[40:44])
-        if signed_capsule and version_check:
-            # FMP header signature - offset(4770 - 4778)
-            self.assertEqual(fmp_signature, data.hex()[4770:4778])
-            # FMP header size - offset(4778 - 4780)
-            self.assertEqual(fmp_size, data.hex()[4778:4780])
-            # firmware version - offset(4786 - 4788)
-            self.assertEqual(fmp_fw_version, data.hex()[4786:4788])
-            # payload offset signed capsule(4802 - 4808)
-            self.assertEqual(payload_data.hex(), data.hex()[4802:4808])
-        elif signed_capsule:
-            # payload offset signed capsule(4770 - 4776)
-            self.assertEqual(payload_data.hex(), data.hex()[4770:4776])
-        elif version_check:
-            # FMP header signature - offset(184 - 192)
-            self.assertEqual(fmp_signature, data.hex()[184:192])
-            # FMP header size - offset(192 - 194)
-            self.assertEqual(fmp_size, data.hex()[192:194])
-            # firmware version - offset(200 - 202)
-            self.assertEqual(fmp_fw_version, data.hex()[200:202])
-            # payload offset for non-signed capsule with version header(216 - 222)
-            self.assertEqual(payload_data.hex(), data.hex()[216:222])
-        else:
-            # payload offset for non-signed capsule with no version header(184 - 190)
-            self.assertEqual(payload_data.hex(), data.hex()[184:190])
+            self.assertEqual(oemflag, hdr['EFI_CAPSULE_HDR.FLAGS'])
+
+        if signed_capsule:
+            self.assertEqual(auth_hdr_revision,
+                             hdr['EFI_FIRMWARE_IMAGE_AUTH.AUTH_INFO.HDR.wREVISION'])
+            self.assertEqual(auth_hdr_cert_type,
+                             hdr['EFI_FIRMWARE_IMAGE_AUTH.AUTH_INFO.HDR.wCERTTYPE'])
+            self.assertEqual(WIN_CERT_TYPE_EFI_GUID,
+                             hdr['EFI_FIRMWARE_IMAGE_AUTH.AUTH_INFO.CERT_TYPE'])
+
+        if version_check:
+            self.assertEqual(fmp_signature,
+                             hdr['FMP_PAYLOAD_HDR.SIGNATURE'])
+            self.assertEqual(fmp_size,
+                             hdr['FMP_PAYLOAD_HDR.HEADER_SIZE'])
+            self.assertEqual(fmp_fw_version,
+                             hdr['FMP_PAYLOAD_HDR.FW_VERSION'])
+
+        self.assertEqual(payload_data_len, int(hdr['Payload Image Size']))
 
     def testCapsuleGen(self):
         """Test generation of EFI capsule"""