crypto: ixp4xx: avoid uninitialized variable use

Commit Message

Herbert Xu Jan. 19, 2016, 1 a.m. UTC

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 <[e-mail]>
> 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 <[e-mail]>
Signed-off-by: Herbert Xu <[e-mail]>

Comments

Arnd Bergmann Jan. 19, 2016, 3:03 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Herbert Xu <[e-mail]>
> 

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 <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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 */