Safer memory functions

[richkong88]

My team has requirements for using safer memory functions (Eg memcpy_s, memset_s from annexk) where the destination buffer size is specified. I would like to put in a compile time configuration switch to allow for this support (adding a copy_mem_s function with a dest size parameter).

I noticed the current copy_mem function allows for overlapped buffers and reentrant code. Are these two items really necessary requirements for libspdm? Perhaps they were a carry over from source code in a different project?

[steven-bellock]

I noticed the current copy_mem function allows for overlapped buffers and reentrant code. Are these two items really necessary requirements for libspdm? Perhaps they were a carry over from source code in a different project?

This was on my TODO list. There are multiple issues.

1. memlib.h functions need the libspdm_ suffix.
2. copy_mem should correspond to memcpy where it should not be called if the buffers overlap. copy_mem could check that they don't overlap and return an error if they do.
3. If libspdm does need to support overlapping buffers then it should have a move_mem function similar to memmove.
4. The os_stub/memlib implementations don't conform to the specifications given in memlib.h. In particular they don't handle the case where source_buffer overlaps destination_buffer. and don't check that If length is greater than (MAX_ADDRESS - destination_buffer + 1), then ASSERT().

[richkong88]

Ok. I put this here in case it helps you. Sorry for the formatting. Not sure why the insert code isn't working right.

/**
  Copies bytes from a source buffer to a destination buffer.

  This function copies "len" bytes from "src" to "dst".

  Asserts and returns a non-zero value if any of the following are true:
    1) ("src" or "dst" are NULL) and "len" is greater 0.
    2) "src" and "dst" overlap.
    3) "len" is greater than "dst_len".
    4) "len" is greater than (MAX_ADDRESS - "dst" + 1).
    5) "len" is greater than (MAX_ADDRESS - "src" + 1).
    6) "len" is greater than (SIZE_MAX >> 1).

  @param    dst       Destination buffer to copy to.
  @param    dst_len   Maximum length in bytes of the destination buffer.
  @param    src       Source buffer to copy from.
  @param    len       The number of bytes to copy.

  @return   0 on success. non-zero on error.
**/

int copy_mem_s(OUT void* dst_buf, IN uintn dst_len, IN const void* src_buf, IN uintn len)
{
    uint8_t* dst;
    uint8_t* src;

    dst = (uint8_t*) dst_buf;
    src = (uint8_t*) src_buf;

    if ((src == NULL || dst == NULL) && len > 0) {
        ASSERT(0);
        return -1;
    }

    if ((src < dst && src + len > dst)
     || (dst < src && dst + len > src)) {
        ASSERT(0);
        return -1;
    }

    if (len > dst_len
     || len > MAX_ADDRESS - (uintn)dst + 1
     || len > MAX_ADDRESS - (uintn)src + 1
     || len > (SIZE_MAX >> 1)) {

        ASSERT(0);
        return -1;
    }

    while (len-- != 0) {
        *(dst++) = *(src++);
    }

    return 0;
}

void* copy_mem(OUT void* destination_buffer, IN const void* source_buffer, IN uintn length)
{
    copy_mem_s(destination_buffer, length, source_buffer, length);
    return destination_buffer;
}

[steven-bellock]

@richkong88 You need to use three back ticks for multiline code. I edited it for you.

[jyao1]

REF: https://stackoverflow.com/questions/870019/memcpy-in-secure-programming

There is debate on the usefulness of copy_mem_s(). The example below seems a typical abuse.

void* copy_mem(OUT void* destination_buffer, IN const void* source_buffer, IN uintn length)
{
    copy_mem_s(destination_buffer, length, source_buffer, length);
    return destination_buffer;
}

I guess most code would just put same dst_len and len. I am not sure how that is useful by replacing it with *_s().

Anyway, I suggest we either keep current copy_mem() or replace it with copy_mem_s() completely. Having both copy_mem() and copy_mem_s() is not useless.

[jyao1]

@richkong88, could you please work on it?

[steven-bellock]

Since the API for memcpy_s uses the restrict keyword do we want to start supporting the restrict keyword for C99 and greater compilers? We can always turn it off for C89 compilers, for example

#ifdef C89
#define RESTRICT
#else
#define RESTRICT restrict
#endif

[richkong88]

The restrict is a potential optimization, correct? why-does-the-restrict-qualifier-still-allow-memcpy-to-access-overlapping-memory. I suppose we could add it, but I don't know that it'll do much for speed, as we're using volatile pointers within the function. (And that is another question of mine.)

[jyao1]

Comment on mem_copy() and mem_move():

The original mem_copy() API definition is from UEFI specification, where the overlap should be handled. The implementation does not follows the definition. The fix could be:

if (source_buffer > destination_buffer) {
      // copy from head
      pointer_dst = (uint8_t *)destination_buffer;
      pointer_src = (uint8_t *)source_buffer;
      while (length-- != 0) {
        *(pointer_dst++) = *(pointer_src++);
      }
} else { // source_buffer <= destination_buffer
      // copy from tail
      pointer_dst = (uint8_t *)destination_buffer + length;
      pointer_src = (uint8_t *)source_buffer + length;
      while (length-- != 0) {
        *(--pointer_dst) = *(--pointer_src);
      }
}

