On Fri, 12 Jun 2020 at 14:15, Sandrine Bailleux sandrine.bailleux@arm.com wrote:

Hello Etienne,

On 6/10/20 1:19 PM, Etienne Carriere via TF-A wrote:

...

Dear all,

As part of a patchset series review (topic scmi-msg), change [1] imports confine_array_index.h header file from OP-TEE OS repository. The file originates from the open source Fuschia project, see link in commit message of [1].

As being imported for external packages, the header file inherits Fushca and OP-TEE notices. The helper function can protect some data structure from side channel attacks leveraging index indirect access overflows during speculative execution. It is not Arm copyright. It is BSD-3-Clause license. I'll add an entry in the docs/license.rst for the file.

Where to locate the file? It is ok to add such a file in include/common? Does it deserve a specific lib path, like include/lib/speculconfie_array_index.h? Maybe add as include/lib/cpus/confine_array_index.h as it is CPU speculative matters?

Before we discuss the location of this header file, have we considered using the compiler support for speculative execution mitigations instead? I am referring to the __builtin_speculation_safe_value() macro, which I believe achieves the same goal as the code you propose to introduce here, i.e. protecting against Spectre v1 bounds-check bypass attacks.

TF-A already uses this compiler builtin today and provides a wrapper macro around it, see SPECULATION_SAFE_VALUE() in include/lib/utils_def.f (although I would argue this is not the best location one could think of...). For reference, this was introduced by commit [1].

True. I can get my changes series to use SPECULATION_SAFE_VALUE() instead.

According to Arm's whitepaper [2], the support for this compiler builtin was added in GCC 9 and LLVM/Clang was to follow shortly.

If these versions are too recent for you, then I believe the official location to get equivalent code is [3]. As stated there:

The header file provided here allows a migration path to using the builtin function for users who are unable to immediately upgrade to a compiler which supports the builtin.

So I would prefer we get the code from there rather than from the OP-TEE project, which got it from the Fuschia project ;)

Fair enough. I'll see with ARM-software/speculation-barrier if confine_array_index() helper function is of interest.

This is licensed under the Boost Software License 1.0, which we've never used in TF-A so I would need to check with our legal department whether this is OK,

Sure, thanks,

but I don't expect any issues there as this is described as a permissive license only requiring preservation of copyright and license notices.

This is assuming that both header files (the one from OP-TEE and the one from the Arm software Github repo) are equivalent... Is this the case? Is the code provided in OP-TEE perhaps more optimized?

The story starts in Fushia implementing x86 and and aarch64. OP-TEE imported the file and made the aarch32 (arm/thumb2) and fixed a Clang issue. Import and maturation took few commits / pull-requests in optee_os.git. Here I thought it would be better to support all 64/32b.

I'll see with ARM-software/speculation-barrier.git about this code.

Thanks for the feedback and suggestions. Regards, Etienne

Regards, Sandrine

[1] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=9edd8...

[2] https://developer.arm.com/support/arm-security-updates/speculative-processor...

[3] https://github.com/ARM-software/speculation-barrier