RPCSEC GSS krb5 KUnit test fails on arm64 with h/w accelerated ciphers enabled
Subsystems (5)
Chuck's recently-added RPCSEC GSS krb5 KUnit test (net/sunrpc/auth_gss/gss_krb5_test.c) is failing on arm64, specifically the RFC 3962 test cases (I'm just pasting the output of 1 case, but all 6 cases fail):
---8<---
[ 237.255197] # Subtest: RFC 3962 encryption
[ 237.255588] # RFC 3962 encryption: EXPECTATION FAILED at net/sunrpc/auth_gss/gss_krb5_test.c:772
Expected memcmp(param->next_iv->data, iv, param->next_iv->len) == 0, but
memcmp(param->next_iv->data, iv, param->next_iv->len) == 1 (0x1) IV mismatch
---8<---
If I disable the hardware accelerated ciphers (CONFIG_CRYPTO_AES_ARM64_CE_BLK and CONFIG_CRYPTO_AES_ARM64_NEON_BLK), then the test works.
Likewise, if I modify Chuck's test to explicitly request "cts(cbc(aes-generic))", then the test works.
The problem is that the asm helper aes_cbc_cts_encrypt in arch/arm64/crypto/aes-modes.S doesn't return the next IV.
If I make the following change, then the test works:
arch/arm64/crypto/aes-modes.S +1 -0
@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
-Scott
Herbert Xu
maintainerQuoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 15 lines · Herbert Xu
Quoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)
Quoted 6 lines
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
The issue seems to be that the caller expects iv_out to have been populated even when doing ciphertext stealing.
This is meaningless, because the output IV can only be used to chain additional encrypted data if the split is at a block boundary. The ciphertext stealing fundamentally terminates the encryption, and produces a block of ciphertext that is shorter than the block size, so what the output IV should be is actually unspecified.
IOW, test cases having plain/ciphertext vectors whose size is not a multiple of the cipher block size should not specify an expected value for the output IV.
Quoted 36 lines · Ard Biesheuvel
Quoted 15 lines · Herbert Xu
Quoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)Quoted 6 lines
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
The issue seems to be that the caller expects
iv_outto have been populated even when doing ciphertext stealing.This is meaningless, because the output IV can only be used to chain additional encrypted data if the split is at a block boundary. The ciphertext stealing fundamentally terminates the encryption, and produces a block of ciphertext that is shorter than the block size, so what the output IV should be is actually unspecified.
Quoted 3 lines
IOW, test cases having plain/ciphertext vectors whose size is not a multiple of the cipher block size should not specify an expected value for the output IV.
The test cases are extracted from RFC 3962 Appendix B. The standard clearly expects there to be a non-zero next IV for plaintext sizes that are not block-aligned.
Also, these test cases pass on other hardware platforms.
--
Chuck Lever
Quoted 37 lines · Chuck Lever III
Quoted 37 lines · Ard Biesheuvel
Quoted 15 lines · Herbert Xu
Quoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)Quoted 6 lines
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
The issue seems to be that the caller expects
iv_outto have been populated even when doing ciphertext stealing.This is meaningless, because the output IV can only be used to chain additional encrypted data if the split is at a block boundary. The ciphertext stealing fundamentally terminates the encryption, and produces a block of ciphertext that is shorter than the block size, so what the output IV should be is actually unspecified.
Quoted 8 lines
Quoted 3 lines
IOW, test cases having plain/ciphertext vectors whose size is not a multiple of the cipher block size should not specify an expected value for the output IV.
The test cases are extracted from RFC 3962 Appendix B. The standard clearly expects there to be a non-zero next IV for plaintext sizes that are not block-aligned.
OK, so this is the Kerberos V specific spec on how to use AES in CBC mode, which appears to describe how to chain multiple CBC encryptions together.
CBC-CTS itself does not define this: the IV is an input only, and the reason we happen to return the IV is because a single CBC operation may be broken up into several ones, which can only be done on block boundaries. This is why CBC-CTS itself passes all its tests: a single CBC-CTS encryption only performs ciphertext stealing at the very end, and the next IV is never used in that case. (This is why the CTS mode tests in crypto/testmgr.h don't have iv_out vectors)
Note that RFC3962 defines that the penultimate block of CBC-CTS ciphertext is used as the next IV. CBC returns the last ciphertext block as the output IV. It is a happy coincidence that the generic CTS template encapsulates CBC in a way where its output IV ends up in the right place.
Quoted 2 lines
Also, these test cases pass on other hardware platforms.
Fair enough.
I am not opposed to fixing this, but given that it is the RFC3962 spec that defines that the next IV is the penultimate full block of the previous CBC-CTS ciphertext, we might consider doing the memcpy() in the Kerberos code not in the CBC-CTS implementations. (The 32-bit ARM code will be broken in the same way, and potentially other implementations as well)
Quoted 72 lines · Ard Biesheuvel
Quoted 37 lines · Chuck Lever III
Quoted 37 lines · Ard Biesheuvel
Quoted 15 lines · Herbert Xu
Quoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)Quoted 6 lines
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
The issue seems to be that the caller expects
iv_outto have been populated even when doing ciphertext stealing.This is meaningless, because the output IV can only be used to chain additional encrypted data if the split is at a block boundary. The ciphertext stealing fundamentally terminates the encryption, and produces a block of ciphertext that is shorter than the block size, so what the output IV should be is actually unspecified.
Quoted 8 lines
Quoted 3 lines
IOW, test cases having plain/ciphertext vectors whose size is not a multiple of the cipher block size should not specify an expected value for the output IV.
The test cases are extracted from RFC 3962 Appendix B. The standard clearly expects there to be a non-zero next IV for plaintext sizes that are not block-aligned.
OK, so this is the Kerberos V specific spec on how to use AES in CBC mode, which appears to describe how to chain multiple CBC encryptions together.
CBC-CTS itself does not define this: the IV is an input only, and the reason we happen to return the IV is because a single CBC operation may be broken up into several ones, which can only be done on block boundaries. This is why CBC-CTS itself passes all its tests: a single CBC-CTS encryption only performs ciphertext stealing at the very end, and the next IV is never used in that case. (This is why the CTS mode tests in
crypto/testmgr.hdon't haveiv_outvectors)Note that RFC3962 defines that the penultimate block of CBC-CTS ciphertext is used as the next IV. CBC returns the last ciphertext block as the output IV. It is a happy coincidence that the generic CTS template encapsulates CBC in a way where its output IV ends up in the right place.
Quoted 2 lines
Also, these test cases pass on other hardware platforms.
Quoted 6 lines
Fair enough.
I am not opposed to fixing this, but given that it is the RFC3962 spec that defines that the next IV is the penultimate full block of the previous CBC-CTS ciphertext, we might consider doing the
memcpy()in the Kerberos code not in the CBC-CTS implementations.
Interesting thought. I'm all about proper layering, so I think this is worth considering. Can you send an RFC patch?
David, any opinion about this for crypto/krb5?
Quoted 3 lines
(The 32-bit ARM
code will be broken in the same way, and potentially other implementations as well)
--
Chuck Lever
Quoted 75 lines · Chuck Lever III
Quoted 75 lines · Ard Biesheuvel
Quoted 37 lines · Chuck Lever III
Quoted 37 lines · Ard Biesheuvel
Quoted 15 lines · Herbert Xu
Quoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)Quoted 6 lines
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
The issue seems to be that the caller expects
iv_outto have been populated even when doing ciphertext stealing.This is meaningless, because the output IV can only be used to chain additional encrypted data if the split is at a block boundary. The ciphertext stealing fundamentally terminates the encryption, and produces a block of ciphertext that is shorter than the block size, so what the output IV should be is actually unspecified.
Quoted 8 lines
Quoted 3 lines
IOW, test cases having plain/ciphertext vectors whose size is not a multiple of the cipher block size should not specify an expected value for the output IV.
The test cases are extracted from RFC 3962 Appendix B. The standard clearly expects there to be a non-zero next IV for plaintext sizes that are not block-aligned.
OK, so this is the Kerberos V specific spec on how to use AES in CBC mode, which appears to describe how to chain multiple CBC encryptions together.
CBC-CTS itself does not define this: the IV is an input only, and the reason we happen to return the IV is because a single CBC operation may be broken up into several ones, which can only be done on block boundaries. This is why CBC-CTS itself passes all its tests: a single CBC-CTS encryption only performs ciphertext stealing at the very end, and the next IV is never used in that case. (This is why the CTS mode tests in
crypto/testmgr.hdon't haveiv_outvectors)Note that RFC3962 defines that the penultimate block of CBC-CTS ciphertext is used as the next IV. CBC returns the last ciphertext block as the output IV. It is a happy coincidence that the generic CTS template encapsulates CBC in a way where its output IV ends up in the right place.
Quoted 2 lines
Also, these test cases pass on other hardware platforms.
Fair enough.
Quoted 8 lines
Quoted 4 lines
I am not opposed to fixing this, but given that it is the RFC3962 spec that defines that the next IV is the penultimate full block of the previous CBC-CTS ciphertext, we might consider doing the
memcpy()in the Kerberos code not in the CBC-CTS implementations.Interesting thought. I'm all about proper layering, so I think this is worth considering. Can you send an RFC patch?
I'm not that familiar with kunit so this is just an off hand suggestion, but I imagine something like the below would suffice
net/sunrpc/auth_gss/gss_krb5_crypto.c +0 -0
@@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher
*cipher, struct xdr_buf *buf,
ret = write_bytes_to_xdr_buf(buf, offset, data, len);
+ /*
+ * CBC-CTS does not define an output IV but RFC 3962 defines it as the
+ * penultimate block of ciphertext, so copy that into the IV buffer
+ * before returning.
+ */
+ if (encrypt)
+ memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));
out:
kfree(data);
return ret;Quoted 92 lines · Ard Biesheuvel
Quoted 75 lines · Chuck Lever III
Quoted 75 lines · Ard Biesheuvel
Quoted 37 lines · Chuck Lever III
Quoted 37 lines · Ard Biesheuvel
Quoted 15 lines · Herbert Xu
Quoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)Quoted 6 lines
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
The issue seems to be that the caller expects
iv_outto have been populated even when doing ciphertext stealing.This is meaningless, because the output IV can only be used to chain additional encrypted data if the split is at a block boundary. The ciphertext stealing fundamentally terminates the encryption, and produces a block of ciphertext that is shorter than the block size, so what the output IV should be is actually unspecified.
Quoted 8 lines
Quoted 3 lines
IOW, test cases having plain/ciphertext vectors whose size is not a multiple of the cipher block size should not specify an expected value for the output IV.
The test cases are extracted from RFC 3962 Appendix B. The standard clearly expects there to be a non-zero next IV for plaintext sizes that are not block-aligned.
OK, so this is the Kerberos V specific spec on how to use AES in CBC mode, which appears to describe how to chain multiple CBC encryptions together.
CBC-CTS itself does not define this: the IV is an input only, and the reason we happen to return the IV is because a single CBC operation may be broken up into several ones, which can only be done on block boundaries. This is why CBC-CTS itself passes all its tests: a single CBC-CTS encryption only performs ciphertext stealing at the very end, and the next IV is never used in that case. (This is why the CTS mode tests in
crypto/testmgr.hdon't haveiv_outvectors)Note that RFC3962 defines that the penultimate block of CBC-CTS ciphertext is used as the next IV. CBC returns the last ciphertext block as the output IV. It is a happy coincidence that the generic CTS template encapsulates CBC in a way where its output IV ends up in the right place.
Quoted 2 lines
Also, these test cases pass on other hardware platforms.
Fair enough.
Quoted 8 lines
Quoted 4 lines
I am not opposed to fixing this, but given that it is the RFC3962 spec that defines that the next IV is the penultimate full block of the previous CBC-CTS ciphertext, we might consider doing the
memcpy()in the Kerberos code not in the CBC-CTS implementations.Interesting thought. I'm all about proper layering, so I think this is worth considering. Can you send an RFC patch?
I'm not that familiar with kunit so this is just an off hand suggestion, but I imagine something like the below would suffice
net/sunrpc/auth_gss/gss_krb5_crypto.c +0
-0@@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher *cipher, struct xdr_buf *buf,
net/sunrpc/auth_gss/gss_krb5_crypto.c gss_krb5_cts_crypt(struct crypto_sync_skcipher · line 649
Quoted 10 lines
net/sunrpc/auth_gss/gss_krb5_crypto.c +7
-0ret =write_bytes_to_xdr_buf (buf, offset, data, len); +/* +* CBC-CTS does not define an output IV but RFC 3962 defines it as the +* penultimate block of ciphertext, so copy that into the IV buffer +* before returning. +*/ +if (encrypt) +memcpy (iv, data,crypto_sync_skcipher_ivsize (cipher));
The whole "update the IV" thing that the crypto API does for some algorithms is very weird and nonstandard, and most algorithms don't implement it. So I'd definitely support handling this in the caller here.
- Eric
Scott Mayhew
Tested-byQuoted 104 lines · Ard Biesheuvel
Quoted 75 lines · Chuck Lever III
Quoted 75 lines · Ard Biesheuvel
Quoted 37 lines · Chuck Lever III
Quoted 37 lines · Ard Biesheuvel
Quoted 15 lines · Herbert Xu
Quoted 13 lines · Scott Mayhew
arch/arm64/crypto/aes-modes.S +1
-0@@ -268,6 +268,7 @@ AES_FUNC_START(aes_cbc_cts_encrypt) add x4, x0, x4 st1 {v0.16b}, [x4] /* overlapping stores */ st1 {v1.16b}, [x0] + st1 {v1.16b}, [x5] ret AES_FUNC_END(aes_cbc_cts_encrypt)Quoted 6 lines
Quoted 3 lines
But I don't know if that change is at all correct! (I've never even looked at arm64 asm before). If someone who's knowledgeable about this code could chime in, I'd appreciate it.
Ard, could you please take a look at this?
The issue seems to be that the caller expects
iv_outto have been populated even when doing ciphertext stealing.This is meaningless, because the output IV can only be used to chain additional encrypted data if the split is at a block boundary. The ciphertext stealing fundamentally terminates the encryption, and produces a block of ciphertext that is shorter than the block size, so what the output IV should be is actually unspecified.
Quoted 8 lines
Quoted 3 lines
IOW, test cases having plain/ciphertext vectors whose size is not a multiple of the cipher block size should not specify an expected value for the output IV.
The test cases are extracted from RFC 3962 Appendix B. The standard clearly expects there to be a non-zero next IV for plaintext sizes that are not block-aligned.
OK, so this is the Kerberos V specific spec on how to use AES in CBC mode, which appears to describe how to chain multiple CBC encryptions together.
CBC-CTS itself does not define this: the IV is an input only, and the reason we happen to return the IV is because a single CBC operation may be broken up into several ones, which can only be done on block boundaries. This is why CBC-CTS itself passes all its tests: a single CBC-CTS encryption only performs ciphertext stealing at the very end, and the next IV is never used in that case. (This is why the CTS mode tests in
crypto/testmgr.hdon't haveiv_outvectors)Note that RFC3962 defines that the penultimate block of CBC-CTS ciphertext is used as the next IV. CBC returns the last ciphertext block as the output IV. It is a happy coincidence that the generic CTS template encapsulates CBC in a way where its output IV ends up in the right place.
Quoted 2 lines
Also, these test cases pass on other hardware platforms.
Fair enough.
Quoted 8 lines
Quoted 4 lines
I am not opposed to fixing this, but given that it is the RFC3962 spec that defines that the next IV is the penultimate full block of the previous CBC-CTS ciphertext, we might consider doing the
memcpy()in the Kerberos code not in the CBC-CTS implementations.Interesting thought. I'm all about proper layering, so I think this is worth considering. Can you send an RFC patch?
I'm not that familiar with kunit so this is just an off hand suggestion, but I imagine something like the below would suffice
net/sunrpc/auth_gss/gss_krb5_crypto.c +7
-0@@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher *cipher, struct xdr_buf *buf, ret =write_bytes_to_xdr_buf (buf, offset, data, len); +/* +* CBC-CTS does not define an output IV but RFC 3962 defines it as the +* penultimate block of ciphertext, so copy that into the IV buffer +* before returning. +*/ +if (encrypt) +memcpy (iv, data,crypto_sync_skcipher_ivsize (cipher)); out:kfree (data);
Quoted 2 lines
return ret;
Thanks, Ard. That fixes it on aarch64 (also ran it on x86_64, ppc64le, s390x, and aarch64 w/ 64k pages). Could you send it as an official patch and add
-Scott