[bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()

[bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()

[Dan Carpenter]

I'm not sure how scmi_api->write_payload() is supposed to work and
I'm getting memory corruption in scmi_pin_control_list_associations_handler().

module/scmi_pin_control/src/mod_scmi_pin_control.c
   309  static int scmi_pin_control_list_associations_handler(
   310      fwk_id_t service_id,
   311      const uint32_t *payload)
   312  {
   313      const struct scmi_pin_control_list_associations_a2p *parameters;
   314      uint32_t payload_size;
   315      uint16_t identifiers_count;
   316      uint16_t total_number_of_associations;
   317      uint16_t remaining_associations;
   318      uint16_t identifier_index;
   319      size_t buffer_allowed_identifiers;
   320      uint16_t mapped_identifier;
   321  
   322      int status;
   323  
   324      struct scmi_pin_control_list_associations_p2a return_values = {
   325          .status = (int32_t)SCMI_GENERIC_ERROR,
   326          .flags = 0,
   327      };
   328  
   329      payload_size = (uint32_t)sizeof(return_values);
   330  
   331      parameters = (const struct scmi_pin_control_list_associations_a2p *)payload;
   332  
   333      status = map_identifier(parameters->identifier, &mapped_identifier);
   334  
   335      if (status != FWK_SUCCESS) {
   336          return_values.status = SCMI_NOT_FOUND;
   337          goto exit;
   338      }
   339  
   340      status = scmi_pin_control_ctx.pinctrl_api->get_total_number_of_associations(
   341          mapped_identifier, parameters->flags, &total_number_of_associations);
   342      if (status != FWK_SUCCESS) {
   343          return_values.status = SCMI_NOT_FOUND;
   344          goto exit;
   345      }
   346  
   347      if (parameters->index >= total_number_of_associations) {
   348          return_values.status = SCMI_NOT_FOUND;
   349          goto exit;
   350      }
   351  
   352      remaining_associations = total_number_of_associations - parameters->index;
   353  
   354      status = number_of_elements_allowed_in_payload(
   355          service_id,
   356          sizeof(struct scmi_pin_control_list_associations_p2a),
   357          sizeof(uint16_t),
   358          (size_t *)&buffer_allowed_identifiers);
   359      if (status != FWK_SUCCESS) {
   360          goto exit;
   361      }
   362  
   363      identifiers_count = (uint16_t)FWK_MIN(
   364          buffer_allowed_identifiers, remaining_associations);
   365  
   366      return_values.flags = identifiers_count;
   367      return_values.flags |= SHIFT_LEFT_BY_POS(
   368          (remaining_associations - identifiers_count),
   369          NUM_OF_REMAINING_ELEMENTS_POS);
   370  
   371      for (identifier_index = 0; identifier_index < identifiers_count;
   372           identifier_index++) {
   373          uint16_t object_id;
   374  
   375          status = scmi_pin_control_ctx.pinctrl_api->get_list_associations(
   376              mapped_identifier,
   377              parameters->flags,
   378              (parameters->index + identifier_index),
   379              &object_id);
   380          if (status != FWK_SUCCESS) {
   381              return_values.status = SCMI_NOT_FOUND;
   382              goto exit;
   383          }
   384  
   385          status = scmi_pin_control_ctx.scmi_api->write_payload(
   386              service_id, payload_size, &object_id, sizeof(object_id));

This code writes the u16 object id to the correct place.  It happens
in transport_write_payload()

   387          if (status != FWK_SUCCESS) {
   388              goto exit;
   389          }
   390  
   391          payload_size += (uint32_t)sizeof(object_id);
   392      }
   393  
   394      return_values.status = (int32_t)SCMI_SUCCESS;
   395  
   396  exit:
   397      return scmi_pin_control_ctx.scmi_api->respond(
   398          service_id,
   399          (return_values.status == SCMI_SUCCESS) ? (void *)&return_values :
   400                                                   (void *)&return_values.status,
   401          (return_values.status == SCMI_SUCCESS) ? payload_size :

But this payload_size will overwrite it again with data from beyond
the end of the return_values struct.  I don't really understand how
write payload is supposed to work.  Maybe we need to create a different
type of ->done() function pointer so that we could call ->write_payload()
in a loop and then call ->done() to actually send the data?

For now, I've just removed the ->write_payload() and done send the
response using ->respond() with an allocated buffer and that solves
my issues.

regards,
dan carpenter

   402                                                   sizeof(return_values.status));
   403  }

Re: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()

[Khaled Ali Ahmed]

Hi Dan,

write_payload API is a helpful function and can lead to easier and more effective implementation, while we ask the Transport to set an object into a certain offset in a service_id buffer, the main reason for that is that we don't know if we will continue the loop successfully or not.
after that, we should call respond which will trigger the Transport that the buffer now is completed and please respond whenever you are ready.

regarding the previous description, I encourage you to use write_payload whenever you can.

thanks, Dan so much


________________________________________
From: Dan Carpenter <dan.carpenter@linaro.org>
Sent: Wednesday, January 15, 2025 10:23 AM
To: Khaled Ali Ahmed <Khaled.AliAhmed@arm.com>
Cc: arm-scmi@vger.kernel.org <arm-scmi@vger.kernel.org>
Subject: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()

I'm not sure how scmi_api->write_payload() is supposed to work and
I'm getting memory corruption in scmi_pin_control_list_associations_handler().

module/scmi_pin_control/src/mod_scmi_pin_control.c
   309  static int scmi_pin_control_list_associations_handler(
   310      fwk_id_t service_id,
   311      const uint32_t *payload)
   312  {
   313      const struct scmi_pin_control_list_associations_a2p *parameters;
   314      uint32_t payload_size;
   315      uint16_t identifiers_count;
   316      uint16_t total_number_of_associations;
   317      uint16_t remaining_associations;
   318      uint16_t identifier_index;
   319      size_t buffer_allowed_identifiers;
   320      uint16_t mapped_identifier;
   321
   322      int status;
   323
   324      struct scmi_pin_control_list_associations_p2a return_values = {
   325          .status = (int32_t)SCMI_GENERIC_ERROR,
   326          .flags = 0,
   327      };
   328
   329      payload_size = (uint32_t)sizeof(return_values);
   330
   331      parameters = (const struct scmi_pin_control_list_associations_a2p *)payload;
   332
   333      status = map_identifier(parameters->identifier, &mapped_identifier);
   334
   335      if (status != FWK_SUCCESS) {
   336          return_values.status = SCMI_NOT_FOUND;
   337          goto exit;
   338      }
   339
   340      status = scmi_pin_control_ctx.pinctrl_api->get_total_number_of_associations(
   341          mapped_identifier, parameters->flags, &total_number_of_associations);
   342      if (status != FWK_SUCCESS) {
   343          return_values.status = SCMI_NOT_FOUND;
   344          goto exit;
   345      }
   346
   347      if (parameters->index >= total_number_of_associations) {
   348          return_values.status = SCMI_NOT_FOUND;
   349          goto exit;
   350      }
   351
   352      remaining_associations = total_number_of_associations - parameters->index;
   353
   354      status = number_of_elements_allowed_in_payload(
   355          service_id,
   356          sizeof(struct scmi_pin_control_list_associations_p2a),
   357          sizeof(uint16_t),
   358          (size_t *)&buffer_allowed_identifiers);
   359      if (status != FWK_SUCCESS) {
   360          goto exit;
   361      }
   362
   363      identifiers_count = (uint16_t)FWK_MIN(
   364          buffer_allowed_identifiers, remaining_associations);
   365
   366      return_values.flags = identifiers_count;
   367      return_values.flags |= SHIFT_LEFT_BY_POS(
   368          (remaining_associations - identifiers_count),
   369          NUM_OF_REMAINING_ELEMENTS_POS);
   370
   371      for (identifier_index = 0; identifier_index < identifiers_count;
   372           identifier_index++) {
   373          uint16_t object_id;
   374
   375          status = scmi_pin_control_ctx.pinctrl_api->get_list_associations(
   376              mapped_identifier,
   377              parameters->flags,
   378              (parameters->index + identifier_index),
   379              &object_id);
   380          if (status != FWK_SUCCESS) {
   381              return_values.status = SCMI_NOT_FOUND;
   382              goto exit;
   383          }
   384
   385          status = scmi_pin_control_ctx.scmi_api->write_payload(
   386              service_id, payload_size, &object_id, sizeof(object_id));

This code writes the u16 object id to the correct place.  It happens
in transport_write_payload()

   387          if (status != FWK_SUCCESS) {
   388              goto exit;
   389          }
   390
   391          payload_size += (uint32_t)sizeof(object_id);
   392      }
   393
   394      return_values.status = (int32_t)SCMI_SUCCESS;
   395
   396  exit:
   397      return scmi_pin_control_ctx.scmi_api->respond(
   398          service_id,
   399          (return_values.status == SCMI_SUCCESS) ? (void *)&return_values :
   400                                                   (void *)&return_values.status,
   401          (return_values.status == SCMI_SUCCESS) ? payload_size :

But this payload_size will overwrite it again with data from beyond
the end of the return_values struct.  I don't really understand how
write payload is supposed to work.  Maybe we need to create a different
type of ->done() function pointer so that we could call ->write_payload()
in a loop and then call ->done() to actually send the data?

For now, I've just removed the ->write_payload() and done send the
response using ->respond() with an allocated buffer and that solves
my issues.

regards,
dan carpenter

   402                                                   sizeof(return_values.status));
   403  }


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.

Re: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()

[Dan Carpenter]

On Wed, Jan 15, 2025 at 11:15:25AM +0000, Khaled Ali Ahmed wrote:
> Hi Dan,
> 
> write_payload API is a helpful function and can lead to easier and
> more effective implementation, while we ask the Transport to set an
> object into a certain offset in a service_id buffer, the main reason
> for that is that we don't know if we will continue the loop
> successfully or not.
>
> after that, we should call respond which will trigger the Transport
> that the buffer now is completed and please respond whenever you are
> ready.
> 
> regarding the previous description, I encourage you to use write_payload
> whenever you can.
> 

I get how it could be nice, but it doesn't work.  I am looking
at transport_write_payload() in particular.

>    385          status = scmi_pin_control_ctx.scmi_api->write_payload(
>    386              service_id, payload_size, &object_id, sizeof(object_id));

In this code we're writing sizeof(object_id) bytes to the middle of the
buffer.  Nothing in transport_write_payload() is saving the "size" so we
don't record which bytes have been written to.

>    397      return scmi_pin_control_ctx.scmi_api->respond(
>    398          service_id,
>    399          (return_values.status == SCMI_SUCCESS) ? (void *)&return_values :
>    400                                                   (void *)&return_values.status,
>    401          (return_values.status == SCMI_SUCCESS) ? payload_size :
>    402                                                   sizeof(return_values.status));

Ideally this code would write sizeof(return_values) bytes to the start
of the buffer and do the transfer.  But we're not passing
sizeof(return_values) as a parameter.

Instead, in this code we're passing "payload_size", which is the total
number of bytes.  So it does the equivalent to
memcpy(channel_ctx->out->buffer, &return_values, payload_size).  That is
a read overflow of the return_values struct and it scribbles over the
data that we copied earlier with the transport_write_payload().

regards,
dan carpenter

Re: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()

[Khaled Ali Ahmed]

Hello Dan,

the name of the variable may be tricky. I agree its current name is payload_size but it would be better to change its name into payload_offset or something else.
the write_payload function using  fwk_str_memcpy which is implementing memory copy. it makes a copy of the payload into payload address + offset.

> Ideally this code would write sizeof(return_values) bytes to the start
> of the buffer and do the transfer.  But we're not passing
> sizeof(return_values) as a parameter.
this means that the actual response size will depend on the status of the response if it is successful we will send the whole buffer, otherwise we will send just the error code as a return value.

I am not sure if it is clear for you otherwise let's plan a meeting and let me clarify more.

regards,
khaled Ahmed

________________________________________
From: Dan Carpenter <dan.carpenter@linaro.org>
Sent: Wednesday, January 15, 2025 12:14 PM
To: Khaled Ali Ahmed <Khaled.AliAhmed@arm.com>
Cc: arm-scmi@vger.kernel.org <arm-scmi@vger.kernel.org>
Subject: Re: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()

On Wed, Jan 15, 2025 at 11:15:25AM +0000, Khaled Ali Ahmed wrote:
> Hi Dan,
>
> write_payload API is a helpful function and can lead to easier and
> more effective implementation, while we ask the Transport to set an
> object into a certain offset in a service_id buffer, the main reason
> for that is that we don't know if we will continue the loop
> successfully or not.
>
> after that, we should call respond which will trigger the Transport
> that the buffer now is completed and please respond whenever you are
> ready.
>
> regarding the previous description, I encourage you to use write_payload
> whenever you can.
>

I get how it could be nice, but it doesn't work.  I am looking
at transport_write_payload() in particular.

>    385          status = scmi_pin_control_ctx.scmi_api->write_payload(
>    386              service_id, payload_size, &object_id, sizeof(object_id));

In this code we're writing sizeof(object_id) bytes to the middle of the
buffer.  Nothing in transport_write_payload() is saving the "size" so we
don't record which bytes have been written to.

>    397      return scmi_pin_control_ctx.scmi_api->respond(
>    398          service_id,
>    399          (return_values.status == SCMI_SUCCESS) ? (void *)&return_values :
>    400                                                   (void *)&return_values.status,
>    401          (return_values.status == SCMI_SUCCESS) ? payload_size :
>    402                                                   sizeof(return_values.status));

Ideally this code would write sizeof(return_values) bytes to the start
of the buffer and do the transfer.  But we're not passing
sizeof(return_values) as a parameter.

Instead, in this code we're passing "payload_size", which is the total
number of bytes.  So it does the equivalent to
memcpy(channel_ctx->out->buffer, &return_values, payload_size).  That is
a read overflow of the return_values struct and it scribbles over the
data that we copied earlier with the transport_write_payload().

regards,
dan carpenter
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.