diff mbox series

[04/16] crypto: pcbc - remove bogus memcpy()s with src == dest

Message ID 20190104041625.3259-5-ebiggers@kernel.org (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show
Series crypto: skcipher template simplifications and conversions | expand

Commit Message

Eric Biggers Jan. 4, 2019, 4:16 a.m. UTC 
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(-)

Comments

David Howells Jan. 4, 2019, 9:57 a.m. UTC | #1 
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
Eric Biggers Jan. 4, 2019, 5:07 p.m. UTC | #2 
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
David Howells Jan. 4, 2019, 5:24 p.m. UTC | #3 
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>
diff mbox series

Patch

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;
 }