Re: [TF-A] [RFC] New feature in TF-A to load encrypted FIP payloads: address implementation concerns
Hi Everyone,
I have tried to address most of the implementation concerns with updated patch-set [1] as follows:
*Concern*: Firmware encryption bit needs to be signed *Address*: Moved the firmware encryption bit from FIP ToC header to "io_uuid_spec_t" struct which is part of "plat_io_policy" that is embedded in the boot-loader (BL1 or BL2) and hence firmware encryption bit is signed.
Also, with this implementation fip_tool is no longer aware of encryption and just encrypted binaries are piped to fip_tool.
*Concern*: Capability to encrypt with different keys for different images *Address*: Passed "img_id" buffer reference as an argument of "plat_get_enc_key_info()" API. So that platforms may choose to either provide a unique key per firmware image or just derive a key from HUK per firmware using "img_id" buffer as a salt.
*Concern*: Coupling of FIP and encryption layer *Address*: Firstly I think we all can agree that encryption layer provides confidentiality protection specific to IO storage. Secondly FIP is actually a packaging layer that sits over actual IO layer and having the encryption layer coupled with FIP provides an abstraction layer for any FIP payload which in turn provides the following features: 1. Allows the firmware certificates to be encrypted as well to protect against cloning satisfying R050_TBBR_PROTECTION requirement. 2. Allows the firmware configuration data to be encrypted as well. 3. Provides a capability to have a secure key store as FIP payload which is protected using HUK.
*Concern*: Allow usage of alternative verify-then-decrypt method *Address*: A platform could disable this encryption layer and implement decryption as part of "bl2_plat_handle_post_image_load()".
Please let me know in case I missed any implementation concerns and feel free to provide your feedback on updated patch-set [1].
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:ope...)
Regards, Sumit
Hi Sumit,
Thanks again. I looked through the implementation and it looks fine. One thing that does not seem to be addressed is the question related to firmware update. How is the TF firmware update expected to work ? Does NS_BL1U require access to the decryption key ?
Thanks Raghu
On 2/19/20 3:51 AM, Sumit Garg wrote:
Hi Everyone,
I have tried to address most of the implementation concerns with updated patch-set [1] as follows:
*Concern*: Firmware encryption bit needs to be signed *Address*: Moved the firmware encryption bit from FIP ToC header to "io_uuid_spec_t" struct which is part of "plat_io_policy" that is embedded in the boot-loader (BL1 or BL2) and hence firmware encryption bit is signed.
Also, with this implementation fip_tool is no longer aware of encryption and just encrypted binaries are piped to fip_tool.
*Concern*: Capability to encrypt with different keys for different images *Address*: Passed "img_id" buffer reference as an argument of "plat_get_enc_key_info()" API. So that platforms may choose to either provide a unique key per firmware image or just derive a key from HUK per firmware using "img_id" buffer as a salt.
*Concern*: Coupling of FIP and encryption layer *Address*: Firstly I think we all can agree that encryption layer provides confidentiality protection specific to IO storage. Secondly FIP is actually a packaging layer that sits over actual IO layer and having the encryption layer coupled with FIP provides an abstraction layer for any FIP payload which in turn provides the following features:
- Allows the firmware certificates to be encrypted as well to protect
against cloning satisfying R050_TBBR_PROTECTION requirement. 2. Allows the firmware configuration data to be encrypted as well. 3. Provides a capability to have a secure key store as FIP payload which is protected using HUK.
*Concern*: Allow usage of alternative verify-then-decrypt method *Address*: A platform could disable this encryption layer and implement decryption as part of "bl2_plat_handle_post_image_load()".
Please let me know in case I missed any implementation concerns and feel free to provide your feedback on updated patch-set [1].
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:ope...)
Regards, Sumit
Hi Raghu,
On Thu, 20 Feb 2020 at 03:19, Raghu Krishnamurthy raghu.ncstate@icloud.com wrote:
Hi Sumit,
Thanks again. I looked through the implementation and it looks fine.
Thanks.
One thing that does not seem to be addressed is the question related to firmware update. How is the TF firmware update expected to work ? Does NS_BL1U require access to the decryption key ?
As per our earlier discussion on gerrit, I would expect BL1 to provide FWU_SMC_IMAGE_DEC_AUTH service.
IIRC, the common alternative way for firmware updates is via UEFI capsule updates and a secure partition or a Trusted OS service could be leveraged for firmware authentication/decryption prior to flash update.
-Sumit
Thanks Raghu
On 2/19/20 3:51 AM, Sumit Garg wrote:
Hi Everyone,
I have tried to address most of the implementation concerns with updated patch-set [1] as follows:
*Concern*: Firmware encryption bit needs to be signed *Address*: Moved the firmware encryption bit from FIP ToC header to "io_uuid_spec_t" struct which is part of "plat_io_policy" that is embedded in the boot-loader (BL1 or BL2) and hence firmware encryption bit is signed.
Also, with this implementation fip_tool is no longer aware of encryption and just encrypted binaries are piped to fip_tool.
*Concern*: Capability to encrypt with different keys for different images *Address*: Passed "img_id" buffer reference as an argument of "plat_get_enc_key_info()" API. So that platforms may choose to either provide a unique key per firmware image or just derive a key from HUK per firmware using "img_id" buffer as a salt.
*Concern*: Coupling of FIP and encryption layer *Address*: Firstly I think we all can agree that encryption layer provides confidentiality protection specific to IO storage. Secondly FIP is actually a packaging layer that sits over actual IO layer and having the encryption layer coupled with FIP provides an abstraction layer for any FIP payload which in turn provides the following features:
- Allows the firmware certificates to be encrypted as well to protect
against cloning satisfying R050_TBBR_PROTECTION requirement. 2. Allows the firmware configuration data to be encrypted as well. 3. Provides a capability to have a secure key store as FIP payload which is protected using HUK.
*Concern*: Allow usage of alternative verify-then-decrypt method *Address*: A platform could disable this encryption layer and implement decryption as part of "bl2_plat_handle_post_image_load()".
Please let me know in case I missed any implementation concerns and feel free to provide your feedback on updated patch-set [1].
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:ope...)
Regards, Sumit
On Thu, 20 Feb 2020 at 13:30, Sumit Garg sumit.garg@linaro.org wrote:
Hi Raghu,
On Thu, 20 Feb 2020 at 03:19, Raghu Krishnamurthy raghu.ncstate@icloud.com wrote:
Hi Sumit,
Thanks again. I looked through the implementation and it looks fine.
Thanks.
One thing that does not seem to be addressed is the question related to firmware update. How is the TF firmware update expected to work ? Does NS_BL1U require access to the decryption key ?
As per our earlier discussion on gerrit, I would expect BL1 to provide FWU_SMC_IMAGE_DEC_AUTH service.
Another thought on this, shouldn't NS_BL2U update complete FIP image as signed payload? In this case NS_BL2U authenticate complete FIP image using FWU_SMC_IMAGE_AUTH in one go. Also, this way updater doesn't need to be aware of any FIP parsing or this encryption feature.
Is there any specific use-case to authenticate each FIP payload individually during firmware update?
-Sumit
IIRC, the common alternative way for firmware updates is via UEFI capsule updates and a secure partition or a Trusted OS service could be leveraged for firmware authentication/decryption prior to flash update.
-Sumit
Thanks Raghu
On 2/19/20 3:51 AM, Sumit Garg wrote:
Hi Everyone,
I have tried to address most of the implementation concerns with updated patch-set [1] as follows:
*Concern*: Firmware encryption bit needs to be signed *Address*: Moved the firmware encryption bit from FIP ToC header to "io_uuid_spec_t" struct which is part of "plat_io_policy" that is embedded in the boot-loader (BL1 or BL2) and hence firmware encryption bit is signed.
Also, with this implementation fip_tool is no longer aware of encryption and just encrypted binaries are piped to fip_tool.
*Concern*: Capability to encrypt with different keys for different images *Address*: Passed "img_id" buffer reference as an argument of "plat_get_enc_key_info()" API. So that platforms may choose to either provide a unique key per firmware image or just derive a key from HUK per firmware using "img_id" buffer as a salt.
*Concern*: Coupling of FIP and encryption layer *Address*: Firstly I think we all can agree that encryption layer provides confidentiality protection specific to IO storage. Secondly FIP is actually a packaging layer that sits over actual IO layer and having the encryption layer coupled with FIP provides an abstraction layer for any FIP payload which in turn provides the following features:
- Allows the firmware certificates to be encrypted as well to protect
against cloning satisfying R050_TBBR_PROTECTION requirement. 2. Allows the firmware configuration data to be encrypted as well. 3. Provides a capability to have a secure key store as FIP payload which is protected using HUK.
*Concern*: Allow usage of alternative verify-then-decrypt method *Address*: A platform could disable this encryption layer and implement decryption as part of "bl2_plat_handle_post_image_load()".
Please let me know in case I missed any implementation concerns and feel free to provide your feedback on updated patch-set [1].
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:ope...)
Regards, Sumit
Hi Sumit, I have given some comments in the review regarding this.
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/2495/10
Basically we need to introduce more abstraction and follow the layering approach of the IO framework to introduce decrypt support.
Best Regards Soby Mathew
-----Original Message----- From: Sumit Garg sumit.garg@linaro.org Sent: 20 February 2020 12:21 To: Raghu Krishnamurthy raghu.ncstate@icloud.com Cc: tf-a@lists.trustedfirmware.org; Soby Mathew Soby.Mathew@arm.com; Sandrine Bailleux Sandrine.Bailleux@arm.com; Joakim Bech joakim.bech@linaro.org; Daniel Thompson daniel.thompson@linaro.org; Miklos Balint Miklos.Balint@arm.com; Kiyoshi Owada owada.kiyoshi@socionext.com; Adrian Shaw Adrian.Shaw@arm.com Subject: Re: [TF-A] [RFC] New feature in TF-A to load encrypted FIP payloads: address implementation concerns
On Thu, 20 Feb 2020 at 13:30, Sumit Garg sumit.garg@linaro.org wrote:
Hi Raghu,
On Thu, 20 Feb 2020 at 03:19, Raghu Krishnamurthy raghu.ncstate@icloud.com wrote:
Hi Sumit,
Thanks again. I looked through the implementation and it looks fine.
Thanks.
One thing that does not seem to be addressed is the question related to firmware update. How is the TF firmware update expected to work ? Does NS_BL1U require access to the decryption key ?
As per our earlier discussion on gerrit, I would expect BL1 to provide FWU_SMC_IMAGE_DEC_AUTH service.
Another thought on this, shouldn't NS_BL2U update complete FIP image as signed payload? In this case NS_BL2U authenticate complete FIP image using FWU_SMC_IMAGE_AUTH in one go. Also, this way updater doesn't need to be aware of any FIP parsing or this encryption feature.
Is there any specific use-case to authenticate each FIP payload individually during firmware update?
-Sumit
IIRC, the common alternative way for firmware updates is via UEFI capsule updates and a secure partition or a Trusted OS service could be leveraged for firmware authentication/decryption prior to flash update.
-Sumit
Thanks Raghu
On 2/19/20 3:51 AM, Sumit Garg wrote:
Hi Everyone,
I have tried to address most of the implementation concerns with updated patch-set [1] as follows:
*Concern*: Firmware encryption bit needs to be signed *Address*: Moved the firmware encryption bit from FIP ToC header to "io_uuid_spec_t" struct which is part of "plat_io_policy" that is embedded in the boot-loader (BL1 or BL2) and hence firmware encryption bit is signed.
Also, with this implementation fip_tool is no longer aware of encryption and just encrypted binaries are piped to fip_tool.
*Concern*: Capability to encrypt with different keys for different images *Address*: Passed "img_id" buffer reference as an argument of "plat_get_enc_key_info()" API. So that platforms may choose to either provide a unique key per firmware image or just derive a key from HUK per firmware using "img_id" buffer as a salt.
*Concern*: Coupling of FIP and encryption layer *Address*: Firstly I think we all can agree that encryption layer provides confidentiality protection specific to IO storage. Secondly FIP is actually a packaging layer that sits over actual IO layer and having the encryption layer coupled with FIP provides an abstraction layer for any FIP payload which in turn provides the
following features:
- Allows the firmware certificates to be encrypted as well to
protect against cloning satisfying R050_TBBR_PROTECTION requirement. 2. Allows the firmware configuration data to be encrypted as well. 3. Provides a capability to have a secure key store as FIP payload which is protected using HUK.
*Concern*: Allow usage of alternative verify-then-decrypt method *Address*: A platform could disable this encryption layer and implement decryption as part of "bl2_plat_handle_post_image_load()".
Please let me know in case I missed any implementation concerns and feel free to provide your feedback on updated patch-set [1].
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+( status:open%20OR%20status:merged)
Regards, Sumit
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.
Hi Soby,
I have added an IO abstraction layer for decryption support. Now this IO decrypt layer can be stacked over any IO/packaging interface eg. FIP etc. to support firmware encryption feature.
The updated patch-set is here [1]. Please have a look and let me know if you have any further comments.
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:ope...)
Regards, Sumit
On Tue, 25 Feb 2020 at 21:05, Soby Mathew Soby.Mathew@arm.com wrote:
Hi Sumit, I have given some comments in the review regarding this.
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/2495/10
Basically we need to introduce more abstraction and follow the layering approach of the IO framework to introduce decrypt support.
Best Regards Soby Mathew
-----Original Message----- From: Sumit Garg sumit.garg@linaro.org Sent: 20 February 2020 12:21 To: Raghu Krishnamurthy raghu.ncstate@icloud.com Cc: tf-a@lists.trustedfirmware.org; Soby Mathew Soby.Mathew@arm.com; Sandrine Bailleux Sandrine.Bailleux@arm.com; Joakim Bech joakim.bech@linaro.org; Daniel Thompson daniel.thompson@linaro.org; Miklos Balint Miklos.Balint@arm.com; Kiyoshi Owada owada.kiyoshi@socionext.com; Adrian Shaw Adrian.Shaw@arm.com Subject: Re: [TF-A] [RFC] New feature in TF-A to load encrypted FIP payloads: address implementation concerns
On Thu, 20 Feb 2020 at 13:30, Sumit Garg sumit.garg@linaro.org wrote:
Hi Raghu,
On Thu, 20 Feb 2020 at 03:19, Raghu Krishnamurthy raghu.ncstate@icloud.com wrote:
Hi Sumit,
Thanks again. I looked through the implementation and it looks fine.
Thanks.
One thing that does not seem to be addressed is the question related to firmware update. How is the TF firmware update expected to work ? Does NS_BL1U require access to the decryption key ?
As per our earlier discussion on gerrit, I would expect BL1 to provide FWU_SMC_IMAGE_DEC_AUTH service.
Another thought on this, shouldn't NS_BL2U update complete FIP image as signed payload? In this case NS_BL2U authenticate complete FIP image using FWU_SMC_IMAGE_AUTH in one go. Also, this way updater doesn't need to be aware of any FIP parsing or this encryption feature.
Is there any specific use-case to authenticate each FIP payload individually during firmware update?
-Sumit
IIRC, the common alternative way for firmware updates is via UEFI capsule updates and a secure partition or a Trusted OS service could be leveraged for firmware authentication/decryption prior to flash update.
-Sumit
Thanks Raghu
On 2/19/20 3:51 AM, Sumit Garg wrote:
Hi Everyone,
I have tried to address most of the implementation concerns with updated patch-set [1] as follows:
*Concern*: Firmware encryption bit needs to be signed *Address*: Moved the firmware encryption bit from FIP ToC header to "io_uuid_spec_t" struct which is part of "plat_io_policy" that is embedded in the boot-loader (BL1 or BL2) and hence firmware encryption bit is signed.
Also, with this implementation fip_tool is no longer aware of encryption and just encrypted binaries are piped to fip_tool.
*Concern*: Capability to encrypt with different keys for different images *Address*: Passed "img_id" buffer reference as an argument of "plat_get_enc_key_info()" API. So that platforms may choose to either provide a unique key per firmware image or just derive a key from HUK per firmware using "img_id" buffer as a salt.
*Concern*: Coupling of FIP and encryption layer *Address*: Firstly I think we all can agree that encryption layer provides confidentiality protection specific to IO storage. Secondly FIP is actually a packaging layer that sits over actual IO layer and having the encryption layer coupled with FIP provides an abstraction layer for any FIP payload which in turn provides the
following features:
- Allows the firmware certificates to be encrypted as well to
protect against cloning satisfying R050_TBBR_PROTECTION requirement. 2. Allows the firmware configuration data to be encrypted as well. 3. Provides a capability to have a secure key store as FIP payload which is protected using HUK.
*Concern*: Allow usage of alternative verify-then-decrypt method *Address*: A platform could disable this encryption layer and implement decryption as part of "bl2_plat_handle_post_image_load()".
Please let me know in case I missed any implementation concerns and feel free to provide your feedback on updated patch-set [1].
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+( status:open%20OR%20status:merged)
Regards, Sumit
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.
Hi Soby, Sandrine,
I hope that I am able to address almost every comment with updated patch-set [1].
So, would it be possible to have this feature incorporated in upcoming TF-A v2.3 release?
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:ope...)
Regards, Sumit
On Fri, 28 Feb 2020 at 10:18, Sumit Garg sumit.garg@linaro.org wrote:
Hi Soby,
I have added an IO abstraction layer for decryption support. Now this IO decrypt layer can be stacked over any IO/packaging interface eg. FIP etc. to support firmware encryption feature.
The updated patch-set is here [1]. Please have a look and let me know if you have any further comments.
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:ope...)
Regards, Sumit
On Tue, 25 Feb 2020 at 21:05, Soby Mathew Soby.Mathew@arm.com wrote:
Hi Sumit, I have given some comments in the review regarding this.
https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/2495/10
Basically we need to introduce more abstraction and follow the layering approach of the IO framework to introduce decrypt support.
Best Regards Soby Mathew
-----Original Message----- From: Sumit Garg sumit.garg@linaro.org Sent: 20 February 2020 12:21 To: Raghu Krishnamurthy raghu.ncstate@icloud.com Cc: tf-a@lists.trustedfirmware.org; Soby Mathew Soby.Mathew@arm.com; Sandrine Bailleux Sandrine.Bailleux@arm.com; Joakim Bech joakim.bech@linaro.org; Daniel Thompson daniel.thompson@linaro.org; Miklos Balint Miklos.Balint@arm.com; Kiyoshi Owada owada.kiyoshi@socionext.com; Adrian Shaw Adrian.Shaw@arm.com Subject: Re: [TF-A] [RFC] New feature in TF-A to load encrypted FIP payloads: address implementation concerns
On Thu, 20 Feb 2020 at 13:30, Sumit Garg sumit.garg@linaro.org wrote:
Hi Raghu,
On Thu, 20 Feb 2020 at 03:19, Raghu Krishnamurthy raghu.ncstate@icloud.com wrote:
Hi Sumit,
Thanks again. I looked through the implementation and it looks fine.
Thanks.
One thing that does not seem to be addressed is the question related to firmware update. How is the TF firmware update expected to work ? Does NS_BL1U require access to the decryption key ?
As per our earlier discussion on gerrit, I would expect BL1 to provide FWU_SMC_IMAGE_DEC_AUTH service.
Another thought on this, shouldn't NS_BL2U update complete FIP image as signed payload? In this case NS_BL2U authenticate complete FIP image using FWU_SMC_IMAGE_AUTH in one go. Also, this way updater doesn't need to be aware of any FIP parsing or this encryption feature.
Is there any specific use-case to authenticate each FIP payload individually during firmware update?
-Sumit
IIRC, the common alternative way for firmware updates is via UEFI capsule updates and a secure partition or a Trusted OS service could be leveraged for firmware authentication/decryption prior to flash update.
-Sumit
Thanks Raghu
On 2/19/20 3:51 AM, Sumit Garg wrote:
Hi Everyone,
I have tried to address most of the implementation concerns with updated patch-set [1] as follows:
*Concern*: Firmware encryption bit needs to be signed *Address*: Moved the firmware encryption bit from FIP ToC header to "io_uuid_spec_t" struct which is part of "plat_io_policy" that is embedded in the boot-loader (BL1 or BL2) and hence firmware encryption bit is signed.
Also, with this implementation fip_tool is no longer aware of encryption and just encrypted binaries are piped to fip_tool.
*Concern*: Capability to encrypt with different keys for different images *Address*: Passed "img_id" buffer reference as an argument of "plat_get_enc_key_info()" API. So that platforms may choose to either provide a unique key per firmware image or just derive a key from HUK per firmware using "img_id" buffer as a salt.
*Concern*: Coupling of FIP and encryption layer *Address*: Firstly I think we all can agree that encryption layer provides confidentiality protection specific to IO storage. Secondly FIP is actually a packaging layer that sits over actual IO layer and having the encryption layer coupled with FIP provides an abstraction layer for any FIP payload which in turn provides the
following features:
- Allows the firmware certificates to be encrypted as well to
protect against cloning satisfying R050_TBBR_PROTECTION requirement. 2. Allows the firmware configuration data to be encrypted as well. 3. Provides a capability to have a secure key store as FIP payload which is protected using HUK.
*Concern*: Allow usage of alternative verify-then-decrypt method *Address*: A platform could disable this encryption layer and implement decryption as part of "bl2_plat_handle_post_image_load()".
Please let me know in case I missed any implementation concerns and feel free to provide your feedback on updated patch-set [1].
[1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+( status:open%20OR%20status:merged)
Regards, Sumit
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.
Hi Sumit,
On 3/2/20 10:37 AM, Sumit Garg wrote:
I hope that I am able to address almost every comment with updated patch-set [1].
Thanks for reworking the patches, they look mostly good from my point of view. The last remaining item would be to address the MISRA violations that Coverity found, which I've copied & pasted for you on Gerrit in the top patch.
I know Soby might still have some concerns, he will let you know the details soon.
So, would it be possible to have this feature incorporated in upcoming TF-A v2.3 release?
I think this is a reasonable target.
One thing, though. I think it's fair to say that this patch stack is quite complex and interacts with lots of different parts of TF-A. I personally don't think I have the full picture in my head yet.
Also, this feature is only used on QEMU right now and I am not aware of anyone trying to enable it for their platforms just yet. Thus there is a risk we might have overlooked some issues that we'll discover at that time.
Furthermore, I know that you've done some testing of this feature on QEMU but this is not integrated into the CI loop right now. Thus, there is a risk that we might break it in the future and this will go unnoticed, unless you plan to test it regularly on your end.
For all these reasons, I would feel more comfortable if the feature was tagged as experimental for now. Would you be open to that?
We already have such experimental features in the tree like BTI, some part of PAuth, measured boot, DebugFS. Marking a feature as experimental mostly just lowers the users expectations, telling them that they are using this code at their own risks. It also means we don't provide any stability guarantees just yet and that we might break compatibility without any notice in order to make the interfaces and implementation evolve as we see fit.
Marking a feature as experimental is usually done by: * Saying so in the documentation. * Having the build system print a warning message when using the build flags enabling the experimental feature. * Saying so in the (upcoming) change log.
Regards, Sandrine
Hi Sandrine,
On Wed, 4 Mar 2020 at 18:27, Sandrine Bailleux sandrine.bailleux@arm.com wrote:
Hi Sumit,
On 3/2/20 10:37 AM, Sumit Garg wrote:
I hope that I am able to address almost every comment with updated patch-set [1].
Thanks for reworking the patches, they look mostly good from my point of view.
Thanks for your review.
The last remaining item would be to address the MISRA violations that Coverity found, which I've copied & pasted for you on Gerrit in the top patch.
I have tried to address most of MISRA violations and updated the patch-set. But since I don't have access to Coverity tool you are using, so can you please check if there is any relevant MISRA violation that I missed?
I know Soby might still have some concerns, he will let you know the details soon.
Sure, I will follow-up with Soby.
So, would it be possible to have this feature incorporated in upcoming TF-A v2.3 release?
I think this is a reasonable target.
Thanks.
One thing, though. I think it's fair to say that this patch stack is quite complex and interacts with lots of different parts of TF-A. I personally don't think I have the full picture in my head yet.
Agree.
Also, this feature is only used on QEMU right now and I am not aware of anyone trying to enable it for their platforms just yet. Thus there is a risk we might have overlooked some issues that we'll discover at that time.
Socionext being a silicon vendor is actively looking for this feature and I think they will build upon this feature to enable firmware encryption on their platforms to meet DRM robustness rules.
Furthermore, I know that you've done some testing of this feature on QEMU but this is not integrated into the CI loop right now. Thus, there is a risk that we might break it in the future and this will go unnoticed, unless you plan to test it regularly on your end.
Yeah we should plan to enable testing for this feature in CI loop.
For all these reasons, I would feel more comfortable if the feature was tagged as experimental for now. Would you be open to that?
Overall I agree with your concerns and fine with tagging this feature as experimental for upcoming release.
We already have such experimental features in the tree like BTI, some part of PAuth, measured boot, DebugFS. Marking a feature as experimental mostly just lowers the users expectations, telling them that they are using this code at their own risks. It also means we don't provide any stability guarantees just yet and that we might break compatibility without any notice in order to make the interfaces and implementation evolve as we see fit.
Marking a feature as experimental is usually done by:
- Saying so in the documentation.
Updated documentation.
- Having the build system print a warning message when using the build
flags enabling the experimental feature.
Updated Makefile to reflect this feature as experimental.
- Saying so in the (upcoming) change log.
I hope you can take care of this.
-Sumit
Regards, Sandrine
Hi Sandrine, Soby,
Is there anything pending on my part to have this feature patch-set merged in TF-A?
-Sumit
On Wed, 4 Mar 2020 at 21:25, Sumit Garg sumit.garg@linaro.org wrote:
Hi Sandrine,
On Wed, 4 Mar 2020 at 18:27, Sandrine Bailleux sandrine.bailleux@arm.com wrote:
Hi Sumit,
On 3/2/20 10:37 AM, Sumit Garg wrote:
I hope that I am able to address almost every comment with updated patch-set [1].
Thanks for reworking the patches, they look mostly good from my point of view.
Thanks for your review.
The last remaining item would be to address the MISRA violations that Coverity found, which I've copied & pasted for you on Gerrit in the top patch.
I have tried to address most of MISRA violations and updated the patch-set. But since I don't have access to Coverity tool you are using, so can you please check if there is any relevant MISRA violation that I missed?
I know Soby might still have some concerns, he will let you know the details soon.
Sure, I will follow-up with Soby.
So, would it be possible to have this feature incorporated in upcoming TF-A v2.3 release?
I think this is a reasonable target.
Thanks.
One thing, though. I think it's fair to say that this patch stack is quite complex and interacts with lots of different parts of TF-A. I personally don't think I have the full picture in my head yet.
Agree.
Also, this feature is only used on QEMU right now and I am not aware of anyone trying to enable it for their platforms just yet. Thus there is a risk we might have overlooked some issues that we'll discover at that time.
Socionext being a silicon vendor is actively looking for this feature and I think they will build upon this feature to enable firmware encryption on their platforms to meet DRM robustness rules.
Furthermore, I know that you've done some testing of this feature on QEMU but this is not integrated into the CI loop right now. Thus, there is a risk that we might break it in the future and this will go unnoticed, unless you plan to test it regularly on your end.
Yeah we should plan to enable testing for this feature in CI loop.
For all these reasons, I would feel more comfortable if the feature was tagged as experimental for now. Would you be open to that?
Overall I agree with your concerns and fine with tagging this feature as experimental for upcoming release.
We already have such experimental features in the tree like BTI, some part of PAuth, measured boot, DebugFS. Marking a feature as experimental mostly just lowers the users expectations, telling them that they are using this code at their own risks. It also means we don't provide any stability guarantees just yet and that we might break compatibility without any notice in order to make the interfaces and implementation evolve as we see fit.
Marking a feature as experimental is usually done by:
- Saying so in the documentation.
Updated documentation.
- Having the build system print a warning message when using the build
flags enabling the experimental feature.
Updated Makefile to reflect this feature as experimental.
- Saying so in the (upcoming) change log.
I hope you can take care of this.
-Sumit
Regards, Sandrine
Hi Sumit, Just some minor tidying up. I have put in new comments now.
Best regards Soby Mathew
-----Original Message----- From: Sumit Garg sumit.garg@linaro.org Sent: 06 March 2020 09:39 To: Sandrine Bailleux Sandrine.Bailleux@arm.com; Soby Mathew Soby.Mathew@arm.com Cc: Joakim Bech joakim.bech@linaro.org; Raghu Krishnamurthy raghu.ncstate@icloud.com; Daniel Thompson daniel.thompson@linaro.org; Miklos Balint Miklos.Balint@arm.com; Kiyoshi Owada owada.kiyoshi@socionext.com; Adrian Shaw Adrian.Shaw@arm.com; tf- a@lists.trustedfirmware.org; nd nd@arm.com Subject: Re: [TF-A] [RFC] New feature in TF-A to load encrypted FIP payloads: address implementation concerns
Hi Sandrine, Soby,
Is there anything pending on my part to have this feature patch-set merged in TF-A?
-Sumit
On Wed, 4 Mar 2020 at 21:25, Sumit Garg sumit.garg@linaro.org wrote:
Hi Sandrine,
On Wed, 4 Mar 2020 at 18:27, Sandrine Bailleux sandrine.bailleux@arm.com wrote:
Hi Sumit,
On 3/2/20 10:37 AM, Sumit Garg wrote:
I hope that I am able to address almost every comment with updated patch-set [1].
Thanks for reworking the patches, they look mostly good from my point of view.
Thanks for your review.
The last remaining item would be to address the MISRA violations that Coverity found, which I've copied & pasted for you on Gerrit in the top patch.
I have tried to address most of MISRA violations and updated the patch-set. But since I don't have access to Coverity tool you are using, so can you please check if there is any relevant MISRA violation that I missed?
I know Soby might still have some concerns, he will let you know the details soon.
Sure, I will follow-up with Soby.
So, would it be possible to have this feature incorporated in upcoming TF-A v2.3 release?
I think this is a reasonable target.
Thanks.
One thing, though. I think it's fair to say that this patch stack is quite complex and interacts with lots of different parts of TF-A. I personally don't think I have the full picture in my head yet.
Agree.
Also, this feature is only used on QEMU right now and I am not aware of anyone trying to enable it for their platforms just yet. Thus there is a risk we might have overlooked some issues that we'll discover at
that time.
Socionext being a silicon vendor is actively looking for this feature and I think they will build upon this feature to enable firmware encryption on their platforms to meet DRM robustness rules.
Furthermore, I know that you've done some testing of this feature on QEMU but this is not integrated into the CI loop right now. Thus, there is a risk that we might break it in the future and this will go unnoticed, unless you plan to test it regularly on your end.
Yeah we should plan to enable testing for this feature in CI loop.
For all these reasons, I would feel more comfortable if the feature was tagged as experimental for now. Would you be open to that?
Overall I agree with your concerns and fine with tagging this feature as experimental for upcoming release.
We already have such experimental features in the tree like BTI, some part of PAuth, measured boot, DebugFS. Marking a feature as experimental mostly just lowers the users expectations, telling them that they are using this code at their own risks. It also means we don't provide any stability guarantees just yet and that we might break compatibility without any notice in order to make the interfaces and implementation evolve as we see fit.
Marking a feature as experimental is usually done by:
- Saying so in the documentation.
Updated documentation.
- Having the build system print a warning message when using the
build flags enabling the experimental feature.
Updated Makefile to reflect this feature as experimental.
- Saying so in the (upcoming) change log.
I hope you can take care of this.
-Sumit
Regards, Sandrine
On Fri, 6 Mar 2020 at 16:09, Soby Mathew Soby.Mathew@arm.com wrote:
Hi Sumit, Just some minor tidying up. I have put in new comments now.
Patch-set updated and rebased.
-Sumit
Best regards Soby Mathew
-----Original Message----- From: Sumit Garg sumit.garg@linaro.org Sent: 06 March 2020 09:39 To: Sandrine Bailleux Sandrine.Bailleux@arm.com; Soby Mathew Soby.Mathew@arm.com Cc: Joakim Bech joakim.bech@linaro.org; Raghu Krishnamurthy raghu.ncstate@icloud.com; Daniel Thompson daniel.thompson@linaro.org; Miklos Balint Miklos.Balint@arm.com; Kiyoshi Owada owada.kiyoshi@socionext.com; Adrian Shaw Adrian.Shaw@arm.com; tf- a@lists.trustedfirmware.org; nd nd@arm.com Subject: Re: [TF-A] [RFC] New feature in TF-A to load encrypted FIP payloads: address implementation concerns
Hi Sandrine, Soby,
Is there anything pending on my part to have this feature patch-set merged in TF-A?
-Sumit
On Wed, 4 Mar 2020 at 21:25, Sumit Garg sumit.garg@linaro.org wrote:
Hi Sandrine,
On Wed, 4 Mar 2020 at 18:27, Sandrine Bailleux sandrine.bailleux@arm.com wrote:
Hi Sumit,
On 3/2/20 10:37 AM, Sumit Garg wrote:
I hope that I am able to address almost every comment with updated patch-set [1].
Thanks for reworking the patches, they look mostly good from my point of view.
Thanks for your review.
The last remaining item would be to address the MISRA violations that Coverity found, which I've copied & pasted for you on Gerrit in the top patch.
I have tried to address most of MISRA violations and updated the patch-set. But since I don't have access to Coverity tool you are using, so can you please check if there is any relevant MISRA violation that I missed?
I know Soby might still have some concerns, he will let you know the details soon.
Sure, I will follow-up with Soby.
So, would it be possible to have this feature incorporated in upcoming TF-A v2.3 release?
I think this is a reasonable target.
Thanks.
One thing, though. I think it's fair to say that this patch stack is quite complex and interacts with lots of different parts of TF-A. I personally don't think I have the full picture in my head yet.
Agree.
Also, this feature is only used on QEMU right now and I am not aware of anyone trying to enable it for their platforms just yet. Thus there is a risk we might have overlooked some issues that we'll discover at
that time.
Socionext being a silicon vendor is actively looking for this feature and I think they will build upon this feature to enable firmware encryption on their platforms to meet DRM robustness rules.
Furthermore, I know that you've done some testing of this feature on QEMU but this is not integrated into the CI loop right now. Thus, there is a risk that we might break it in the future and this will go unnoticed, unless you plan to test it regularly on your end.
Yeah we should plan to enable testing for this feature in CI loop.
For all these reasons, I would feel more comfortable if the feature was tagged as experimental for now. Would you be open to that?
Overall I agree with your concerns and fine with tagging this feature as experimental for upcoming release.
We already have such experimental features in the tree like BTI, some part of PAuth, measured boot, DebugFS. Marking a feature as experimental mostly just lowers the users expectations, telling them that they are using this code at their own risks. It also means we don't provide any stability guarantees just yet and that we might break compatibility without any notice in order to make the interfaces and implementation evolve as we see fit.
Marking a feature as experimental is usually done by:
- Saying so in the documentation.
Updated documentation.
- Having the build system print a warning message when using the
build flags enabling the experimental feature.
Updated Makefile to reflect this feature as experimental.
- Saying so in the (upcoming) change log.
I hope you can take care of this.
-Sumit
Regards, Sandrine
tf-a@lists.trustedfirmware.org
-
Raghu Krishnamurthy -
Sandrine Bailleux -
Soby Mathew -
Sumit Garg