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

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

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  }