The use case I could think is to move a memory forward or afterword inside of a buffer. E.g. to adjust an offset.

Since current mem_cpy() does not handle it and we can run code correctly, I dont think it is needed now. We can revisit later, if we see such use case.

[richkong88]

@jyao1 @steven-bellock I started moving the uses of copy_mem to copy_mem_s and found that there are over 800 uses of copy_mem with most of them (650+) in the unit_tests. I'm not real keen on moving all of the unit tests ones over and am wondering it is ok to leave them as copy_mem and only change the ones in include, library, and os_stub to copy_mem_s. I would then only define copy_mem_s in the include, library, and os_stub source and define copy_mem in some unit_test framework file. Thoughts?

[jyao1]

I think it should be all-or-none action. Half migration is not my preference.

If we want to introduce the new version, we should always use the new version copy_mem.

Otherwise, people need to think which version be used.

[steven-bellock]

Ideally the tests should not be leveraging functions from the implementation they are testing, since it can mask a bug in the implementation. In https://github.com/DMTF/libspdm/issues/388 I asked if memlib functions need to be exposed to the integrator (or test) via requester(responder)_lib.h and at the time we decided to leave them exposed for now. But this might be a good time to

1. Transition the tests to using something independent of libspdm, like memcpy.
2. Hide memlib functions from the integrator.

[jyao1]

I think it is unrelated topic to memlib or not. The question is: if we want to use unsafe version memcpy (or copy_mem) or safe version memcpy_s (or copy_mem_s).

My preference is: dont mix them. If we decide to use safe version memcpy_s (or copy_mem_s), we should use it in our project. No exception.

[steven-bellock]

• In case of error, the "dst_buf" is left unmodifed.
• This behavior is different than C11 memcopy_s, which zeros the "dst_buf"
• if there is a runtime violation.

So does that mean the integrator can't implement copy_mem_s via memcopy_s?

[richkong88]

@jyao1 Very well. I looked at the unit_test usages again and most of them are easy to change. I will proceed by changing the unit_tests first. @steven-bellock Agree. It would be bad if integrators could not use mem_copy_s. Will change the behavior to zero "dst_buf".

[richkong88]

For reference, these are the approximate number of usages of copy_mem per group. I will convert them over to copy_mem_s over multiple PRs of probably 100-200 copy_mem changes per PR. I will start with the unit_tests and work my way down the list. After finishing them all, I will change the name from copy_mem_s to copy_mem, and get rid of the older 3 parameter copy_mem code.

I will start this the week of Feb 14th.

Directory - # of copy_mem usages (Total ~ 800)

Unit Test - Fuzzing - 106 unit_test\fuzzing - 106

Unit Test - Various: (17) unit_test\spdm_transport_test_lib unit_test\test_crypt\ unit_test\test_size\

Unit Test - Requester - (485 - 10 files)

unit_test\test_spdm_requester\challenge.c: - 43 unit_test\test_spdm_requester\end_session.c: - 24 unit_test\test_spdm_requester\finish.c: - 53

unit_test\test_spdm_requester\get_certificate.c: - 21 unit_test\test_spdm_requester\get_measurements.c: - 80 unit_test\test_spdm_requester\heartbeat.c: - 24

unit_test\test_spdm_requester\key_exchange.c: - 106 unit_test\test_spdm_requester\key_update.c: - 62

unit_test\test_spdm_requester\psk_exchange.c: - 42 unit_test\test_spdm_requester\psk_finish.c: - 30

Unit Test - Responder (116 - 8 files)

unit_test\test_spdm_responder\end_session.c: - 7 unit_test\test_spdm_responder\finish.c: - 5 unit_test\test_spdm_responder\heartbeat.c: - 7 unit_test\test_spdm_responder\key_update.c: - 35 unit_test\test_spdm_responder\measurements.c: - 1 unit_test\test_spdm_responder\psk_exchange.c: - 14 unit_test\test_spdm_responder\psk_finish.c: - 15 unit_test\test_spdm_responder\respond_if_ready.c: - 32

OS_Stub - 52 os_stub\cryptlib_mbedtls\hmac\hmac_sha.c: os_stub\cryptlib_mbedtls\pem\pem.c: os_stub\cryptlib_mbedtls\pk\dh.c: os_stub\cryptlib_mbedtls\pk\x509.c: os_stub\cryptlib_mbedtls\rand\rand.c: os_stub\cryptlib_mbedtls\sys_call\timer_wrapper_host.c: os_stub\cryptlib_null\cipher\aead_aes_gcm.c: os_stub\cryptlib_openssl\pem\pem.c: os_stub\cryptlib_openssl\pk\sm2.c: os_stub\cryptlib_openssl\pk\x509.c: os_stub\memlib\copy_mem.c: os_stub\openssllib\rand_pool.c: os_stub\spdm_device_secret_lib_sample\cert.c: os_stub\spdm_device_secret_lib_sample\lib.c:

