The local variables with type 'struct arm_smccc_res' are initialized
just after the declaration to avoid any possible read usage prior
to any write usage, which would constitute a violation of
MISRA C:2012 Rule 9.1.

This is already prevented by suitable checks in the code,
but the correctness of this approach is difficult to prove and
reason about.

Therefore, storing a suitable initial value in those registers
(OPTEE_SMC_RETURN_ENOTAVAIL) will prevent futher checks from
assuming the operation performed by the macro 'arm_smccc_smc'
was completed correctly.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
I was in doubt about the safe value to put in 'optee_relinquish_resources'
therefore I zero-initialized it.
---
 xen/arch/arm/tee/optee.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 301d205a36..2c2ae88c28 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -171,6 +171,10 @@ static bool optee_probe(void)
 {
     struct dt_device_node *node;
     struct arm_smccc_res resp;
+    resp.a0 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    resp.a1 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    resp.a2 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    resp.a3 = OPTEE_SMC_RETURN_ENOTAVAIL;
 
     /* Check for entry in dtb */
     node = dt_find_compatible_node(NULL, NULL, "linaro,optee-tz");
@@ -229,6 +233,7 @@ static int optee_domain_init(struct domain *d)
 {
     struct arm_smccc_res resp;
     struct optee_domain *ctx;
+    resp.a0 = OPTEE_SMC_RETURN_ENOTAVAIL;
 
     ctx = xzalloc(struct optee_domain);
     if ( !ctx )
@@ -640,7 +645,7 @@ static void free_optee_shm_buf_pg_list(struct optee_domain *ctx,
 
 static int optee_relinquish_resources(struct domain *d)
 {
-    struct arm_smccc_res resp;
+    struct arm_smccc_res resp = {0};
     struct optee_std_call *call, *call_tmp;
     struct shm_rpc *shm_rpc, *shm_rpc_tmp;
     struct optee_shm_buf *optee_shm_buf, *optee_shm_buf_tmp;
@@ -1169,6 +1174,10 @@ static void do_call_with_arg(struct optee_domain *ctx,
                              register_t a3, register_t a4, register_t a5)
 {
     struct arm_smccc_res res;
+    res.a0 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    res.a1 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    res.a2 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    res.a3 = OPTEE_SMC_RETURN_ENOTAVAIL;
 
     arm_smccc_smc(a0, a1, a2, a3, a4, a5, 0, OPTEE_CLIENT_ID(current->domain),
                   &res);
@@ -1608,6 +1617,8 @@ static void handle_exchange_capabilities(struct cpu_user_regs *regs)
 {
     struct arm_smccc_res resp;
     uint32_t caps;
+    resp.a0 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    resp.a1 = OPTEE_SMC_RETURN_ENOTAVAIL;
 
     /* Filter out unknown guest caps */
     caps = get_user_reg(regs, 1);
@@ -1643,6 +1654,10 @@ static bool optee_handle_call(struct cpu_user_regs *regs)
 {
     struct arm_smccc_res resp;
     struct optee_domain *ctx = current->domain->arch.tee;
+    resp.a0 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    resp.a1 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    resp.a2 = OPTEE_SMC_RETURN_ENOTAVAIL;
+    resp.a3 = OPTEE_SMC_RETURN_ENOTAVAIL;
 
     if ( !ctx )
         return false;
-- 
2.34.1

Hi Nicola,

On 20/07/2023 15:29, Nicola Vetrini wrote:
> The local variables with type 'struct arm_smccc_res' are initialized
> just after the declaration to avoid any possible read usage prior
> to any write usage, which would constitute a violation of
> MISRA C:2012 Rule 9.1.
> 
> This is already prevented by suitable checks in the code,
> but the correctness of this approach is difficult to prove and
> reason about.

So I looked at the implementation of arm_smccc_smc(). For arm64, it is 
(simplified):

if ( cpus_have_const_cap(ARM_SMCCC_1_1) )
    arm_smccc_1_1_smc(__VA_ARGS__);
else
    arm_smccc_1_0_smc(_VA_ARGS__);

In arm_smccc_1_1_smc(), we will explicitly initialize __res:

if ( ___res )
   *___res = (typeof(*___res)) {r0, r1, r2, r3};


Whereas for arm_smccc_1_0_smc(), we would call assembly function. I 
assuming this is the problem?

I think this is similar to the discussion we had on set_interrupts() and 
dt_set_cells(). If so, couldn't we tell ECLAIR that 
__arm_smccc_1_0_smc() will always initialize *res?

Cheers,

-- 
Julien Grall

On 20/07/23 17:54, Julien Grall wrote:
> Hi Nicola,
> 
> On 20/07/2023 15:29, Nicola Vetrini wrote:
>> The local variables with type 'struct arm_smccc_res' are initialized
>> just after the declaration to avoid any possible read usage prior
>> to any write usage, which would constitute a violation of
>> MISRA C:2012 Rule 9.1.
>>
>> This is already prevented by suitable checks in the code,
>> but the correctness of this approach is difficult to prove and
>> reason about.
> 
> So I looked at the implementation of arm_smccc_smc(). For arm64, it is 
> (simplified):
> 
> if ( cpus_have_const_cap(ARM_SMCCC_1_1) )
>     arm_smccc_1_1_smc(__VA_ARGS__);
> else
>     arm_smccc_1_0_smc(_VA_ARGS__);
> 
> In arm_smccc_1_1_smc(), we will explicitly initialize __res:
> 
> if ( ___res )
>    *___res = (typeof(*___res)) {r0, r1, r2, r3};
> 
> 
> Whereas for arm_smccc_1_0_smc(), we would call assembly function. I 
> assuming this is the problem?
> 
> I think this is similar to the discussion we had on set_interrupts() and 
> dt_set_cells(). If so, couldn't we tell ECLAIR that 
> __arm_smccc_1_0_smc() will always initialize *res?
> 

This is slightly different because of the chained variadic macro 
expansions of arm_smccc_smc. I could have stated that arm_smccc_smc 
initializes its args, but because it's variadic I can't narrow it down 
to a specific index, therefore the property is not correct, because the 
input arguments are instead expected to be read by the macro. The same 
reasoning applies for all variadic macros that have some input and 
output parameters, not just this one.

In the end, if these were fixed-argument functions or macros we can aim 
for that, and that would obsolete this patch.

Regards,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Hi,

On 21/07/2023 08:28, Nicola Vetrini wrote:
> On 20/07/23 17:54, Julien Grall wrote:
>> Hi Nicola,
>>
>> On 20/07/2023 15:29, Nicola Vetrini wrote:
>>> The local variables with type 'struct arm_smccc_res' are initialized
>>> just after the declaration to avoid any possible read usage prior
>>> to any write usage, which would constitute a violation of
>>> MISRA C:2012 Rule 9.1.
>>>
>>> This is already prevented by suitable checks in the code,
>>> but the correctness of this approach is difficult to prove and
>>> reason about.
>>
>> So I looked at the implementation of arm_smccc_smc(). For arm64, it is 
>> (simplified):
>>
>> if ( cpus_have_const_cap(ARM_SMCCC_1_1) )
>>     arm_smccc_1_1_smc(__VA_ARGS__);
>> else
>>     arm_smccc_1_0_smc(_VA_ARGS__);
>>
>> In arm_smccc_1_1_smc(), we will explicitly initialize __res:
>>
>> if ( ___res )
>>    *___res = (typeof(*___res)) {r0, r1, r2, r3};
>>
>>
>> Whereas for arm_smccc_1_0_smc(), we would call assembly function. I 
>> assuming this is the problem?
>>
>> I think this is similar to the discussion we had on set_interrupts() 
>> and dt_set_cells(). If so, couldn't we tell ECLAIR that 
>> __arm_smccc_1_0_smc() will always initialize *res?
>>
> 
> This is slightly different because of the chained variadic macro 
> expansions of arm_smccc_smc. I could have stated that arm_smccc_smc 
> initializes its args, but because it's variadic I can't narrow it down 
> to a specific index, therefore the property is not correct, because the 
> input arguments are instead expected to be read by the macro. The same 
> reasoning applies for all variadic macros that have some input and 
> output parameters, not just this one.
> 
> In the end, if these were fixed-argument functions or macros we can aim 
> for that, and that would obsolete this patch.

They are all ending up to call a fixed-argument macro 
(__arm_smccc_1_0_smc_7()) and then function __arm_smccc_1_0_smc().

Are you suggesting that this would still not be enough for Eclair?

Cheers,

-- 
Julien Grall