Hi Robert, Andrej,
Regarding the first point, TF-M and Mbed Crypto are two separate projects, both containing a version of the standard "psa/crypto.h" header. Neither project can remove the header, nor rename it because the name is standardised by the PSA specs.
When Mbed Crypto is used as a library by TF-M, we install its PSA headers to "include/mbedcrypto/psa/crypto.h" and then add only the base "include" directory to the include search paths. Then there is no conflict between TF-M and Mbed Crypto headers, because the former can be included with #include "psa/crypto.h" and the latter with #include "mbedcrypto/psa/crypto.h". Only the Crypto service is linked with Mbed Crypto, which it uses as its backend implementation, so that is why it is the only part of TF-M to include Mbed Crypto headers. All other parts of TF-M include the TF-M psa/crypto.h header, which is implemented by service requests to the Crypto service.
The only other simple solution I see to this is not to add the Mbed Crypto include directory to the search path at all. Then Mbed Crypto headers would need to be included with #include "mbed-crypto/include/psa/crypto.h" etc.
I didn't get chance to read the other issues yet, but maybe it would be easier to create a task for each one on Phabricator (https://developer.trustedfirmware.org/), so that we can keep track of the discussion and work for each issue more easily?
Best wishes, Jamie
-----Original Message----- From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Andrej Butok via TF-M Sent: 12 September 2019 11:52 To: Robert Rostohar Robert.Rostohar@arm.com Cc: tf-m@lists.trustedfirmware.org Subject: Re: [TF-M] TF-M / CMSIS-Pack Alignment
Hi Robert,
Great! I gave up to convince about the first point https://developer.trustedfirmware.org/T428 As you are from ARM, hope, you will able to push through all your improvements.
Thanks, Andrej Butok
-----Original Message----- From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Robert Rostohar via TF-M Sent: Thursday, September 12, 2019 12:34 PM To: tf-m@lists.trustedfirmware.org Subject: [TF-M] TF-M / CMSIS-Pack Alignment
Hi,
We are looking into providing TF-M as a CMSIS-Pack [1] and have discovered a few issues in TF-M that are currently blocking us.
1. Crypto headers ./interface/include/psa clash with headers from mbed-crypto .include/psa
It seems that TF-M copies the crypto headers from mbed-crypto folder ./include/psa into folder ./mbedcrypto/psa. However TF-M also provides different crypto headers in folder ./interface/include/psa.
TF-M modules typically include "psa/crypto.h" except crypto service modules which include "mbedcrypto/psa/crypto.h" through "tfm_mbedcrypto_include.h".
The problem is that in our tools both include folders (./include from mbed-crypto installation and ./interface/include from TF-M) are in the global search path causing wrong headers being used.
Another issues is the use of "mbedcrypto" prefix in include "mbedcrypto/psa/crypto.h". We have mbed-crypto already installed and copying crypto headers would not be needed when using include "psa/crypto.h".
1. Device header
TF-M currently uses "cmsis.h" as the device header. This is not compliant with CMSIS [2] which defines the naming convention for device headers, startup files and system configuration files.
Silicon vendors typically define header filenames that match their device names.
The device agnostic way proposed by CMSIS is to use a preprocessor define CMSIS_device_header that reflects the actual device name and is provided by the build environment.
We suggest to replace: #include "cmsis.h" with: #include CMSIS_device_header
This would affect the following modules: ./secure_fw/core/arch/tfm_arch_v8m_base.c ./secure_fw/core/arch/tfm_arch_v8m_main.c ./secure_fw/core/arch/include/tfm_arch.h ./platform/ext/target: various target files
1. Conditional inclusion of secure services: Storage, Crypto, Attestation
Our concept is based on software components and we have described each secure service as a single component that is user selectable. This requires conditional inclusion of a secure service based on preprocessor definitions.
TF-M already supports this for secure services Audit Logging (#ifdef TFM_PARTITION_AUDIT_LOG) and Platform (#ifdef TFM_PARTITION_PLATFORM) and also for all test services (#ifdef TFM_PARTITION_TEST_...).
We suggest to add this also to secure services Storage (#ifdef TFM_PARTITION_STORAGE), Crypto (#ifdef TFM_PARTITION_CRYPTO) and Attestation (#ifdef TFM_PARTITION_INITIAL_ATTESTATION).
This would affect the following modules: ./secure_fw/services/tfm_partition_defs.inc ./secure_fw/services/tfm_service_list.inc ./secure_fw/services/tfm_spm_db.inc ./secure_fw/ns_callable/tfm_veneers.c ./interface/include/tfm_veneers.h
We are aware that those file are supposed to be autogenerated however we use them directly at this point. Adding the mentioned preprocessor defines should be trivial and would unblock us.
1. Conditional inclusion of individual test suites
We have described also test suites as individual components that are user selectable. This requires conditional inclusion of test suites based on preprocessor definitions.
TF-M already supports this for some test suites (#ifdef ENABLE_AUDIT_LOGGING_SERVICE_TESTS, ...).
We suggest to add this also for all other test suites.
Adding conditional inclusion for secure test suites: ./test/framework/secure_suites.c #ifdef ENABLE_SECURE_STORAGE_SERVICE_TESTS #ifdef ENABLE_CRYPTO_SERVICE_TESTS #ifdef ENABLE_INITIAL_ATTESTATION_SERVICE_TESTS
Adding conditional inclusion for non-secure test suites: ./test/framework/non_secure_suites.c #ifdef ENABLE_SECURE_STORAGE_SERVICE_TESTS #ifdef ENABLE_CRYPTO_SERVICE_TESTS #ifdef ENABLE_INITIAL_ATTESTATION_SERVICE_TESTS #ifdef ENABLE_QCBOR_TESTS
1. Deprecated Invert Test suite
Invert test suite seems to be deprecated. Tests do nothing and just return. It would make sense to remove it.
When we expose it as a component to the user it unnecessary increases the complexity of having another component that does nothing.
1. Tests on non-secure side include headers from secure side
Non-secure software should not include any secure side internal headers (ex: from ./secure_fw/core/include) but only those that are exposed as APIs (./interface/include).
The following test suites on the non-secure side include internal headers from secure side:
Attestation: attestation_ns_interface_testsuite.c #include "secure_fw/services/initial_attestation/attestation.h"
Core Positive: core_ns_positive_testsuite.c #include "test/test_services/tfm_core_test/core_test_defs.h" #include "tfm_core.h" // from ./secure_fw/core/include through core_test_defs.h #include "tfm_plat_test.h // from ./platform/include
Core Interactive: core_ns_interactive_testsuite.c #include "test/test_services/tfm_core_test/core_test_defs.h" #include "tfm_core.h" // from ./secure_fw/core/include through core_test_defs.h
./app/tfm_integ_test.c: #include "test/test_services/tfm_core_test/core_test_defs.h" #include "tfm_core.h" // from ./secure_fw/core/include through core_test_defs.h
This actually causes a compile error in our build because tfm_core.h defines the LOG_MSG macro (through secure_utilities.h) which clashes with the inline static function LOG_MSG defined in tfm_integ_test.h. We had to patch the tfm_integ_test.c by adding #undef LOG_MSG after the secure header is indirectly included.
./app/main_ns.c: #include "target_cfg.h" // from ./platform/ext/target/<target_name>
target_cfg.h from secure side also contains USART driver definitions for non-secure side. This should be decoupled and non-secure side should not include that header.
1. Dummy platform files
./platform/ext/target/<target_name>/dummy_boot_seed.c ./platform/ext/target/<target_name>/dummy_crypto_keys.c ./platform/ext/target/<target_name>/dummy_device_id.c ./platform/ext/target/<target_name>/dummy_nv_counters.c ./platform/ext/target/<target_name>/attest_hal.c
Dummy platform files are intended for testing only and provide a quick way of starting to test TF-M even when the platform files are not yet ported to the platform that the customer using.
They are identical and duplicated for all existing targets.
We propose to remove the mentioned dummy files from each target and put them in a single folder (./platform/ext/target/template).
This simplifies maintenance of the files and also provides a single location of those files that are being used as a platform independent component.
1. Console via USART
Console on secure side is retargeted to CMSIS USART driver (./platform/ext/common/uart_stdout.c).
USART driver Send function is called also from SVC with highest interrupt priority which blocks the USART interrupt and leads to deadlock. This is not manifested with Musca USART drivers which implement blocking send - not compliant with CMSIS USART Driver [3]. It does occur instantly with any other CMSIS compliant USART driver.
As far as I understand the console on secure side will be redesigned to cope with that.
There are also other issues with using the USART driver:
* missing wait while busy after send * missing power on/off
This is being address with: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.tru...
It also tries to address printf retargeting issues however this needs another iteration.
Console on non-secure side is also retargeted to CMSIS USART driver (./app/main_ns.c) however has less constrains.
It has the same issues with using the USART driver:
* missing wait while busy after send * missing power on/off
This is being address with: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.tru...
It also tries to address printf retargeting issues however this needs another iteration.
1. USART driver implementations for platforms included in TF-M
As already mentioned all USART drivers implemented for various platforms included in TF-M are not compliant with CMSIS USART Driver specification [3]. They implement blocking send/receive and no power on/off.
Drivers should be rewritten and should pass the CMSIS Driver Validation [4].
Please look into the above issues and help us to overcome them.
Thanks, Robert
[1] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co... [2] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co... [3] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co... [4] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co...
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. -- TF-M mailing list TF-M@lists.trustedfirmware.org https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.trus...
Hi Jamie,
The current solution is limiting how TF-M can be used. This is blocking toolchain and silicon vendors for wider adoption of TF-M. I have explained this already to @Ashutosh Singh and he confirmed that this is an issue.
Yes, you can prebuild mbed-crypto and use it as a library. However in systems based on software components, TF-M is a component and mbed-crypto also. They are built within a single project with all registered includes..
Having different implementations of an API header with the same name is simply not acceptable when components are global.
It would be really good to create a task for each of the issues I have reported in order to track them. I would appreciate if someone from TF-M team can create the tasks and assign to the right person.
Thanks, Robert
-----Original Message----- From: Jamie Fox Jamie.Fox@arm.com Sent: Thursday 12 September 2019 18:13 To: Andrej Butok andrey.butok@nxp.com; Robert Rostohar Robert.Rostohar@arm.com; tf-m@lists.trustedfirmware.org Cc: nd nd@arm.com Subject: RE: TF-M / CMSIS-Pack Alignment
Hi Robert, Andrej,
Regarding the first point, TF-M and Mbed Crypto are two separate projects, both containing a version of the standard "psa/crypto.h" header. Neither project can remove the header, nor rename it because the name is standardised by the PSA specs.
When Mbed Crypto is used as a library by TF-M, we install its PSA headers to "include/mbedcrypto/psa/crypto.h" and then add only the base "include" directory to the include search paths. Then there is no conflict between TF-M and Mbed Crypto headers, because the former can be included with #include "psa/crypto.h" and the latter with #include "mbedcrypto/psa/crypto.h". Only the Crypto service is linked with Mbed Crypto, which it uses as its backend implementation, so that is why it is the only part of TF-M to include Mbed Crypto headers. All other parts of TF-M include the TF-M psa/crypto.h header, which is implemented by service requests to the Crypto service.
The only other simple solution I see to this is not to add the Mbed Crypto include directory to the search path at all. Then Mbed Crypto headers would need to be included with #include "mbed-crypto/include/psa/crypto.h" etc.
I didn't get chance to read the other issues yet, but maybe it would be easier to create a task for each one on Phabricator (https://developer.trustedfirmware.org/), so that we can keep track of the discussion and work for each issue more easily?
Best wishes, Jamie
-----Original Message----- From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Andrej Butok via TF-M Sent: 12 September 2019 11:52 To: Robert Rostohar Robert.Rostohar@arm.com Cc: tf-m@lists.trustedfirmware.org Subject: Re: [TF-M] TF-M / CMSIS-Pack Alignment
Hi Robert,
Great! I gave up to convince about the first point https://developer.trustedfirmware.org/T428 As you are from ARM, hope, you will able to push through all your improvements.
Thanks, Andrej Butok
-----Original Message----- From: TF-M tf-m-bounces@lists.trustedfirmware.org On Behalf Of Robert Rostohar via TF-M Sent: Thursday, September 12, 2019 12:34 PM To: tf-m@lists.trustedfirmware.org Subject: [TF-M] TF-M / CMSIS-Pack Alignment
Hi,
We are looking into providing TF-M as a CMSIS-Pack [1] and have discovered a few issues in TF-M that are currently blocking us.
1. Crypto headers ./interface/include/psa clash with headers from mbed-crypto .include/psa
It seems that TF-M copies the crypto headers from mbed-crypto folder ./include/psa into folder ./mbedcrypto/psa. However TF-M also provides different crypto headers in folder ./interface/include/psa.
TF-M modules typically include "psa/crypto.h" except crypto service modules which include "mbedcrypto/psa/crypto.h" through "tfm_mbedcrypto_include.h".
The problem is that in our tools both include folders (./include from mbed-crypto installation and ./interface/include from TF-M) are in the global search path causing wrong headers being used.
Another issues is the use of "mbedcrypto" prefix in include "mbedcrypto/psa/crypto.h". We have mbed-crypto already installed and copying crypto headers would not be needed when using include "psa/crypto.h".
1. Device header
TF-M currently uses "cmsis.h" as the device header. This is not compliant with CMSIS [2] which defines the naming convention for device headers, startup files and system configuration files.
Silicon vendors typically define header filenames that match their device names.
The device agnostic way proposed by CMSIS is to use a preprocessor define CMSIS_device_header that reflects the actual device name and is provided by the build environment.
We suggest to replace: #include "cmsis.h" with: #include CMSIS_device_header
This would affect the following modules: ./secure_fw/core/arch/tfm_arch_v8m_base.c ./secure_fw/core/arch/tfm_arch_v8m_main.c ./secure_fw/core/arch/include/tfm_arch.h ./platform/ext/target: various target files
1. Conditional inclusion of secure services: Storage, Crypto, Attestation
Our concept is based on software components and we have described each secure service as a single component that is user selectable. This requires conditional inclusion of a secure service based on preprocessor definitions.
TF-M already supports this for secure services Audit Logging (#ifdef TFM_PARTITION_AUDIT_LOG) and Platform (#ifdef TFM_PARTITION_PLATFORM) and also for all test services (#ifdef TFM_PARTITION_TEST_...).
We suggest to add this also to secure services Storage (#ifdef TFM_PARTITION_STORAGE), Crypto (#ifdef TFM_PARTITION_CRYPTO) and Attestation (#ifdef TFM_PARTITION_INITIAL_ATTESTATION).
This would affect the following modules: ./secure_fw/services/tfm_partition_defs.inc ./secure_fw/services/tfm_service_list.inc ./secure_fw/services/tfm_spm_db.inc ./secure_fw/ns_callable/tfm_veneers.c ./interface/include/tfm_veneers.h
We are aware that those file are supposed to be autogenerated however we use them directly at this point. Adding the mentioned preprocessor defines should be trivial and would unblock us.
1. Conditional inclusion of individual test suites
We have described also test suites as individual components that are user selectable. This requires conditional inclusion of test suites based on preprocessor definitions.
TF-M already supports this for some test suites (#ifdef ENABLE_AUDIT_LOGGING_SERVICE_TESTS, ...).
We suggest to add this also for all other test suites.
Adding conditional inclusion for secure test suites: ./test/framework/secure_suites.c #ifdef ENABLE_SECURE_STORAGE_SERVICE_TESTS #ifdef ENABLE_CRYPTO_SERVICE_TESTS #ifdef ENABLE_INITIAL_ATTESTATION_SERVICE_TESTS
Adding conditional inclusion for non-secure test suites: ./test/framework/non_secure_suites.c #ifdef ENABLE_SECURE_STORAGE_SERVICE_TESTS #ifdef ENABLE_CRYPTO_SERVICE_TESTS #ifdef ENABLE_INITIAL_ATTESTATION_SERVICE_TESTS #ifdef ENABLE_QCBOR_TESTS
1. Deprecated Invert Test suite
Invert test suite seems to be deprecated. Tests do nothing and just return. It would make sense to remove it.
When we expose it as a component to the user it unnecessary increases the complexity of having another component that does nothing.
1. Tests on non-secure side include headers from secure side
Non-secure software should not include any secure side internal headers (ex: from ./secure_fw/core/include) but only those that are exposed as APIs (./interface/include).
The following test suites on the non-secure side include internal headers from secure side:
Attestation: attestation_ns_interface_testsuite.c #include "secure_fw/services/initial_attestation/attestation.h"
Core Positive: core_ns_positive_testsuite.c #include "test/test_services/tfm_core_test/core_test_defs.h" #include "tfm_core.h" // from ./secure_fw/core/include through core_test_defs.h #include "tfm_plat_test.h // from ./platform/include
Core Interactive: core_ns_interactive_testsuite.c #include "test/test_services/tfm_core_test/core_test_defs.h" #include "tfm_core.h" // from ./secure_fw/core/include through core_test_defs.h
./app/tfm_integ_test.c: #include "test/test_services/tfm_core_test/core_test_defs.h" #include "tfm_core.h" // from ./secure_fw/core/include through core_test_defs.h
This actually causes a compile error in our build because tfm_core.h defines the LOG_MSG macro (through secure_utilities.h) which clashes with the inline static function LOG_MSG defined in tfm_integ_test.h. We had to patch the tfm_integ_test.c by adding #undef LOG_MSG after the secure header is indirectly included.
./app/main_ns.c: #include "target_cfg.h" // from ./platform/ext/target/<target_name>
target_cfg.h from secure side also contains USART driver definitions for non-secure side. This should be decoupled and non-secure side should not include that header.
1. Dummy platform files
./platform/ext/target/<target_name>/dummy_boot_seed.c ./platform/ext/target/<target_name>/dummy_crypto_keys.c ./platform/ext/target/<target_name>/dummy_device_id.c ./platform/ext/target/<target_name>/dummy_nv_counters.c ./platform/ext/target/<target_name>/attest_hal.c
Dummy platform files are intended for testing only and provide a quick way of starting to test TF-M even when the platform files are not yet ported to the platform that the customer using.
They are identical and duplicated for all existing targets.
We propose to remove the mentioned dummy files from each target and put them in a single folder (./platform/ext/target/template).
This simplifies maintenance of the files and also provides a single location of those files that are being used as a platform independent component.
1. Console via USART
Console on secure side is retargeted to CMSIS USART driver (./platform/ext/common/uart_stdout.c).
USART driver Send function is called also from SVC with highest interrupt priority which blocks the USART interrupt and leads to deadlock. This is not manifested with Musca USART drivers which implement blocking send - not compliant with CMSIS USART Driver [3]. It does occur instantly with any other CMSIS compliant USART driver.
As far as I understand the console on secure side will be redesigned to cope with that.
There are also other issues with using the USART driver:
* missing wait while busy after send * missing power on/off
This is being address with: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.tru...
It also tries to address printf retargeting issues however this needs another iteration.
Console on non-secure side is also retargeted to CMSIS USART driver (./app/main_ns.c) however has less constrains.
It has the same issues with using the USART driver:
* missing wait while busy after send * missing power on/off
This is being address with: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.tru...
It also tries to address printf retargeting issues however this needs another iteration.
1. USART driver implementations for platforms included in TF-M
As already mentioned all USART drivers implemented for various platforms included in TF-M are not compliant with CMSIS USART Driver specification [3]. They implement blocking send/receive and no power on/off.
Drivers should be rewritten and should pass the CMSIS Driver Validation [4].
Please look into the above issues and help us to overcome them.
Thanks, Robert
[1] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co... [2] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co... [3] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co... [4] https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.keil.co...
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. -- TF-M mailing list TF-M@lists.trustedfirmware.org https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.trus... -- TF-M mailing list TF-M@lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-m
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
tf-m@lists.trustedfirmware.org
-
Jamie Fox -
Robert Rostohar