Library - Requester - 25 library\spdm_requester_lib\libspdm_req_challenge.c: library\spdm_requester_lib\libspdm_req_encap_certificate.c: library\spdm_requester_lib\libspdm_req_encap_challenge_auth.c: library\spdm_requester_lib\libspdm_req_encap_error.c: library\spdm_requester_lib\libspdm_req_get_certificate.c: library\spdm_requester_lib\libspdm_req_get_digests.c: library\spdm_requester_lib\libspdm_req_get_measurements.c: library\spdm_requester_lib\libspdm_req_get_version.c: library\spdm_requester_lib\libspdm_req_key_exchange.c: library\spdm_requester_lib\libspdm_req_psk_exchange.c:

Library - Responder - 13 library\spdm_responder_lib\libspdm_rsp_certificate.c: library\spdm_responder_lib\libspdm_rsp_challenge_auth.c: library\spdm_responder_lib\libspdm_rsp_encap_challenge.c: library\spdm_responder_lib\libspdm_rsp_encap_get_certificate.c: library\spdm_responder_lib\libspdm_rsp_encap_get_digests.c: library\spdm_responder_lib\libspdm_rsp_encap_key_update.c: library\spdm_responder_lib\libspdm_rsp_error.c: library\spdm_responder_lib\libspdm_rsp_handle_response_state.c: library\spdm_responder_lib\libspdm_rsp_key_update.c: library\spdm_responder_lib\libspdm_rsp_measurements.c: library\spdm_responder_lib\libspdm_rsp_version.c:

Library - Secured Message - 55 library\spdm_secured_message_lib\libspdm_secmes_context_data.c: library\spdm_secured_message_lib\libspdm_secmes_encode_decode.c: library\spdm_secured_message_lib\libspdm_secmes_key_exchange.c: library\spdm_secured_message_lib\libspdm_secmes_session.c:

Library - Various - 43 include\hal\library\memlib.h: library\spdm_common_lib\libspdm_com_context_data.c: library\spdm_common_lib\libspdm_com_crypto_service_session.c: library\spdm_common_lib\libspdm_com_crypto_service.c: library\spdm_common_lib\libspdm_com_opaque_data.c: library\spdm_common_lib\libspdm_com_support.c: library\spdm_crypt_lib\libspdm_crypt_crypt.c: library\spdm_transport_mctp_lib\libspdm_mctp_common.c: library\spdm_transport_mctp_lib\libspdm_mctp_mctp.c: library\spdm_transport_pcidoe_lib\libspdm_doe_pcidoe.c:

[steven-bellock]

@richkong88 Can you give an example of how dst_len will be used?

[richkong88]

Sure. In some cases, I will need to calculate what space is left in a destination buffer and use as "dst_len". I feel there are many cases in the library code as it typically fills a buffer. Take this example (I've removed irrelevant code).

return_status
spdm_build_opaque_data_supported_version_data(IN spdm_context_t *spdm_context,
                                              IN OUT uintn *data_out_size,
                                              OUT void *data_out)
{
    uintn final_data_size;
    secured_message_general_opaque_data_table_header_t    *general_opaque_data_table_header;
    secured_message_opaque_element_table_header_t    *opaque_element_table_header;
    secured_message_opaque_element_supported_version_t    *opaque_element_support_version;
    spdm_version_number_t   *versions_list;

    general_opaque_data_table_header = data_out;   
    opaque_element_table_header = (void *) (general_opaque_data_table_header + 1);
    opaque_element_support_version = (void *) (opaque_element_table_header + 1);

    versions_list = (void *)(opaque_element_support_version + 1);

   /* data_out is start of buffer, version_list is what we've used so far.
    * subtract the difference between the two from *data_out_size 
    * to get what room is left in the destination buffer */
    copy_mem_s(versions_list, *data_out_size - ((uintn)versions_list - (uintn)data_out),
               spdm_context->local_context.secured_message_version.spdm_version,
               spdm_context->local_context.secured_message_version.spdm_version_count *
               sizeof(spdm_version_number_t));

In other cases, it's pretty simple when the destination is an entire struct or some other fixed size variable. I feel many of the unit_test cases are like this.

static uintn m_local_buffer_size;
static uint8_t m_local_buffer[LIBSPDM_MAX_MESSAGE_SMALL_BUFFER_SIZE];
...
    copy_mem_s(&m_local_buffer[m_local_buffer_size], 
             sizeof(m_local_buffer),
             spdm_response,
             (uintn)ptr - (uintn)spdm_response);

    copy_mem_s(secured_message_context->application_secret.request_data_salt,
             sizeof(secured_message_context->application_secret.request_data_salt),
             ptr, secured_message_context->aead_iv_size);

[richkong88]

@jyao1 @steven-bellock All the usages of the 3 parameter copy_mem have been switched to 4 parameter copy_mem_s. Since there will only be one copy memory function (4 parameter version), I think it was desired to change the copy_mem_s function name now to copy_mem? Just want to confirm one more before changing the 800+ instances and removing the 3 parameter version.

[steven-bellock]

That's fine from my side. If someone wants the classic memcpy they can set dst_len == len.

[jyao1]

is this done ?

[richkong88]

Yes. Closing.