* [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()
@ 2025-01-15 10:23 Dan Carpenter
2025-01-15 11:18 ` Khaled Ali Ahmed
[not found] ` <GV2PR08MB795595222CD06A1F5B60CD0FEB192@GV2PR08MB7955.eurprd08.prod.outlook.com>
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2025-01-15 10:23 UTC (permalink / raw)
To: Khaled Ali Ahmed; +Cc: arm-scmi@vger.kernel.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 }
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()
2025-01-15 10:23 [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler() Dan Carpenter
@ 2025-01-15 11:18 ` Khaled Ali Ahmed
[not found] ` <GV2PR08MB795595222CD06A1F5B60CD0FEB192@GV2PR08MB7955.eurprd08.prod.outlook.com>
1 sibling, 0 replies; 4+ messages in thread
From: Khaled Ali Ahmed @ 2025-01-15 11:18 UTC (permalink / raw)
To: Dan Carpenter; +Cc: arm-scmi@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 4+ messages in thread[parent not found: <GV2PR08MB795595222CD06A1F5B60CD0FEB192@GV2PR08MB7955.eurprd08.prod.outlook.com>]
* Re: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()
[not found] ` <GV2PR08MB795595222CD06A1F5B60CD0FEB192@GV2PR08MB7955.eurprd08.prod.outlook.com>
@ 2025-01-15 12:14 ` Dan Carpenter
2025-01-15 12:31 ` Khaled Ali Ahmed
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-01-15 12:14 UTC (permalink / raw)
To: Khaled Ali Ahmed; +Cc: arm-scmi@vger.kernel.org
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler()
2025-01-15 12:14 ` Dan Carpenter
@ 2025-01-15 12:31 ` Khaled Ali Ahmed
0 siblings, 0 replies; 4+ messages in thread
From: Khaled Ali Ahmed @ 2025-01-15 12:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: arm-scmi@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-15 12:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-15 10:23 [bug report] SCP: memory corruption in scmi_pin_control_list_associations_handler() Dan Carpenter
2025-01-15 11:18 ` Khaled Ali Ahmed
[not found] ` <GV2PR08MB795595222CD06A1F5B60CD0FEB192@GV2PR08MB7955.eurprd08.prod.outlook.com>
2025-01-15 12:14 ` Dan Carpenter
2025-01-15 12:31 ` Khaled Ali Ahmed
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.