[PATCH] crypto: ixp4xx: avoid uninitialized variable use

[PATCH] crypto: ixp4xx: avoid uninitialized variable use

[Arnd Bergmann]

The move to the new AEAD interface introduced a path through the
aead_perform() function in the ixp4xx_crypto driver that leaves
lastlen uninitialized, as gcc warns:

crypto/ixp4xx_crypto.c:1072:5: error: 'lastlen' may be used uninitialized in this function [-Werror=maybe-uninitialized]
crypto/ixp4xx_crypto.c: In function 'aead_perform':
  if (unlikely(lastlen < authsize)) {

I don't really understand what the code does, but the warning
appears to be correct, and this is my best guess at how it
should behave instead: I'm introducing a temporary variable
that indicates whether we need to allocate an extra buffer
or not, and defaults that variable to 'false', so we only
allocate the buffer if one of the cases happen where we know
that "lastlen < authsize".

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: d7295a8dc965 ("crypto: ixp4xx - Convert to new AEAD interface")
---

Hi Herbert,

It was one of your patches that introduced the warning, so you may
be able to come up with a better fix than I did. Please see this as
a bug report. I have applied it in my ARM randconfig test tree to shut
up the warning for now.

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index e52496a172d0..00c39a5aa4c7 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -999,6 +999,7 @@ static int aead_perform(struct aead_request *req, int encrypt,
 				GFP_KERNEL : GFP_ATOMIC;
 	enum dma_data_direction src_direction = DMA_BIDIRECTIONAL;
 	unsigned int lastlen;
+	bool shortbuf = false;
 
 	if (qmgr_stat_full(SEND_QID))
 		return -EAGAIN;
@@ -1052,6 +1053,8 @@ static int aead_perform(struct aead_request *req, int encrypt,
 			if (lastlen >= authsize)
 				crypt->icv_rev_aes = buf->phys_addr +
 						     buf->buf_len - authsize;
+			else
+				shortbuf = true;
 		}
 	}
 
@@ -1067,9 +1070,11 @@ static int aead_perform(struct aead_request *req, int encrypt,
 		if (lastlen >= authsize)
 			crypt->icv_rev_aes = buf->phys_addr +
 					     buf->buf_len - authsize;
+		else
+			shortbuf = true;
 	}
 
-	if (unlikely(lastlen < authsize)) {
+	if (unlikely(shortbuf)) {
 		/* The 12 hmac bytes are scattered,
 		 * we need to copy them into a safe buffer */
 		req_ctx->hmac_virt = dma_pool_alloc(buffer_pool, flags,

Re: [PATCH] crypto: ixp4xx: avoid uninitialized variable use

[Herbert Xu]

On Mon, Jan 18, 2016 at 04:40:15PM +0100, Arnd Bergmann wrote:
> The move to the new AEAD interface introduced a path through the
> aead_perform() function in the ixp4xx_crypto driver that leaves
> lastlen uninitialized, as gcc warns:
> 
> crypto/ixp4xx_crypto.c:1072:5: error: 'lastlen' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> crypto/ixp4xx_crypto.c: In function 'aead_perform':
>   if (unlikely(lastlen < authsize)) {
> 
> I don't really understand what the code does, but the warning
> appears to be correct, and this is my best guess at how it
> should behave instead: I'm introducing a temporary variable
> that indicates whether we need to allocate an extra buffer
> or not, and defaults that variable to 'false', so we only
> allocate the buffer if one of the cases happen where we know
> that "lastlen < authsize".
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: d7295a8dc965 ("crypto: ixp4xx - Convert to new AEAD interface")
> ---
> 
> Hi Herbert,
> 
> It was one of your patches that introduced the warning, so you may
> be able to come up with a better fix than I did. Please see this as
> a bug report. I have applied it in my ARM randconfig test tree to shut
> up the warning for now.

How about this?

---8<---
Subject: crypto: ixp4xx - Fix false lastlen uninitialised warning

This patch fixes a false positive uninitialised variable warning
in aead_perform by moving the source processing in front of the
destination processing, thus ensuring that the initialisation of
lastlen is always visible to gcc.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index e52496a..2296934 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -1031,6 +1031,18 @@ static int aead_perform(struct aead_request *req, int encrypt,
 	BUG_ON(ivsize && !req->iv);
 	memcpy(crypt->iv, req->iv, ivsize);
 
+	buf = chainup_buffers(dev, req->src, crypt->auth_len,
+			      &src_hook, flags, src_direction);
+	req_ctx->src = src_hook.next;
+	crypt->src_buf = src_hook.phys_next;
+	if (!buf)
+		goto free_buf_src;
+
+	lastlen = buf->buf_len;
+	if (lastlen >= authsize)
+		crypt->icv_rev_aes = buf->phys_addr +
+				     buf->buf_len - authsize;
+
 	req_ctx->dst = NULL;
 
 	if (req->src != req->dst) {
@@ -1055,20 +1067,6 @@ static int aead_perform(struct aead_request *req, int encrypt,
 		}
 	}
 
-	buf = chainup_buffers(dev, req->src, crypt->auth_len,
-			      &src_hook, flags, src_direction);
-	req_ctx->src = src_hook.next;
-	crypt->src_buf = src_hook.phys_next;
-	if (!buf)
-		goto free_buf_src;
-
-	if (!encrypt || !req_ctx->dst) {
-		lastlen = buf->buf_len;
-		if (lastlen >= authsize)
-			crypt->icv_rev_aes = buf->phys_addr +
-					     buf->buf_len - authsize;
-	}
-
 	if (unlikely(lastlen < authsize)) {
 		/* The 12 hmac bytes are scattered,
 		 * we need to copy them into a safe buffer */
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: ixp4xx: avoid uninitialized variable use

[Arnd Bergmann]

On Tuesday 19 January 2016 09:00:21 Herbert Xu wrote:
> Subject: crypto: ixp4xx - Fix false lastlen uninitialised warning
> 
> This patch fixes a false positive uninitialised variable warning
> in aead_perform by moving the source processing in front of the
> destination processing, thus ensuring that the initialisation of
> lastlen is always visible to gcc.
> 
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 

I've checked that the warnings are now gone, and after reviewing the
code again, I see now that it was indeed a false positive,
contrary to what I thought before.

Patch looks good.

Acked-by: Arnd Bergmann <arnd@arndb.de>