From: Eric Biggers ebiggers@google.com
The memcpy()s in the PCBC implementation use walk->iv as both the source and destination, which has undefined behavior. These memcpy()'s are actually unneeded, because walk->iv is already used to hold the previous plaintext block XOR'd with the previous ciphertext block. Thus, walk->iv is already updated to its final value.
So remove the broken and unnecessary memcpy()s.
Fixes: 91652be5d1b9 ("[CRYPTO] pcbc: Add Propagated CBC template") Cc: stable@vger.kernel.org # v2.6.21+ Cc: David Howells dhowells@redhat.com Signed-off-by: Eric Biggers ebiggers@google.com --- crypto/pcbc.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/crypto/pcbc.c b/crypto/pcbc.c index 8aa10144407c0..1b182dfedc948 100644 --- a/crypto/pcbc.c +++ b/crypto/pcbc.c @@ -51,7 +51,7 @@ static int crypto_pcbc_encrypt_segment(struct skcipher_request *req, unsigned int nbytes = walk->nbytes; u8 *src = walk->src.virt.addr; u8 *dst = walk->dst.virt.addr; - u8 *iv = walk->iv; + u8 * const iv = walk->iv;
do { crypto_xor(iv, src, bsize); @@ -72,7 +72,7 @@ static int crypto_pcbc_encrypt_inplace(struct skcipher_request *req, int bsize = crypto_cipher_blocksize(tfm); unsigned int nbytes = walk->nbytes; u8 *src = walk->src.virt.addr; - u8 *iv = walk->iv; + u8 * const iv = walk->iv; u8 tmpbuf[MAX_CIPHER_BLOCKSIZE];
do { @@ -84,8 +84,6 @@ static int crypto_pcbc_encrypt_inplace(struct skcipher_request *req, src += bsize; } while ((nbytes -= bsize) >= bsize);
- memcpy(walk->iv, iv, bsize); - return nbytes; }
@@ -121,7 +119,7 @@ static int crypto_pcbc_decrypt_segment(struct skcipher_request *req, unsigned int nbytes = walk->nbytes; u8 *src = walk->src.virt.addr; u8 *dst = walk->dst.virt.addr; - u8 *iv = walk->iv; + u8 * const iv = walk->iv;
do { crypto_cipher_decrypt_one(tfm, dst, src); @@ -132,8 +130,6 @@ static int crypto_pcbc_decrypt_segment(struct skcipher_request *req, dst += bsize; } while ((nbytes -= bsize) >= bsize);
- memcpy(walk->iv, iv, bsize); - return nbytes; }
@@ -144,7 +140,7 @@ static int crypto_pcbc_decrypt_inplace(struct skcipher_request *req, int bsize = crypto_cipher_blocksize(tfm); unsigned int nbytes = walk->nbytes; u8 *src = walk->src.virt.addr; - u8 *iv = walk->iv; + u8 * const iv = walk->iv; u8 tmpbuf[MAX_CIPHER_BLOCKSIZE] __aligned(__alignof__(u32));
do { @@ -156,8 +152,6 @@ static int crypto_pcbc_decrypt_inplace(struct skcipher_request *req, src += bsize; } while ((nbytes -= bsize) >= bsize);
- memcpy(walk->iv, iv, bsize); - return nbytes; }
Eric Biggers ebiggers@kernel.org wrote:
- u8 *iv = walk->iv;
- u8 * const iv = walk->iv;
Does adding this const actually gain anything? (this is done twice)
David
On Fri, Jan 04, 2019 at 09:57:13AM +0000, David Howells wrote:
Eric Biggers ebiggers@kernel.org wrote:
- u8 *iv = walk->iv;
- u8 * const iv = walk->iv;
Does adding this const actually gain anything? (this is done twice)
David
It makes it clearer what's going on, especially since some modes update the 'iv' pointer after each block (delaying the copy to 'walk.iv' until the end) but others can't do that. The 'const' is helpful to further distinguish these two cases, which were confused in both the pcbc and cfb implementations.
- Eric
Eric Biggers ebiggers@kernel.org wrote:
It makes it clearer what's going on, especially since some modes update the 'iv' pointer after each block (delaying the copy to 'walk.iv' until the end) but others can't do that. The 'const' is helpful to further distinguish these two cases, which were confused in both the pcbc and cfb implementations.
I'm not sure I agree that it makes it clearer, but:
Reviewed-and-tested-by: David Howells dhowells@redhat.com
linux-stable-mirror@lists.linaro.org