ibmtss

ibmtss copied to clipboard

This PR adds the following functions:

• TSS_Free, wrapper for free or similar function
• TSS_SetMemoryFunctions, allow the usage of platform specific malloc, realloc and free functions. Must be called before any other TSS function. If not called malloc, realloc and free will be used.

Replaced all calls to free with calls to TSS_Free.

Small comment: It would be nice to explain 'why' this is needed in the pull request. I can add that to the documentation.

Question 1: ifdef for Windows and Posix were added to the three calls. Why is this needed, since the code is the same in both cases?

Question 2: I suspect that calling TSS_SetMemoryFunctions() after a malloc and before a free would be unpredictable. Is there a use case for setting it more than once?

If not, perhaps the function should check for 'already set' and return an error.

If there is a use case for setting it more than once, then perhaps 'all three parameters NULL' should be legal, a way to set back to the default.

Another comment: There's no test case. I'd rather not upstream untested code.

I used the library in UEFI applications and there is no standard library (no malloc, realloc, free,...), but there are direct replacements for most of the standard functions (strlen, strcpy, memcpy,...). I solved this by using defines for most of the standard functions and provided function pointers for malloc, realloc and free.

I created this PR because others may also use the library in environments where the standard library is not available.

Answer 1: You are right that does not make sense, I am working on another PR and this was part of it.

Answer 2: You are correct if you change TSS_SetMemoryFunctions calls to free with pointers that were allocated using the previous malloc are undefined behaviour. I don't think that there is a use case for setting it more then once. I will add a check to ensure it can only be set once.

Answer test case: I used the test scripts but they are not ideal for this kind of PR. Is there already a test collection for such "internal" changes?

This patch did not apply either to the current head or on top of your previous patch. I think you have to 'rebase' to head, but I'm not a git expert.

I though I suggested this once before: E.g., (applies to all three) If tssMalloc is statically initialized to malloc, then the if statement within TSS_Malloc is unnecessary. I think the tssAllowMemoryCustomize flag is also unnecessary - if tssMalloc != malloc, it's been set.

It also needs a regression test. I wonder if using the openssl OPENSSL_malloc() function would work.

It seems i missed your suggestion with initializing the functions to their stdlib counterpart.

Using OPENSSL_malloc, OPENSSL_realloc and OPENSSL_free should work, maybe this would be a much simpler solution:

• Use malloc, realloc and free if TPM_TSS_NOCRYPTO is defined.
• Use OPENSSL_malloc, OPENSSL_realloc and OPENSSL_free if TPM_TSS_NOCRYPTO is not defined.

I don't know if any special regression tests would be required if this solution is used. The existing tests should still work and if the user calls CRYPTO_set_mem_functions it is his responsibility to do this before calling any other crypto function (including ibmtss).

The only disadvantage is that there is no way to change the memory functions if ibmtss is dynamically linked. If this should be possible TSS_SetMemoryFunctions (as wrapper for CRYPTO_set_mem_functions) would still be necessary and with it some new regression tests.

I think you misread my suggestion. I was suggesting the openssl commands for the regression test, not for the implementation.

Openssl cannot be used for the implementation because the TSS can use other crypto libraries. I have an implementation using mbedtls.