[PATCH v4 1/8] binman: ti-secure: Add support for firewalling entities

[Simon Glass]

Hi Manorit,

On Tue, 7 Nov 2023 at 02:40, Manorit Chawdhry <m-chawdhry at ti.com> wrote:
>
> Hi Simon,
>
> On 08:29-20231012, Simon Glass wrote:
> > Hi Manorit,
> >
> > On Wed, 11 Oct 2023 at 22:46, Manorit Chawdhry <m-chawdhry at ti.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On 20:41-20231011, Simon Glass wrote:
> > > > Hi Manorit,
> > > >
> > > > On Tue, 10 Oct 2023 at 23:25, Manorit Chawdhry <m-chawdhry at ti.com> wrote:
> > > > >
> > > > > We can now firewall entities while loading them through our secure
> > > > > entity TIFS, the required information should be present in the
> > > > > certificate that is being parsed by TIFS.
> > > > >
> > > > > The following commit adds the support to enable the certificates to be
> > > > > generated if the firewall configurations are present in the binman dtsi
> > > > > nodes.
> > > > >
> > > > > Signed-off-by: Manorit Chawdhry <m-chawdhry at ti.com>
> > > > > ---
> > > > >  tools/binman/btool/openssl.py   | 16 +++++++-
> > > > >  tools/binman/etype/ti_secure.py | 90 +++++++++++++++++++++++++++++++++++++++++
> > > > >  tools/binman/etype/x509_cert.py |  3 +-
> > > > >  3 files changed, 106 insertions(+), 3 deletions(-)
> > > > >
> > > >
> > > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > > >
> > > > I'm still a little worried about the error reporting if the user
> > > > leaves out a property. Does it do the right thing?
> > > >
> > >
> > > I did make a test also along the lines that would check all the
> > > firewalling properties and just auth-in-place isn't checked at this
> > > stage and people could end up adding the firewalling node without
> > > auth-in-place. Do you want to cover that test case as well? Regarding
> > > the other firewalling properties ( immitating the test by removing
> > > "id"), We get this:
> > >
> > > [..]
> > >   AR      spl/common/spl/built-in.o
> > >   LD      spl/u-boot-spl
> > >   OBJCOPY spl/u-boot-spl-nodtb.bin
> > >   SYM     spl/u-boot-spl.sym
> > >   CAT     spl/u-boot-spl-dtb.bin
> > >   COPY    spl/u-boot-spl.bin
> > >   BINMAN  .binman_stamp
> > > binman: id can't be None in firewall node
> >
> > That's a bit vague...at least it should show the full path to the node
> > that failure, e.g. use self.Raise(). Also, I assume the property is
> > missing and the None refers to the Python value for the var. Better to
> > provide a nice error.
>
> I was working on this so I realised that Firewall class that I have is
> not an etype so I have hacked a bit around to get that self.Raise in but
> hoping that shouldn't be much of a problem..
>
> Does this error look okay now?
>
> [..]
>   COPY    spl/u-boot-spl.bin
>   BINMAN  .binman_stamp
> binman: Node '/binman/ti-spl/fit/images/atf/ti-secure': Subnode 'firewall' is missing properties: id,region

Yes

>
> >
> > Any possible user error should ideally be checked, to avoid later confusion.
> >
> > See Entry.ensure_props()
>
> I was thinking of using this as well but I noticed that this gets called
> in the beginning itself and since the Firewall class that I have is not
> an etype then I won't be getting the information that all this is
> failing in firewalling related node and might be confused with debugging
> in ti-secure hence thought of updating the dataclass only with the
> function only as you suggested below. Thanks for the suggestions!

That's fine, so long as you have the node path as you do above...it is
pretty easy for people to understand it when you have that.

>
> Regards,
> Manorit
>
> >
> > You could add an optional argument - the list of props to check. If
> > present it could use that instead of self.required_props
> >
> > Hmm better might be to have a new function which tells the user that
> > since the firewall property is present, it needs these others ones
> > too.
> >
> > Regards,
> > Simon

Regards.

Simon