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(¶meters, 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;
Hi Dan, Thank you for sharing the proposal. We are happy to accept this solution once it has been pushed to a public repository so we can review and track it properly. One point to highlight as we move forward: we always ensure that separate buffers are used for input and output to avoid any potential side effects or data corruption. Thanks again for the contribution, and we look forward to reviewing the implementation. Best regards, Khaled Ali Ahmed
Sent from Outlook for Mac
From: Dan Carpenter dan.carpenter@linaro.org Date: Friday, 30 January 2026 at 11:26 To: Jens Wiklander jens.wiklander@linaro.org, Khaled Ali Ahmed Khaled.AliAhmed@arm.com, arm-scmi@vger.kernel.org arm-scmi@vger.kernel.org, op-tee@lists.trustedfirmware.org op-tee@lists.trustedfirmware.org Subject: OP-TEE: memory corruption in scmi_pin_control_list_associations_handler()
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(¶meters, 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
op-tee@lists.trustedfirmware.org
-
Dan Carpenter -
Khaled Ali Ahmed