-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable little-endian signing and dual-endian verification in SPDM 1.0… #2324
Conversation
…\1.1 Signed-off-by: Kong, Richard <[email protected]>
Signed-off-by: Kong, Richard <[email protected]>
Signed-off-by: Kong, Richard <[email protected]>
@richkong88 , would you please squash the commit? There is no need to add patch for coding style fix. |
include/library/spdm_lib_config.h
Outdated
@@ -383,4 +383,68 @@ | |||
#define LIBSPDM_CHECK_SPDM_CONTEXT 1 | |||
#endif | |||
|
|||
/* Enable Endianness for signatures on SPDM1.0 and SPDM1.1. | |||
* Big-endian is default, but little-endian may need to be supported as well. | |||
* See issue: https://github.com/DMTF/libspdm/issues/2151 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we need this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Yes, I'll put everything into a single CL, once all feedback is incorporated. |
I think we need to update req_asym version as well, such as Also, can we have some unit test or CI to ensure there will be no regression? |
{ | ||
#if LIBSPDM_SPDM_10_11_SIGN_LITTLE_ENDIAN_RSA_ECDSA | ||
uint32_t swap_endian_rsa = 0; | ||
uint32_t swap_endian_ecdsa = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think swap_endian_rsa and swap_endian_ecdsa should be a bool
type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
static void libspdm_copy_signature_swap_endian( | ||
uint8_t *endian_swapped_signature_buffer, | ||
size_t endian_swapped_signature_buffer_size, | ||
bool is_dual_buffer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_dual_buffer
brings confusion to me, and it takes me sometime to understand what it really means.
Maybe just define two functions: libspdm_copy_signature_swap_endian_for_ecdsa() and libspdm_copy_signature_swap_endian_for_rsa() ?
Then those functions can be reused by libspdm_asym_signature_swap_endian_if_necessary().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I split into rsa and ecdsa versions and tried to make the code clearer.
Signed-off-by: Kong, Richard <[email protected]>
Will update req_asym versions and add unit tests. |
In general the macros should only be used if they impart a significant reduction in size. In this case they don't so I would like to see this set at runtime through |
While I'm not generally opposed to a runtime decision, it seems that the libspdm_crypt_asym routines do not receive an "spdm_context" to extract the libspdm_set_data field? I think any "context" parameters are crypto contexts. So making it runtime would involve changing a lot of interfaces (to take another parameter to indicate the runtime choic) which I was trying to minimize. Please correct me if I'm misunderstanding this.
I'm fine lumping 1.0 and 1.1 together. @jyao1 Jiewen, do you think lumping them together is ok? |
Signed-off-by: Kong, Richard <[email protected]>
If we use runtime mechanism, we can combine 1.0 and 1.1 because the integrator can make final decision based upon version. But if we want to keep MACRO, then I think we should separate them to allow the flexibility, because the text in 1.0 and 1.1 are different. |
Going to try a runtime mechanism. Closing this pull request. |
…\1.1
Fix: #2151