[RFC PATCH 0/2] RFC version for ATF and OP-TEE Firewalling
This is a start to firewall ATF and OP-TEE memory regions using the firewalls present in K3 SoCs. Please give reviews as to what can be better in the implementation.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com --- Manorit Chawdhry (2): binman: ti-secure: Add support for firewalling entities binman: j721e: Add firewall configurations for atf
arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++ tools/binman/btool/openssl.py | 16 +++- tools/binman/etype/ti_secure.py | 54 +++++++++++++ tools/binman/etype/x509_cert.py | 3 +- 4 files changed, 231 insertions(+), 3 deletions(-) --- base-commit: 321d7b4d875a77552a969dd6ea5bbed2644fcb0c change-id: 20230724-binman-firewalling-65ecdb23ec0a
Best regards,
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 | 54 +++++++++++++++++++++++++++++++++++++++++ tools/binman/etype/x509_cert.py | 3 ++- 3 files changed, 70 insertions(+), 3 deletions(-)
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..a980d10f4197 100644 --- a/tools/binman/etype/ti_secure.py +++ b/tools/binman/etype/ti_secure.py @@ -7,9 +7,39 @@
from binman.entry import EntryArg from binman.etype.x509_cert import Entry_x509_cert +from dataclasses import dataclass
from dtoc import fdt_util
+def hex_octet(n): + x = '%x' % (n,) + return ('0' * (len(x) % 2)) + x + +@dataclass +class Firewall(): + id: int + region: int + control : int + permissions: list[hex] + start_address: str + end_address: str + + def get_certificate(self) -> str: + unique_indentifier = f"{self.id}{self.region}" + cert = f""" +firewallID{unique_indentifier} = INTEGER:{self.id} +region{unique_indentifier} = INTEGER:{self.region} +control{unique_indentifier} = INTEGER:{hex(self.control)} +nPermissionRegs{unique_indentifier} = INTEGER:{len(self.permissions)} +""" + for index, permission in enumerate(self.permissions): + cert += f"""permissions{unique_indentifier}{index} = INTEGER:{hex(permission)} +""" + cert += f"""startAddress{unique_indentifier} = FORMAT:HEX,OCT:{hex_octet(self.start_address)} +endAddress{unique_indentifier} = FORMAT:HEX,OCT:{hex_octet(self.end_address)} +""" + return cert + class Entry_ti_secure(Entry_x509_cert): """Entry containing a TI x509 certificate binary
@@ -32,11 +62,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 +85,21 @@ 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: + 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,
Hi Manorit
On 05/09/23 13:51, 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 | 54 +++++++++++++++++++++++++++++++++++++++++ tools/binman/etype/x509_cert.py | 3 ++- 3 files changed, 70 insertions(+), 3 deletions(-)
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 thespecific host in firewall protected region- num_firewalls (int): The number of firewalls in theextended certificate- certificate (str): Extended firewall certificate withthe 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..a980d10f4197 100644 --- a/tools/binman/etype/ti_secure.py +++ b/tools/binman/etype/ti_secure.py @@ -7,9 +7,39 @@
from binman.entry import EntryArg from binman.etype.x509_cert import Entry_x509_cert +from dataclasses import dataclass
from dtoc import fdt_util
+def hex_octet(n):
- x = '%x' % (n,)
- return ('0' * (len(x) % 2)) + x
If this was the only way to grab the address this way, maybe consider adding this as a common fdt_util function?
+@dataclass +class Firewall():
- id: int
- region: int
- control : int
- permissions: list[hex]
- start_address: str
- end_address: str
- def get_certificate(self) -> str:
unique_indentifier = f"{self.id}{self.region}"cert = f"""+firewallID{unique_indentifier} = INTEGER:{self.id} +region{unique_indentifier} = INTEGER:{self.region} +control{unique_indentifier} = INTEGER:{hex(self.control)} +nPermissionRegs{unique_indentifier} = INTEGER:{len(self.permissions)} +"""
s/indentifier/identifier ?
for index, permission in enumerate(self.permissions):cert += f"""permissions{unique_indentifier}{index} = INTEGER:{hex(permission)}+"""
cert += f"""startAddress{unique_indentifier} = FORMAT:HEX,OCT:{hex_octet(self.start_address)}+endAddress{unique_indentifier} = FORMAT:HEX,OCT:{hex_octet(self.end_address)} +"""
return cert- class Entry_ti_secure(Entry_x509_cert): """Entry containing a TI x509 certificate binary
@@ -32,11 +62,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_placeself.ReadFirewallNode() self.sha = fdt_util.GetInt(self._node, 'sha', 512) self.req_dist_name = {'C': 'US', 'ST': 'TX',@@ -46,6 +85,21 @@ 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'] = 0for node in self._node.subnodes: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'] += 1self.firewall_cert_data['certificate'] += firewall.get_certificate()def GetCertificate(self, required): """Get the contents of this entrydiff --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,
For v1: - ti-secure node should contain the documentation of how to use this firewall sub-node, basically whatever you've put in x509_cert_sysfw() - add test(s) for complete test coverage
Hi Neha,
On 09:16-20230906, Neha Malcom Francis wrote:
Hi Manorit
On 05/09/23 13:51, 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 | 54 +++++++++++++++++++++++++++++++++++++++++ tools/binman/etype/x509_cert.py | 3 ++- 3 files changed, 70 insertions(+), 3 deletions(-)
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 thespecific host in firewall protected region- num_firewalls (int): The number of firewalls in theextended certificate- certificate (str): Extended firewall certificate withthe 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..a980d10f4197 100644 --- a/tools/binman/etype/ti_secure.py +++ b/tools/binman/etype/ti_secure.py @@ -7,9 +7,39 @@ from binman.entry import EntryArg from binman.etype.x509_cert import Entry_x509_cert +from dataclasses import dataclass from dtoc import fdt_util +def hex_octet(n):
- x = '%x' % (n,)
- return ('0' * (len(x) % 2)) + x
If this was the only way to grab the address this way, maybe consider adding this as a common fdt_util function?
Ah yes, I believe it can come helpful to other people using openssl too as openssl gets the input in this hex_octet form. Will be moving there as you mentioned. Thanks for the suggestion!
+@dataclass +class Firewall():
- id: int
- region: int
- control : int
- permissions: list[hex]
- start_address: str
- end_address: str
- def get_certificate(self) -> str:
unique_indentifier = f"{self.id}{self.region}"cert = f"""+firewallID{unique_indentifier} = INTEGER:{self.id} +region{unique_indentifier} = INTEGER:{self.region} +control{unique_indentifier} = INTEGER:{hex(self.control)} +nPermissionRegs{unique_indentifier} = INTEGER:{len(self.permissions)} +"""
s/indentifier/identifier ?
Ah yes, a typo.
for index, permission in enumerate(self.permissions):cert += f"""permissions{unique_indentifier}{index} = INTEGER:{hex(permission)}+"""
cert += f"""startAddress{unique_indentifier} = FORMAT:HEX,OCT:{hex_octet(self.start_address)}+endAddress{unique_indentifier} = FORMAT:HEX,OCT:{hex_octet(self.end_address)} +"""
return cert- class Entry_ti_secure(Entry_x509_cert): """Entry containing a TI x509 certificate binary
@@ -32,11 +62,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_placeself.ReadFirewallNode() self.sha = fdt_util.GetInt(self._node, 'sha', 512) self.req_dist_name = {'C': 'US', 'ST': 'TX',@@ -46,6 +85,21 @@ 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'] = 0for node in self._node.subnodes: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'] += 1self.firewall_cert_data['certificate'] += firewall.get_certificate()def GetCertificate(self, required): """Get the contents of this entrydiff --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,For v1:
- ti-secure node should contain the documentation of how to use this
firewall sub-node, basically whatever you've put in x509_cert_sysfw()
- add test(s) for complete test coverage
Sure, would keep these in mind. Thanks for reviewing!
Regards, Manorit
-- Thanking You Neha Malcom Francis
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com --- arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+)
diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi index 4f566c21a9af..0569a592597e 100644 --- a/arch/arm/dts/k3-j721e-binman.dtsi +++ b/arch/arm/dts/k3-j721e-binman.dtsi @@ -330,6 +330,73 @@ ti-secure { content = <&atf>; keyfile = "custMpk.pem"; + auth_in_place = <0xa02>; + + // cpu_0_cpu_0_msmc Background Firewall - 0 + firewall-0 { + id = <257>; + region = <0>; + control = <0x31a>; + permissions = <0xc3ffff>; + start_address = <0x0 0x0>; + end_address = <0xff 0xffffffff>; + }; + + // cpu_0_cpu_0_msmc Foreground Firewall + firewall-1 { + id = <257>; + region = <1>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x70000000>; + end_address = <0x0 0x7001ffff>; + }; + + // dru_0_msmc Background Firewall - 0 + firewall-4 { + id = <284>; + region = <0>; + control = <0x31a>; + permissions = <0xc3ffff>; + start_address = <0x0 0x0>; + end_address = <0xff 0xffffffff>; + }; + + // dru_0_msmc Foreground Firewall + firewall-5 { + id = <284>; + region = <1>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x70000000>; + end_address = <0x0 0x7001ffff>; + }; + + // Slave Background Firewall - 0 + // Already configured by secure entity + + // Slave Foreground Firewall + firewall-7 { + id = <4760>; + region = <1>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x70000000>; + end_address = <0x0 0x7001ffff>; + }; + + // Slave Background Firewall - 0 + // Already configured by secure entity + + // Slave Foreground Firewall + firewall-9 { + id = <4761>; + region = <1>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x70000000>; + end_address = <0x0 0x7001ffff>; + }; }; atf: atf-bl31 { }; @@ -346,6 +413,100 @@ ti-secure { content = <&tee>; keyfile = "custMpk.pem"; + auth_in_place = <0xa02>; + + // cpu_0_cpu_0_msmc Background Firewall - 0 + // configured during ATF Firewalling + + // cpu_0_cpu_0_msmc Foreground Firewall - 1 + // configured during ATF Firewalling + + // cpu_0_cpu_0_msmc Foreground Firewall - 2 + firewall-1 { + id = <257>; + region = <2>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x9e800000>; + end_address = <0x0 0x9fffffff>; + }; + + // dru_0_msmc Background Firewall - 0 + // configured during ATF Firewalling + + // dru_0_msmc Foreground Firewall - 1 + // configured during ATF Firewalling + + // dru_0_msmc Foreground Firewall - 2 + firewall-5 { + id = <284>; + region = <2>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x9e800000>; + end_address = <0x0 0x9fffffff>; + }; + + // Slave Background Firewall - 0 + firewall-6 { + id = <4762>; + region = <0>; + control = <0x31a>; + permissions = <0xc3ffff>; + start_address = <0x0 0x80000000>; + end_address = <0x0 0xffffffff>; + }; + + // Slave Background Firewall - 1 + firewall-7 { + id = <4762>; + region = <1>; + control = <0x31a>; + permissions = <0xc3ffff>; + start_address = <0x8 0x0>; + end_address = <0xf 0xffffffff>; + }; + + // Slave Foreground Firewall + firewall-8 { + id = <4762>; + region = <2>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x9e800000>; + end_address = <0x0 0x9fffffff>; + }; + + // Slave Background Firewall - 0 + firewall-9 { + id = <4763>; + region = <0>; + control = <0x31a>; + permissions = <0xc3ffff>; + start_address = <0x0 0x80000000>; + end_address = <0x0 0xffffffff>; + }; + + // Slave Background Firewall - 1 + firewall-10 { + id = <4763>; + region = <1>; + control = <0x31a>; + permissions = <0xc3ffff>; + start_address = <0x8 0x0>; + end_address = <0xf 0xffffffff>; + }; + + // Slave Foreground Firewall + firewall-11 { + id = <4763>; + region = <2>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x9e800000>; + end_address = <0x0 0x9fffffff>; + }; + }; tee: tee-os { };
On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com
arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+)
diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi index 4f566c21a9af..0569a592597e 100644 --- a/arch/arm/dts/k3-j721e-binman.dtsi +++ b/arch/arm/dts/k3-j721e-binman.dtsi @@ -330,6 +330,73 @@ ti-secure { content = <&atf>; keyfile = "custMpk.pem";
auth_in_place = <0xa02>;// cpu_0_cpu_0_msmc Background Firewall - 0
I don't have a personal preference, but I see most comments in DTS are C style /* */.
Also it might be easier to understand if you put the comments right before the properties that they relate to, i.g.
firewall-0 { /* cpu_0_cpu_0_msmc */ id = <257>; region = <0>; /* Background, Locked */ control = <0x31a>; permissions = <0xc3ffff>; start_address = <0x0 0x0>; end_address = <0xff 0xffffffff>; };
For permissions, I wonder if it would be easier to read if we add some helper macros:
#define FWPRIVID_ALL 0xc3
#define FWPERM_SECURE_PRIV_WRITE BIT(0) #define FWPERM_SECURE_PRIV_READ BIT(1) #define FWPERM_SECURE_PRIV_CACHEABLE BIT(2) #define FWPERM_SECURE_PRIV_DEBUG BIT(3) #define FWPERM_SECURE_USER_WRITE BIT(4) ...
Then you would have:
permissions = <FWPRIVID_ALL | FWPERM_SECURE_PRIV_WRITE | FWPERM_SECURE_PRIV_READ | ...
;
firewall-0 {id = <257>;region = <0>;control = <0x31a>;permissions = <0xc3ffff>;start_address = <0x0 0x0>;end_address = <0xff 0xffffffff>;};// cpu_0_cpu_0_msmc Foreground Firewallfirewall-1 {id = <257>;region = <1>;control = <0x1a>;permissions = <0x0100ff>;start_address = <0x0 0x70000000>;
This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR? Not sure how you would get the end address as we don't really know ATF size..
Andrew
Hi Andrew,
On 05/09/23 20:52, Andrew Davis wrote:
On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
[...]
+ // cpu_0_cpu_0_msmc Foreground Firewall + firewall-1 { + id = <257>; + region = <1>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x70000000>;
This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR? Not sure how you would get the end address as we don't really know ATF size..
I think this could be handled within binman IF CONFIG_K3_ATF_LOAD_ADDR and the ATF binary were not only A53/A72 inputs. But since it is, I don't see how that can be implemented.
Andrew
Hi Neha, Andrew,
On 09:20-20230906, Neha Malcom Francis wrote:
Hi Andrew,
On 05/09/23 20:52, Andrew Davis wrote:
On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
[...]
+ // cpu_0_cpu_0_msmc Foreground Firewall + firewall-1 { + id = <257>; + region = <1>; + control = <0x1a>; + permissions = <0x0100ff>; + start_address = <0x0 0x70000000>;
This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR? Not sure how you would get the end address as we don't really know ATF size..
I think this could be handled within binman IF CONFIG_K3_ATF_LOAD_ADDR and the ATF binary were not only A53/A72 inputs. But since it is, I don't see how that can be implemented.
Andrew,
I don't think that just using the CONFIG would be a good solution as the slave firewalls would not be able to protect the updated ATF address.
Maybe we can use configs and put some warning in build time if the ATF address is not the default, would see how this could be done getting the size is still an issue as Neha mentioned that they have to be inputs for all the build stages.
Neha,
Would you be able to elaborate more on that part?
Regards, Manorit
Andrew
-- Thanking You Neha Malcom Francis
Hi Andrew,
On 10:22-20230905, Andrew Davis wrote:
On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com
arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+)
diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi index 4f566c21a9af..0569a592597e 100644 --- a/arch/arm/dts/k3-j721e-binman.dtsi +++ b/arch/arm/dts/k3-j721e-binman.dtsi @@ -330,6 +330,73 @@ ti-secure { content = <&atf>; keyfile = "custMpk.pem";
auth_in_place = <0xa02>;// cpu_0_cpu_0_msmc Background Firewall - 0I don't have a personal preference, but I see most comments in DTS are C style /* */.
Also it might be easier to understand if you put the comments right before the properties that they relate to, i.g.
firewall-0 { /* cpu_0_cpu_0_msmc */ id = <257>; region = <0>; /* Background, Locked */ control = <0x31a>; permissions = <0xc3ffff>; start_address = <0x0 0x0>; end_address = <0xff 0xffffffff>; };
For permissions, I wonder if it would be easier to read if we add some helper macros:
#define FWPRIVID_ALL 0xc3
#define FWPERM_SECURE_PRIV_WRITE BIT(0) #define FWPERM_SECURE_PRIV_READ BIT(1) #define FWPERM_SECURE_PRIV_CACHEABLE BIT(2) #define FWPERM_SECURE_PRIV_DEBUG BIT(3) #define FWPERM_SECURE_USER_WRITE BIT(4) ...
Then you would have:
permissions = <FWPRIVID_ALL |
I think we'll have to do some shift here for the right calculations.
#define FWPRIVID_SHIFT 8
permissions = <(FWPRIVID_ALL << FWPRIVID_SHIFT)
FWPERM_SECURE_PRIV_WRITE | FWPERM_SECURE_PRIV_READ |
Also, I would like to make FWPERM_SECURE_PRIV_RWCD too as it'll be commonly used.
Would be sending another RFC with this change if you are fine with the suggestions. Thank you for reviewing!
Regards, Manorit
...
;
firewall-0 {id = <257>;region = <0>;control = <0x31a>;permissions = <0xc3ffff>;start_address = <0x0 0x0>;end_address = <0xff 0xffffffff>;};// cpu_0_cpu_0_msmc Foreground Firewallfirewall-1 {id = <257>;region = <1>;control = <0x1a>;permissions = <0x0100ff>;start_address = <0x0 0x70000000>;This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR? Not sure how you would get the end address as we don't really know ATF size..
Andrew
On 9/6/23 12:18 AM, Manorit Chawdhry wrote:
Hi Andrew,
On 10:22-20230905, Andrew Davis wrote:
On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com
arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+)
diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi index 4f566c21a9af..0569a592597e 100644 --- a/arch/arm/dts/k3-j721e-binman.dtsi +++ b/arch/arm/dts/k3-j721e-binman.dtsi @@ -330,6 +330,73 @@ ti-secure { content = <&atf>; keyfile = "custMpk.pem";
auth_in_place = <0xa02>;// cpu_0_cpu_0_msmc Background Firewall - 0I don't have a personal preference, but I see most comments in DTS are C style /* */.
Also it might be easier to understand if you put the comments right before the properties that they relate to, i.g.
firewall-0 { /* cpu_0_cpu_0_msmc */ id = <257>; region = <0>; /* Background, Locked */ control = <0x31a>; permissions = <0xc3ffff>; start_address = <0x0 0x0>; end_address = <0xff 0xffffffff>; };
For permissions, I wonder if it would be easier to read if we add some helper macros:
#define FWPRIVID_ALL 0xc3
#define FWPERM_SECURE_PRIV_WRITE BIT(0) #define FWPERM_SECURE_PRIV_READ BIT(1) #define FWPERM_SECURE_PRIV_CACHEABLE BIT(2) #define FWPERM_SECURE_PRIV_DEBUG BIT(3) #define FWPERM_SECURE_USER_WRITE BIT(4) ...
Then you would have:
permissions = <FWPRIVID_ALL |
I think we'll have to do some shift here for the right calculations.
#define FWPRIVID_SHIFT 8
permissions = <(FWPRIVID_ALL << FWPRIVID_SHIFT)
Right, I must have meant #define FWPRIVID_ALL 0xc30000 above, but a shift looks better.
FWPERM_SECURE_PRIV_WRITE | FWPERM_SECURE_PRIV_READ |Also, I would like to make FWPERM_SECURE_PRIV_RWCD too as it'll be commonly used.
Would be sending another RFC with this change if you are fine with the suggestions. Thank you for reviewing!
Works for me, should get rid of several of these magic numbers.
Andrew
Regards, Manorit
...;
firewall-0 {id = <257>;region = <0>;control = <0x31a>;permissions = <0xc3ffff>;start_address = <0x0 0x0>;end_address = <0xff 0xffffffff>;};// cpu_0_cpu_0_msmc Foreground Firewallfirewall-1 {id = <257>;region = <1>;control = <0x1a>;permissions = <0x0100ff>;start_address = <0x0 0x70000000>;This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR? Not sure how you would get the end address as we don't really know ATF size..
Andrew
Hi Andrew,
On 08:54-20230906, Andrew Davis wrote:
On 9/6/23 12:18 AM, Manorit Chawdhry wrote:
Hi Andrew,
On 10:22-20230905, Andrew Davis wrote:
On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com
arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+)
diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi index 4f566c21a9af..0569a592597e 100644 --- a/arch/arm/dts/k3-j721e-binman.dtsi +++ b/arch/arm/dts/k3-j721e-binman.dtsi @@ -330,6 +330,73 @@ ti-secure { content = <&atf>; keyfile = "custMpk.pem";
auth_in_place = <0xa02>;// cpu_0_cpu_0_msmc Background Firewall - 0I don't have a personal preference, but I see most comments in DTS are C style /* */.
Also it might be easier to understand if you put the comments right before the properties that they relate to, i.g.
firewall-0 { /* cpu_0_cpu_0_msmc */ id = <257>; region = <0>; /* Background, Locked */ control = <0x31a>; permissions = <0xc3ffff>; start_address = <0x0 0x0>; end_address = <0xff 0xffffffff>; };
For permissions, I wonder if it would be easier to read if we add some helper macros:
#define FWPRIVID_ALL 0xc3
#define FWPERM_SECURE_PRIV_WRITE BIT(0) #define FWPERM_SECURE_PRIV_READ BIT(1) #define FWPERM_SECURE_PRIV_CACHEABLE BIT(2) #define FWPERM_SECURE_PRIV_DEBUG BIT(3) #define FWPERM_SECURE_USER_WRITE BIT(4) ...
Then you would have:
permissions = <FWPRIVID_ALL |
I think we'll have to do some shift here for the right calculations.
#define FWPRIVID_SHIFT 8
permissions = <(FWPRIVID_ALL << FWPRIVID_SHIFT)
Right, I must have meant #define FWPRIVID_ALL 0xc30000 above, but a shift looks better.
FWPERM_SECURE_PRIV_WRITE | FWPERM_SECURE_PRIV_READ |Also, I would like to make FWPERM_SECURE_PRIV_RWCD too as it'll be commonly used.
Would be sending another RFC with this change if you are fine with the suggestions. Thank you for reviewing!
Works for me, should get rid of several of these magic numbers.
Andrew
firewall-0 { id = <257>; region = <0>; control = <(FWCTRL_EN | FWCTRL_LOCK | FWCTRL_BG | FWCTRL_CACHE)>; permissions = <((FWPRIVID_ALL << FWPRIVID_SHIFT) | FWPERM_SECURE_PRIV_RWCD | FWPERM_SECURE_USER_RWCD | FWPERM_NON_SECURE_PRIV_RWCD | FWPERM_NON_SECURE_USER_RWCD)>; start_address = <0x0 0x0>; end_address = <0xff 0xffffffff>; };
Does this look okay now?
Regards, Manorit
Regards, Manorit
...;
firewall-0 {id = <257>;region = <0>;control = <0x31a>;permissions = <0xc3ffff>;start_address = <0x0 0x0>;end_address = <0xff 0xffffffff>;};// cpu_0_cpu_0_msmc Foreground Firewallfirewall-1 {id = <257>;region = <1>;control = <0x1a>;permissions = <0x0100ff>;start_address = <0x0 0x70000000>;This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR? Not sure how you would get the end address as we don't really know ATF size..
Andrew
On 9/8/23 6:43 AM, Manorit Chawdhry wrote:
Hi Andrew,
On 08:54-20230906, Andrew Davis wrote:
On 9/6/23 12:18 AM, Manorit Chawdhry wrote:
Hi Andrew,
On 10:22-20230905, Andrew Davis wrote:
On 9/5/23 3:21 AM, Manorit Chawdhry wrote:
The following commits adds the configuration of firewalls required to protect ATF and OP-TEE memory region from non-secure reads and writes using master and slave firewalls present in our K3 SOCs.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com
arch/arm/dts/k3-j721e-binman.dtsi | 161 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+)diff --git a/arch/arm/dts/k3-j721e-binman.dtsi b/arch/arm/dts/k3-j721e-binman.dtsi index 4f566c21a9af..0569a592597e 100644 --- a/arch/arm/dts/k3-j721e-binman.dtsi +++ b/arch/arm/dts/k3-j721e-binman.dtsi @@ -330,6 +330,73 @@ ti-secure { content = <&atf>; keyfile = "custMpk.pem";
auth_in_place = <0xa02>;// cpu_0_cpu_0_msmc Background Firewall - 0I don't have a personal preference, but I see most comments in DTS are C style /* */.
Also it might be easier to understand if you put the comments right before the properties that they relate to, i.g.
firewall-0 { /* cpu_0_cpu_0_msmc */ id = <257>; region = <0>; /* Background, Locked */ control = <0x31a>; permissions = <0xc3ffff>; start_address = <0x0 0x0>; end_address = <0xff 0xffffffff>; };
For permissions, I wonder if it would be easier to read if we add some helper macros:
#define FWPRIVID_ALL 0xc3
#define FWPERM_SECURE_PRIV_WRITE BIT(0) #define FWPERM_SECURE_PRIV_READ BIT(1) #define FWPERM_SECURE_PRIV_CACHEABLE BIT(2) #define FWPERM_SECURE_PRIV_DEBUG BIT(3) #define FWPERM_SECURE_USER_WRITE BIT(4) ...
Then you would have:
permissions = <FWPRIVID_ALL |
I think we'll have to do some shift here for the right calculations.
#define FWPRIVID_SHIFT 8 permissions = <(FWPRIVID_ALL << FWPRIVID_SHIFT)Right, I must have meant #define FWPRIVID_ALL 0xc30000 above, but a shift looks better.
FWPERM_SECURE_PRIV_WRITE | FWPERM_SECURE_PRIV_READ |Also, I would like to make FWPERM_SECURE_PRIV_RWCD too as it'll be commonly used.
Would be sending another RFC with this change if you are fine with the suggestions. Thank you for reviewing!
Works for me, should get rid of several of these magic numbers.
Andrew
firewall-0 { id = <257>; region = <0>; control = <(FWCTRL_EN | FWCTRL_LOCK | FWCTRL_BG | FWCTRL_CACHE)>; permissions = <((FWPRIVID_ALL << FWPRIVID_SHIFT) | FWPERM_SECURE_PRIV_RWCD | FWPERM_SECURE_USER_RWCD | FWPERM_NON_SECURE_PRIV_RWCD | FWPERM_NON_SECURE_USER_RWCD)>; start_address = <0x0 0x0>; end_address = <0xff 0xffffffff>; };
Does this look okay now?
Yes, that can be parsed by humans now, which is much less error-prone than bitmask'd magic numbers :)
Andrew
Regards, Manorit
Regards, Manorit
...;
firewall-0 {id = <257>;region = <0>;control = <0x31a>;permissions = <0xc3ffff>;start_address = <0x0 0x0>;end_address = <0xff 0xffffffff>;};// cpu_0_cpu_0_msmc Foreground Firewallfirewall-1 {id = <257>;region = <1>;control = <0x1a>;permissions = <0x0100ff>;start_address = <0x0 0x70000000>;This address might change if one moves ATF, might work to use CONFIG_K3_ATF_LOAD_ADDR? Not sure how you would get the end address as we don't really know ATF size..
Andrew
Manorit Chawdhry m-chawdhry@ti.com writes:
This is a start to firewall ATF and OP-TEE memory regions using the firewalls present in K3 SoCs. Please give reviews as to what can be better in the implementation.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com
Hi Manorit, thanks for the patches.
I hope this solution is easily scalable to sitara devices as well. Mainly considering the difference in type of firewalls.
Regards, Kamlesh
Hi Kamlesh,
On 14:49-20230912, Kamlesh Gurudasani wrote:
Manorit Chawdhry m-chawdhry@ti.com writes:
This is a start to firewall ATF and OP-TEE memory regions using the firewalls present in K3 SoCs. Please give reviews as to what can be better in the implementation.
Signed-off-by: Manorit Chawdhry m-chawdhry@ti.com
Hi Manorit, thanks for the patches.
I hope this solution is easily scalable to sitara devices as well. Mainly considering the difference in type of firewalls.
I am trying to keep both types of devices in mind and will change the implementation if required to incorporate both of them. I don't see any problems as such in the current implementation w.r.t Sitara devices also, will see if any occurs and will update you. Thanks.
Regards, Manorit
Regards, Kamlesh
participants (4)
-
Andrew Davis -
Kamlesh Gurudasani -
Manorit Chawdhry -
Neha Malcom Francis