Session ID from MCTP transport met 32 bit alignment access issue

[LeeTroy]

Hi,

libspdm/library/spdm_transport_mctp_lib/libspdm_mctp_mctp.c

Lines 135 to 136 in acf5e97

	*session_id = (uint32_t *)((uint8_t *)transport_message +
	sizeof(mctp_message_header_t));

Sometimes I got garbage session_id value from mctp decode, it seems to be a 32-bit alignment issue the following code segment. The size of mctp_header_t is 1, so the pointer to session_id might be set to 4n+1. My platform is ARM Cortex-M4 running Zephyr (requester+responder) and ARM Cortex-A7 running OpenBMC (responder), and both side met this issue. Do I need to add compiler options for resolving this?

I've made an experiement, and the garbage session_id is gone.

#if 1
uint8_t *buf = malloc(4);
*session_id = buf;
buf[0] = *((uint8_t *)transport_message + sizeof(mctp_message_header_t));
buf[1] = *((uint8_t *)transport_message + sizeof(mctp_message_header_t)+1);
buf[2] = *((uint8_t *)transport_message + sizeof(mctp_message_header_t)+2);
buf[3] = *((uint8_t *)transport_message + sizeof(mctp_message_header_t)+3);
#else
*session_id = (uint32_t *)((uint8_t *)transport_message +
                           sizeof(mctp_message_header_t));
#endif

Thanks,
Troy Lee

[jyao1]

@LeeTroy , I feel your work-around changed code logic.

Originally, the session_id is a pointer to transport message.
With your workaround, the session_id is a pointer to the heap.

Is there any possibility that the transport_message is changed?
To narrow down the issue, could you please add some printf() here for session ID pointer, as well as the session ID data? to prove it is garbage or not?

[LeeTroy]

It might depends on the implementation of acquire_receiver_buffer and release_receiver_buffer. In my design, I malloc() an memory in acquire_receiver_buffer then free() in release_receiver_buffer.

libspdm/library/spdm_responder_lib/libspdm_rsp_communication.c

Lines 49 to 65 in acf5e97

	status = libspdm_process_request(spdm_context, &session_id, &is_app_message,
	request_size, request);
	libspdm_release_receiver_buffer (spdm_context);
	if (LIBSPDM_STATUS_IS_ERROR(status)) {
	return status;
	}
	/* save the value of session_id */
	if(session_id != NULL) {
	tmp_session_id = *session_id;
	session_id_ptr = &tmp_session_id;
	} else {
	session_id_ptr = NULL;
	}
	/* build and send response message */
	libspdm_acquire_sender_buffer (spdm_context, &message_size, (void **)&message);

In L51 frees the receiver buffer, but the session_id is a pointer to the receiver buffer(transport message buffer).

libspdm_responder_dispatch_message:39 session_id[(nil)] = 00000000
Acquire msg_buf_ptr=0x945918.                                            <--- Allocate the receive buffer at 0x945918
ast_spdm_device_receive_message(0x44969c, 0x93a198, 4672, 0x945918)
Slave READ r=5 errno=2
RECV message length:61
Slave READ r=62 errno=2
RECV DATA [0000003d]:
18 59 94 00 3d 00 00 00 1a 59 94 00 35 00 00 00 18 59 94 00 40 12 00 00 00 00 00 00 19 59 94 00
34 00 00 00 00 00 00 00 00 00 00 00 00 7a 7a e5 00 00 00 00 00 00 00 00 00 00 00 00 00
libspdm_responder_dispatch_message:46 session_id[(nil)] = 00000000
SpdmReceiveRequest[.] ...
transport_message buffer:
06 ff ff ff ff 00 00 34 00 1a 64 b1 35 af b7 9a f7 64 cf 86 a8 83 ca 82 62 3c f2 e7 75 5e c7 1b
9e e2 3d 7b 3a 77 ad 5d 93 52 4e 7b a2 f2 9a 5f 92 9c 6b d6 7d ec 25 f5 5e 13 0a f0 a4
Original session_id pointer=0x945919 value=ffffffff
libspdm_get_session_info_via_session_id:165 Expect session_id=ffffffff session_info[0].session_id=ffffffff
libspdm_get_session_info_via_session_id:165 Expect session_id=ffffffff session_info[0].session_id=ffffffff
SpdmReceiveRequest[ffffffff] (0x4):
0000: 12 e8 00 00
libspdm_responder_dispatch_message:53 session_id[0x945919] = ffffffff.   <--- Valid accessing the receive buffer at 0x945919
Release msg_buf_ptr=0x945918.                                            <--- Release the receive buffer at 0x945918
libspdm_responder_dispatch_message:59 session_id[0x945919] = 280094c6    <--- Invalid accessing the receive buffer at 0x945919
libspdm_responder_dispatch_message:68 session_id[0x945919] = 280094c6
libspdm_responder_dispatch_message:75 session_id[0x945919] = 00000000
libspdm_build_response:335 session_id[0x7eff5c04] = 280094c6
libspdm_build_response:350 session_id[0x7eff5c04] = 280094c6
libspdm_build_response:415 session_id[0x7eff5c04] = 280094c6
libspdm_get_session_info_via_session_id:165 Expect session_id=280094c6 session_info[0].session_id=ffffffff
libspdm_get_session_info_via_session_id:165 Expect session_id=280094c6 session_info[1].session_id=00000000
libspdm_get_session_info_via_session_id:165 Expect session_id=280094c6 session_info[2].session_id=00000000
libspdm_get_session_info_via_session_id:165 Expect session_id=280094c6 session_info[3].session_id=00000000
libspdm_get_session_info_via_session_id - not found session_id

Should I use static array like spdm-emu for sender/receiver buffer instead of dynamically allocated?

[LeeTroy]

Another approach is moving the release receiver buffer call after the response message is constructed.

[LeeTroy]

I've submitted a PR #869 for this.

[jyao1]

Thanks. Please use

 status = libspdm_process_request(spdm_context, &session_id, &is_app_message, 
                                  request_size, request); 
 if (LIBSPDM_STATUS_IS_ERROR(status)) { 
     libspdm_release_receiver_buffer (spdm_context); 
     return status; 
 } 
  
 /* save the value of session_id */ 
 if(session_id != NULL) { 
     tmp_session_id = *session_id; 
     session_id_ptr = &tmp_session_id; 
 } else { 
     session_id_ptr = NULL; 
 } 

 /* release buffer after use session_id, before acquire buffer */
 libspdm_release_receiver_buffer (spdm_context); 
  
 /* build and send response message */ 
 libspdm_acquire_sender_buffer (spdm_context, &message_size, (void **)&message); 

[LeeTroy]

Got it.