OP-TEE: memory corruption in scmi_pin_control_list_associations_handler()

[Dan Carpenter <dan.carpenter@linaro.org>]

When we do SCMI on OP-TEE, the in buffer and out buffer are the same buffer.
I added some debug code to confirm this:

diff --git a/module/msg_smt/src/mod_msg_smt.c b/module/msg_smt/src/mod_msg_smt.c
index 853fe66b8d27..1e92dcd49c29 100644
--- a/module/msg_smt/src/mod_msg_smt.c
+++ b/module/msg_smt/src/mod_msg_smt.c
@@ -175,6 +175,11 @@ static int smt_write_payload(fwk_id_t channel_id,
     if (!channel_ctx->locked)
         return FWK_E_ACCESS;
 
+    FWK_LOG_ERR("OUT=%p IN=%p %s",
+               channel_ctx->out->payload,
+               channel_ctx->in->payload,
+               (channel_ctx->out->payload == channel_ctx->in->payload) ? "equal" : "different");
+
     memcpy(((uint8_t*)channel_ctx->out->payload) + offset, payload, size);
 
     return FWK_SUCCESS;

And it's true:

[    0.000000] OUT=0x9c401004 IN=0x9c401004 equal

This normally isn't a problem because we read a few inputs at the
start of the function and then write out the result to the output at the
end.

But in the scmi_pin_control_list_associations_handler() it does a loop
where each iteration reads from parameters->index which is input and
writes to the output with scmi_pin_control_ctx.scmi_api->write_payload()
and that corrupts the input data.

Copying the input buffer to a the stack the issue for me, but I feel
like it is a hack.  It would be better to use separate buffers for
input and output.

I think this comes from:
https://github.com/OP-TEE/optee_os/blob/master/core/kernel/pseudo_ta.c#L93
I added a print statement to there as well:

diff --git a/core/kernel/pseudo_ta.c b/core/kernel/pseudo_ta.c
index 587faa41a770..426870fb934c 100644
--- a/core/kernel/pseudo_ta.c
+++ b/core/kernel/pseudo_ta.c
@@ -90,6 +90,7 @@ static TEE_Result copy_in_param(struct ts_session *s __maybe_unused,
                                va = NULL;
                        }
 
+                       EMSG("n=%lu va=%p", n, va);
                        tee_param[n].memref.buffer = va;
                        tee_param[n].memref.size = mem->size;
                        break;

E/TC:? 0 copy_in_param:93 n=1 va=0x9c401000
E/TC:? 0 copy_in_param:93 n=2 va=0x9c401000

Here is the patch to save "parameters" to a different buffer.

---
 .../scmi_pin_control/src/mod_scmi_pin_control.c   | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/module/scmi_pin_control/src/mod_scmi_pin_control.c b/module/scmi_pin_control/src/mod_scmi_pin_control.c
index a0b90dd2b73f..54e613b70f69 100644
--- a/module/scmi_pin_control/src/mod_scmi_pin_control.c
+++ b/module/scmi_pin_control/src/mod_scmi_pin_control.c
@@ -344,7 +344,7 @@ static int scmi_pin_control_list_associations_handler(
     fwk_id_t service_id,
     const uint32_t *payload)
 {
-    const struct scmi_pin_control_list_associations_a2p *parameters;
+    const struct scmi_pin_control_list_associations_a2p parameters;
     uint32_t payload_size;
     uint16_t identifiers_count;
     uint16_t total_number_of_associations;
@@ -362,8 +362,9 @@ static int scmi_pin_control_list_associations_handler(
     payload_size = (uint32_t)sizeof(return_values);
 
     parameters = (const struct scmi_pin_control_list_associations_a2p *)payload;
+    memcpy(&parameters, payload, sizeof(parameters));
 
-    status = map_identifier(parameters->identifier, &mapped_identifier);
+    status = map_identifier(parameters.identifier, &mapped_identifier);
 
     if (status != FWK_SUCCESS) {
         return_values.status = SCMI_NOT_FOUND;
@@ -371,7 +372,7 @@ static int scmi_pin_control_list_associations_handler(
     }
 
     status = scmi_pin_control_ctx.pinctrl_api->get_total_number_of_associations(
-        mapped_identifier, parameters->flags, &total_number_of_associations);
+        mapped_identifier, parameters.flags, &total_number_of_associations);
     if (status != FWK_SUCCESS) {
         return_values.status = SCMI_NOT_FOUND;
         goto exit;
@@ -388,11 +389,11 @@ static int scmi_pin_control_list_associations_handler(
 
     identifiers_count = (uint16_t)FWK_MIN(
         buffer_allowed_identifiers,
-        (uint16_t)(total_number_of_associations - parameters->index));
+        (uint16_t)(total_number_of_associations - parameters.index));
 
     return_values.flags = identifiers_count;
     return_values.flags |= SHIFT_LEFT_BY_POS(
-        (total_number_of_associations - parameters->index - identifiers_count),
+        (total_number_of_associations - parameters.index - identifiers_count),
         NUM_OF_REMAINING_ELEMENTS_POS);
 
     for (identifier_index = 0; identifier_index < identifiers_count;
@@ -401,8 +402,8 @@ static int scmi_pin_control_list_associations_handler(
 
         status = scmi_pin_control_ctx.pinctrl_api->get_list_associations(
             mapped_identifier,
-            parameters->flags,
-            (parameters->index + identifier_index),
+            parameters.flags,
+            (parameters.index + identifier_index),
             &object_id);
         if (status != FWK_SUCCESS) {
             return_values.status = SCMI_NOT_FOUND;
-- 
2.51.0