| Message ID | 20231011-binman-firewalling-v4-1-a08085d300e9@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | ATF and OP-TEE Firewalling for K3 devices. | expand |
Hi Manorit, On Tue, 10 Oct 2023 at 23:25, Manorit Chawdhry <m-chawdhry@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@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@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? Regards, Simon
Hi Simon, On 20:41-20231011, Simon Glass wrote: > Hi Manorit, > > On Tue, 10 Oct 2023 at 23:25, Manorit Chawdhry <m-chawdhry@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@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@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 > Regards, > Simon
Hi Simon, On 11:54-20231011, Manorit Chawdhry 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@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(-) > > from binman.entry import EntryArg > from binman.etype.x509_cert import Entry_x509_cert > +from dataclasses import dataclass What all python versions do we support in u-boot? I see that dataclasses are in-built from python3.7 but for older versions we would need to install them separately, do I need to add this in buildman requirements for those versions? Regards, Manorit
Hi Manorit, On Wed, 11 Oct 2023 at 23:02, Manorit Chawdhry <m-chawdhry@ti.com> wrote: > > Hi Simon, > > On 11:54-20231011, Manorit Chawdhry 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@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(-) > > > > from binman.entry import EntryArg > > from binman.etype.x509_cert import Entry_x509_cert > > +from dataclasses import dataclass > > What all python versions do we support in u-boot? I see that dataclasses > are in-built from python3.7 but for older versions we would need to > install them separately, do I need to add this in buildman requirements > for those versions? I would prefer not to worry about Python 3.6 if possible. There are a few workarounds in buildman for it, though. Regards, Simon
Hi Manorit, On Wed, 11 Oct 2023 at 22:46, Manorit Chawdhry <m-chawdhry@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@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@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@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. Any possible user error should ideally be checked, to avoid later confusion. See Entry.ensure_props() 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
Hi Simon, On 08:29-20231012, Simon Glass wrote: > Hi Manorit, > > On Wed, 11 Oct 2023 at 22:46, Manorit Chawdhry <m-chawdhry@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@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@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@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 > > 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! 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
Hi Manorit, On Tue, 7 Nov 2023 at 02:40, Manorit Chawdhry <m-chawdhry@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@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@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@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@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
diff --git a/tools/binman/btool/openssl.py b/tools/binman/btool/openssl.py index aad3b61ae27c..dff439df211f 100644 --- a/tools/binman/btool/openssl.py +++ b/tools/binman/btool/openssl.py @@ -82,7 +82,7 @@ imageSize = INTEGER:{len(indata)} return self.run_cmd(*args) def x509_cert_sysfw(self, cert_fname, input_fname, key_fname, sw_rev, - config_fname, req_dist_name_dict): + config_fname, req_dist_name_dict, firewall_cert_data): """Create a certificate to be booted by system firmware Args: @@ -94,6 +94,13 @@ imageSize = INTEGER:{len(indata)} req_dist_name_dict (dict): Dictionary containing key-value pairs of req_distinguished_name section extensions, must contain extensions for C, ST, L, O, OU, CN and emailAddress + firewall_cert_data (dict): + - auth_in_place (int): The Priv ID for copying as the + specific host in firewall protected region + - num_firewalls (int): The number of firewalls in the + extended certificate + - certificate (str): Extended firewall certificate with + the information for the firewall configurations. Returns: str: Tool output @@ -121,6 +128,7 @@ basicConstraints = CA:true 1.3.6.1.4.1.294.1.3 = ASN1:SEQUENCE:swrv 1.3.6.1.4.1.294.1.34 = ASN1:SEQUENCE:sysfw_image_integrity 1.3.6.1.4.1.294.1.35 = ASN1:SEQUENCE:sysfw_image_load +1.3.6.1.4.1.294.1.37 = ASN1:SEQUENCE:firewall [ swrv ] swrv = INTEGER:{sw_rev} @@ -132,7 +140,11 @@ imageSize = INTEGER:{len(indata)} [ sysfw_image_load ] destAddr = FORMAT:HEX,OCT:00000000 -authInPlace = INTEGER:2 +authInPlace = INTEGER:{hex(firewall_cert_data['auth_in_place'])} + +[ firewall ] +numFirewallRegions = INTEGER:{firewall_cert_data['num_firewalls']} +{firewall_cert_data['certificate']} ''', file=outf) args = ['req', '-new', '-x509', '-key', key_fname, '-nodes', '-outform', 'DER', '-out', cert_fname, '-config', config_fname, diff --git a/tools/binman/etype/ti_secure.py b/tools/binman/etype/ti_secure.py index d939dce57139..76649b07b1b8 100644 --- a/tools/binman/etype/ti_secure.py +++ b/tools/binman/etype/ti_secure.py @@ -7,9 +7,40 @@ from binman.entry import EntryArg from binman.etype.x509_cert import Entry_x509_cert +from dataclasses import dataclass from dtoc import fdt_util +@dataclass +class Firewall(): + id: int + region: int + control : int + permissions: list[hex] + start_address: str + end_address: str + + def __post_init__(self): + for key, val in self.__dict__.items(): + if val is None: + raise Exception(f"{key} can't be None in firewall node") + + def get_certificate(self) -> str: + unique_identifier = f"{self.id}{self.region}" + cert = f""" +firewallID{unique_identifier} = INTEGER:{self.id} +region{unique_identifier} = INTEGER:{self.region} +control{unique_identifier} = INTEGER:{hex(self.control)} +nPermissionRegs{unique_identifier} = INTEGER:{len(self.permissions)} +""" + for index, permission in enumerate(self.permissions): + cert += f"""permissions{unique_identifier}{index} = INTEGER:{hex(permission)} +""" + cert += f"""startAddress{unique_identifier} = FORMAT:HEX,OCT:{self.start_address:02x} +endAddress{unique_identifier} = FORMAT:HEX,OCT:{self.end_address:02x} +""" + return cert + class Entry_ti_secure(Entry_x509_cert): """Entry containing a TI x509 certificate binary @@ -17,6 +48,11 @@ class Entry_ti_secure(Entry_x509_cert): - content: List of phandles to entries to sign - keyfile: Filename of file containing key to sign binary with - sha: Hash function to be used for signing + - auth-in-place: This is an integer field that contains two pieces + of information + Lower Byte - Remains 0x02 as per our use case + ( 0x02: Move the authenticated binary back to the header ) + Upper Byte - The Host ID of the core owning the firewall Output files: - input.<unique_name> - input file passed to openssl @@ -25,6 +61,35 @@ class Entry_ti_secure(Entry_x509_cert): - cert.<unique_name> - output file generated by openssl (which is used as the entry contents) + Depending on auth-in-place information in the inputs, we read the + firewall nodes that describe the configurations of firewall that TIFS + will be doing after reading the certificate. + + The syntax of the firewall nodes are as such: + + firewall-257-0 { + id = <257>; /* The ID of the firewall being configured */ + region = <0>; /* Region number to configure */ + + control = /* The control register */ + <(FWCTRL_EN | FWCTRL_LOCK | FWCTRL_BG | FWCTRL_CACHE)>; + + permissions = /* The permission registers */ + <((FWPRIVID_ALL << FWPRIVID_SHIFT) | + FWPERM_SECURE_PRIV_RWCD | + FWPERM_SECURE_USER_RWCD | + FWPERM_NON_SECURE_PRIV_RWCD | + FWPERM_NON_SECURE_USER_RWCD)>; + + /* More defines can be found in k3-security.h */ + + start_address = /* The Start Address of the firewall */ + <0x0 0x0>; + end_address = /* The End Address of the firewall */ + <0xff 0xffffffff>; + }; + + openssl signs the provided data, using the TI templated config file and writes the signature in this entry. This allows verification that the data is genuine. @@ -32,11 +97,20 @@ class Entry_ti_secure(Entry_x509_cert): def __init__(self, section, etype, node): super().__init__(section, etype, node) self.openssl = None + self.firewall_cert_data: dict = { + 'auth_in_place': 0x02, + 'num_firewalls': 0, + 'certificate': '', + } def ReadNode(self): super().ReadNode() self.key_fname = self.GetEntryArgsOrProps([ EntryArg('keyfile', str)], required=True)[0] + auth_in_place = fdt_util.GetInt(self._node, 'auth-in-place') + if auth_in_place: + self.firewall_cert_data['auth_in_place'] = auth_in_place + self.ReadFirewallNode() self.sha = fdt_util.GetInt(self._node, 'sha', 512) self.req_dist_name = {'C': 'US', 'ST': 'TX', @@ -46,6 +120,22 @@ class Entry_ti_secure(Entry_x509_cert): 'CN': 'TI Support', 'emailAddress': 'support@ti.com'} + def ReadFirewallNode(self): + self.firewall_cert_data['certificate'] = "" + self.firewall_cert_data['num_firewalls'] = 0 + for node in self._node.subnodes: + if 'firewall' in node.name: + firewall = Firewall( + fdt_util.GetInt(node, 'id'), + fdt_util.GetInt(node, 'region'), + fdt_util.GetInt(node, 'control'), + fdt_util.GetPhandleList(node, 'permissions'), + fdt_util.GetInt64(node, 'start_address'), + fdt_util.GetInt64(node, 'end_address'), + ) + self.firewall_cert_data['num_firewalls'] += 1 + self.firewall_cert_data['certificate'] += firewall.get_certificate() + def GetCertificate(self, required): """Get the contents of this entry diff --git a/tools/binman/etype/x509_cert.py b/tools/binman/etype/x509_cert.py index d028cfe38cd9..9e1cf479023b 100644 --- a/tools/binman/etype/x509_cert.py +++ b/tools/binman/etype/x509_cert.py @@ -98,7 +98,8 @@ class Entry_x509_cert(Entry_collection): key_fname=self.key_fname, config_fname=config_fname, sw_rev=self.sw_rev, - req_dist_name_dict=self.req_dist_name) + req_dist_name_dict=self.req_dist_name, + firewall_cert_data=self.firewall_cert_data) elif type == 'rom': stdout = self.openssl.x509_cert_rom( cert_fname=output_fname,
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@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(-)