[PATCH v4] Sm3 sm4 ipsec support - #2385
Conversation
github-actions
Bot
changed the title
9b2d84b to
5c7d71d
Compare
github-actions
Bot
changed the title
5c7d71d to
fd1cf02
Compare
github-actions
Bot
changed the title
This patch add ipsec SM3 SM4 capability, and add icvlen value for SM3 hash. Signed-off-by: Nithinsen Kaithakadan <nkaithakadan@marvell.com>
This patch add ipsec SM3 and SM4 test cases. Signed-off-by: Nithinsen Kaithakadan <nkaithakadan@marvell.com>
added inbound and outbound IPsec AH transport mode tests using sm3-hmac authentication. Signed-off-by: Daphne Priscilla <df@marvell.com> Signed-off-by: Nithinsen Kaithakadan <nkaithakadan@marvell.com>
fd1cf02 to
c3c0548
Compare
github-actions
Bot
changed the title
|
Hi @JannePeltonen , could you please review this patch set? |
| icv_len = 16; | ||
| break; | ||
| case ODP_AUTH_ALG_SM3_HMAC: | ||
| icv_len = 16; |
There was a problem hiding this comment.
Where does this come from? This appears to be half the output length of HMAC-SM3. Without knowing better, I think other plausible lenghts would be 12 and 32 bytes. Do you have a normative reference for how SM4-CBC and HMAC-SM3 are to be used with IPsec or other relevant documentation you could point to?
| int ipsec_check_esp_sm4_cbc_sm3(void) | ||
| { | ||
| return ipsec_check_esp(ODP_CIPHER_ALG_SM4_CBC, 128, | ||
| ODP_AUTH_ALG_SM3_HMAC, 256); |
There was a problem hiding this comment.
Here key length for HMAC-SM3 is 32 bytes but the key used in the test cases is 20 bytes long, so this is internally inconsistent. Which key length is correct? Do you have any source reference?
| ipsec_sa_destroy(sa); | ||
| } | ||
|
|
||
| static void test_in_ipv4_esp_sm4_cbc_sm3(void) |
There was a problem hiding this comment.
This function is placed between different AES-CBC + SHA variants. It would be better to have this in a more logical place (maybe just before test_ipsec_print). And the same placement comment applies to other functions, test vectors, etc added by these comments. Please try not to place them randomly.
| * some implementations may only support specific pairings (for example, | ||
| * SM4-CBC with SM3-HMAC), but the test suite itself does not require | ||
| * or assume any fixed cipher/auth pairing. This excludes combined mode | ||
| * algorithms such as AES-GCM. |
There was a problem hiding this comment.
This comment is overly verbose (at least to my taste) and perhaps not well placed here. One problem is that it makes it ambiguous what the last sentence ("This excludes...") refers to. If you really want to edit the comment, I think it would be sufficient to say that some combinations are not supported by all implementations (but there is a comment about that later too, so even that is not really necessary, IMHO).
|
|
||
| /* If SM4 and SM3 are not properly paired, SA creation failure is acceptable. */ | ||
| if ((cipher == ODP_CIPHER_ALG_SM4_CBC && auth != ODP_AUTH_ALG_SM3_HMAC) || | ||
| (auth == ODP_AUTH_ALG_SM3_HMAC && cipher != ODP_CIPHER_ALG_SM4_CBC)) |
There was a problem hiding this comment.
When an algorithm is used by itself wihtout combining it with any other algorithm (i.e. the other algorithm is the null algorithm), then SA creation should not fail, but this code would allow it for the new algorithms. I think think test should be moved after all the "must not fail" checks in this function.
| }, | ||
| }; | ||
|
|
||
| static const ODP_UNUSED ipsec_test_packet pkt_ipv4_esp_sm4_cbc_sm3 = { |
There was a problem hiding this comment.
Where does this test vector come from? Have you generated it yourself or does it come from e.g. some standard? And the same question to the other test vectors in the PR.
| 0x29, 0xb0, 0x30, 0x7b, 0x02, 0x7c, 0x8c, 0x4f | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
The commit messages of all the commits could be improved to match ODP style better (imperative mood, subject line prefix). Here are some suggestions that you could take inspiration from:
helper: ipsec: add SM4-CBC and HMAC-SM3 support
Recognize SM4-CBC and HMAC-SM3 as known algorithms with IPsec.
Add the algorithms in the IPsec algorithm capability check helper
functions and return the default ICV length for SM3-HMAC too.
validation: ipsec: add SM4-CBC and HMAC-SM3 test cases
Add test cases for SM4-CBC and HMAC-SM3.
validation: ipsec: add AH test cases for HMAC-SM3
Add test cases for inbound and outbound AH transport mode
using the HMAC-SM3 algorithm.
3 participants
Add IPsec support for SM3 and SM4