Hi,
Does ARMv8 TrustZone provide BL1 in a secure ROM? Can a TPM be used as a
trusted root or is it useless?
If the TPM is changed to another, is the boot performed?
Thanks,
Inigo.
Hi Sumit,
On 1/8/20 7:09 AM, Sumit Garg via TF-A wrote:
> Hi Sandrine,
>
> On Fri, 20 Dec 2019 at 12:29, Sumit Garg <sumit.garg(a)linaro.org> wrote:
>>
>> Hi Sandrine,
>>
>> On Wed, 18 Dec 2019 at 21:07, Sandrine Bailleux
>> <Sandrine.Bailleux(a)arm.com> wrote:
>>>
>>> Hi Sumit,
>>>
>>> I've started providing some early review comments on your patches in
>>> Gerrit but these are mainly about implementation details. I would like
>>> to continue the higher-level design discussions here in parallel on the
>>> mailing list, if that's fine with you.
>>>
>>
>> Yes that's perfectly fine with me.
>>
>
> Do you have any further comments? As I have already incorporated most
> of the your comments related to design and implementation details in
> latest v4 patch-set [1].
>
> [1] https://review.trustedfirmware.org/q/topic:%22tbbr%252Ffw_enc%22+(status:op…
Apologies, with the Christmas break, I haven't had a chance to look at
your patches again. I will allocate some time for it this week and get
back to you.
Regards,
Sandrine
Hi Sumit,
I've started providing some early review comments on your patches in
Gerrit but these are mainly about implementation details. I would like
to continue the higher-level design discussions here in parallel on the
mailing list, if that's fine with you.
On 12/6/19 1:29 PM, Sumit Garg via TF-A wrote:
> Hi Sandrine,
>
> On Thu, 5 Dec 2019 at 20:31, Sandrine Bailleux
> <Sandrine.Bailleux(a)arm.com> wrote:
>>>> Also, I am still trying to get my head around how this would integrate
>>>> with a cryptographic engine where the key does not leave the chip. I can
>>>> imagine that we could get the address of the encrypted firmware image
>>>> from the FIP, pass that to a cryptographic engine, request it to decrypt
>>>> it and store the result somewhere in Trusted RAM. In this case, we
>>>> wouldn't call plat_get_fip_encryption_key(). Do you have any idea how we
>>>> would pull this off? Like how the different modules (IO layer, crypto
>>>> module, image parser module, ...) would integrate together?
>>>
>>> In this case, I would expect platform to provide key identifier rather
>>> than actual key as part of plat_get_fip_encryption_key() which is then
>>> passed onto auth_decrypt() that is implementation specific to
>>> cryptographic engine in similar terms as currently done for mbedTLS
>>> backend.
>>
>> Ah I see, so plat_get_fip_encryption_key() could return either the key
>> itself or a key identifier. Just like plat_get_rotpk_info() can return
>> either the key or a hash of it today. However, in the case of
>> plat_get_rotpk_info(), it also returns some flags indicating which one
>> it is. Don't we need something similar for
>> plat_get_fip_encryption_key()? How will the crypto module be able to
>> tell the difference between a key and a key ID otherwise? Or would you
>> expect a given crypto module backend to always use either keys or key
>> IDs, but not both?
>>
>
> It perfectly makes sense for crypto module backend to support both key
> ID and actual key. But I was thinking from use-case perspective that
> why would one like to expose key to the firmware if the crypto module
> could better protect it. And I could come up with one use-case where
> the SSK (burnt in SoC fuses) is provided as an actual key and BSSK
> (accessible only to crypto module) is provided via a key ID.
Yes, that makes sense to me.
> So yes I agree with you to provide additional flags field along with
> key buffer. Also, I think it makes sense to rename
> "plat_get_fip_encryption_key()" to "plat_get_fip_enc_key_info()".
Agree.
>>>> I have some concerns around the generation of the initialization vectors
>>>> in the encrypt_fw tool. Right now, IVs are simply a random sequence of
>>>> bytes (obtained through a call to OpenSSL's RAND_bytes() API). Now, I
>>>> would imagine that RAND_bytes() is typically based on a good random
>>>> number generator and thus will generate different sequences every time
>>>> it is called. At least, as long as it is called from the same machine
>>>> every time. But what if we encrypt a new FIP bundle from a different
>>>> machine, say in the context of a firmware update? Is it not possible
>>>> that it might choose the same IV out of bad luck?
>>>>
>>>> Perhaps that's an issue left to provisioning/manufacturing time and is
>>>> out of the scope here. But it worries me because AFAIU, the security of
>>>> AES-GCM is critically undermined if the same nonce is used multiple
>>>> times with the same key (see section 5.1.1. "Nonce reuse" in RFC 5116).
>>>> If the encryption key is the SSK (rather than the BSSK) then I guess the
>>>> probability is even higher, as it is shared amongst a class of devices.
>>>>
>>>
>>> Agree that "nonce" should be unique and using a random number
>>> generator available on build machine was an effort towards that. But
>>> thinking about the case that you have mentioned, I think we could have
>>> an optional user provided "nonce" as an input to "encrypt_fw" tool, so
>>> that the user is aware to randomly generate and provide a unique
>>> nonce.
>>
>> Yes, I like your suggestion of specifying the nonce to the tool. But I
>> think it should be the default behaviour then. You mention an *optional*
>> user-provided nonce, I would like to suggest we make this mandatory
>> instead. We could provide an option to request the tool to generate the
>> nonce, intended for development purposes.
>>
>
> Making nonce as default and mandatory command line option seems to be
> overkill for a user. It may also lead to unsafe practices of
> hardcoding a nonce in Makefile, shell scripts etc. So I think we
> should keep this as optional only for a narrow use-case where people
> might not trust uniqueness property of RNG from multiple build
> machines.
I don't know... Hard-coding a nonce in the makefile is unsafe for sure
but I would think that a nonce generated by the tool is also unsafe
without proper management of these nonces across keys and devices, but
in a much more subtle manner. I still think the nonce generation would
be better left outside of the scope of this tool.
> BTW, I have already added nonce command line option as part of v3
> patch-set [1]. Please have a look.
>
> [1] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/2496/2..3
>
>>>> Impact on memory footprint and performance
>>>> ------------------------------------------
>>>>
>>>> Do you know what the performance impact is when this feature is enabled
>>>> in TF-A, to decrypt images at boot time? Obviously it depends on the
>>>> platform and whether there is a dedicated cryptographic engine, and I
>>>> suppose you cannot really get any relevant measurements out of QEMU but
>>>> I would be interested if you've got any rough numbers.
>>>
>>> Following are measurements based on qemu for mbedTLS software library
>>> based authenticated decryption:
>>>
>>> BL31 plain:
>>> NOTICE: Load image time: 137us, size: 28KB
>>> BL31 encrypted:
>>> NOTICE: Load image time: 3979us, size: 28KB
>>>
>>> BL32 plain:
>>> NOTICE: Load image time: 1791us, size: 360KB
>>> BL32 encrypted:
>>> NOTICE: Load image time: 36339us, size: 360KB
>>
>> Thanks. So it's a 29% increase for BL31 and 20% for BL32. I would have
>> naively expected similar percentages, any idea why we get such a
>> difference between BL31 and BL32? I am just curious. Maybe it's down to
>> how the crypto algorithm/mode of operation works under the hood?
>
> Yes this additional increase for BL31 seems to be due to one time
> initialization operations carried on first invocation of mbedTLS gcm
> APIs, especially time taken to generate AES tables (see call to
> aes_gen_tables() in mbedtls/library/aes.c +576).
Ah I see, thanks for the explanation.
Regarding the memory impact, given that it's adding quite a bit of code
and data (around 9KB for BL1 and 12KB for BL2 from what you were saying
in a previous email), I think we need to make this feature optional and
wrap it under a build flag. Not everyone will need to encrypt firmware
images, having only authentication is a valid use case as well. If I am
not mistaken, your patches right now unconditionally pull the mbedTLS
primitives into BL1 and BL2 (as long as TBBR is enabled of course).
As a matter of fact, I am not able to build BL1 on FVP platform with
your patches, presumably due to the unconditional size increase. The
following build command:
> make TRUSTED_BOARD_BOOT=1 ARM_ROTPK_LOCATION=devel_rsa
MBEDTLS_DIR=mbedtls bl1
... fails at link time:
aarch64-linux-gnu-ld.bfd: build/fvp/release/bl1/bl1.elf section
`xlat_table' will not fit in region `RAM'
aarch64-linux-gnu-ld.bfd: BL1's RW section has exceeded its limit.
aarch64-linux-gnu-ld.bfd: region `RAM' overflowed by 4096 bytes
Makefile:889: recipe for target 'build/fvp/release/bl1/bl1.elf' failed
Regards,
Sandrine
Hi
> -----Original Message-----
> From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Soby Mathew
> via TF-A
> Sent: 06 January 2020 12:34
> To: Olivier Deprez <Olivier.Deprez(a)arm.com>; tf-a(a)lists.trustedfirmware.org;
> Sheetal Tigadoli <sheetal.tigadoli(a)broadcom.com>
> Cc: nd <nd(a)arm.com>
> Subject: Re: [TF-A] [Question] Why are the log related macros defined in
> multiple of 10's
>
> > Hi All,
> >
> > I wanted to understand the reason behind defining LOG_LEVEL_* macros
> > as multiple of 10's, also same goes with LOG_MARKER_* macros.
> > If we set the LOG macros to 1,2,3 and so on, we can avoid the mod and
> > division operation when printing the msgs to console.
> >
> > Please let me your thoughts on this.
> >
> > Link to this question on Phabricator -
> > https://developer.trustedfirmware.org/T624
> >
> > Thanks
> > Sheetal
>
As Sandrine said in Phabricator, the reason was simply to leave space between the levels to allow adding intermediate levels without breaking compatibility. Compatibility was a possible concern if external tools used the log level and you only rebuilt some of the TF-A components (components could end up using different values). However, I suspect the actual level value is never emitted in practice (just the text prefix) so this may not be a real concern.
> Hi Sheetal
> I didn't quite get reason for the optimization from multiples of 10 to
> consecutive numbers. The mod operation you refer to are assertions which will
> be compiled out when DEBUG=0 or when ENABLE_ASSSERTIONS=0. Other than that
> there is a single divide by 10 operation within the default implementation of
> plat_log_get_prefix().
>
>
> If the logging utility is considered performance critical, removing the
> single divide by 10 will not result in any gain. Usually the bottleneck for
> logs are UART baudrates and the CPU spends most of time spinning in a wait
> loop for UART to be free. Hence I don't see any need for change for log
> levels macro if the intent is performance optimization.
>
+1. Unless there's an actual performance gain, this sounds like an unnecessary change.
Dan.
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.
On 06/01/2020 12:34, Soby Mathew via TF-A wrote:
>> Hi All,
>>
>> I wanted to understand the reason behind defining LOG_LEVEL_* macros as
>> multiple of 10's, also same goes with LOG_MARKER_* macros.
>> If we set the LOG macros to 1,2,3 and so on, we can avoid the mod and
>> division operation when printing the msgs to console.
>>
>> Please let me your thoughts on this.
>>
>> Link to this question on Phabricator -
>> https://developer.trustedfirmware.org/T624
>>
>> Thanks
>> Sheetal
>
> Hi Sheetal
> I didn't quite get reason for the optimization from multiples of 10 to
> consecutive numbers. The mod operation you refer to are assertions which
> will be compiled out when DEBUG=0 or when ENABLE_ASSSERTIONS=0. Other
> than that there is a single divide by 10 operation within the default
> implementation of plat_log_get_prefix().
>
>
> If the logging utility is considered performance critical, removing the
> single divide by 10 will not result in any gain. Usually the bottleneck
> for logs are UART baudrates and the CPU spends most of time spinning in
> a wait loop for UART to be free. Hence I don't see any need for change
> for log levels macro if the intent is performance optimization.
>
> Best Regards
> Soby Mathew
>
Hi Sheetal
There is some discussion related to this here :
https://github.com/ARM-software/tf-issues/issues/462 and see associated
issues and PRs for more details.
Best Regards
Soby Mathew
>> --
>> TF-A mailing list
>> TF-A(a)lists.trustedfirmware.org
>> https://lists.trustedfirmware.org/mailman/listinfo/tf-a
>>
>
> Hi All,
>
> I wanted to understand the reason behind defining LOG_LEVEL_* macros as
> multiple of 10's, also same goes with LOG_MARKER_* macros.
> If we set the LOG macros to 1,2,3 and so on, we can avoid the mod and
> division operation when printing the msgs to console.
>
> Please let me your thoughts on this.
>
> Link to this question on Phabricator -
> https://developer.trustedfirmware.org/T624
>
> Thanks
> Sheetal
Hi Sheetal
I didn't quite get reason for the optimization from multiples of 10 to
consecutive numbers. The mod operation you refer to are assertions which
will be compiled out when DEBUG=0 or when ENABLE_ASSSERTIONS=0. Other
than that there is a single divide by 10 operation within the default
implementation of plat_log_get_prefix().
If the logging utility is considered performance critical, removing the
single divide by 10 will not result in any gain. Usually the bottleneck
for logs are UART baudrates and the CPU spends most of time spinning in
a wait loop for UART to be free. Hence I don't see any need for change
for log levels macro if the intent is performance optimization.
Best Regards
Soby Mathew
> --
> TF-A mailing list
> TF-A(a)lists.trustedfirmware.org
> https://lists.trustedfirmware.org/mailman/listinfo/tf-a
>
Hi Varun
Could you take a look at the Nvidia driver defect reported by the Coverity scan?
Regards.
Alexei.
________________________________
From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of scan-admin--- via TF-A <tf-a(a)lists.trustedfirmware.org>
Sent: 03 January 2020 08:19
To: tf-a(a)lists.trustedfirmware.org <tf-a(a)lists.trustedfirmware.org>
Subject: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware
Hi,
Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan.
1 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)
** CID 352832: Uninitialized variables (UNINIT)
/plat/nvidia/tegra/soc/t194/drivers/mce/nvg.c: 197 in nvg_update_ccplex_gsc()
________________________________________________________________________________________________________
*** CID 352832: Uninitialized variables (UNINIT)
/plat/nvidia/tegra/soc/t194/drivers/mce/nvg.c: 197 in nvg_update_ccplex_gsc()
191 ret = EINVAL;
192 } else {
193 nvg_set_request_data((uint64_t)TEGRA_NVG_CHANNEL_UPDATE_CCPLEX_GSC,
194 (uint64_t)gsc_idx);
195 }
196
>>> CID 352832: Uninitialized variables (UNINIT)
>>> Using uninitialized value "ret".
197 return ret;
198 }
199
200 /*
201 * Cache clean operation for all CCPLEX caches.
202 */
________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V0…
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
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.