Re: [arm-integrator:ixp4xx-test 22/23] drivers/crypto/ixp4xx_crypto.c:1060 aead_perform() error: uninitialized symbol 'icv_rev_aes'.

[Arnd Bergmann <arnd@arndb.de>]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

On Fri, Oct 11, 2019 at 12:41 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git ixp4xx-test
> head:   1b826e686b390fd1eeb347def38185fefcbd5140
> commit: 811c4aaa704314770fe5a70944100732fdc1f3b8 [22/23] crypto: ixp4xx: avoid BUILD_BUG_ON()
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> New smatch warnings:
> drivers/crypto/ixp4xx_crypto.c:1060 aead_perform() error: uninitialized symbol 'icv_rev_aes'.

Right, this was uncovered by allowing the driver to be compile-tested on
x86. smatch is of course right to point out the uninitialized variable,
but it seems harmless as it is only uninitialized in case of an error,
and the uninitialized variable here gets copied into another data structure
that then gets put back, so it seems completely harmless

There is a trivial fix:

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 1c6a04ecbf8f..a4eb1e98c87b 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -1057,9 +1057,9 @@ static int aead_perform(struct aead_request
*req, int encrypt,
                 * we need to copy them into a safe buffer */
                req_ctx->hmac_virt = dma_pool_alloc(buffer_pool, flags,
                                                    &icv_rev_aes);
-               crypt->icv_rev_aes = (u32)icv_rev_aes;
                if (unlikely(!req_ctx->hmac_virt))
                        goto free_buf_dst;
+               crypt->icv_rev_aes = (u32)icv_rev_aes;
                if (!encrypt) {
                        scatterwalk_map_and_copy(req_ctx->hmac_virt,
                                req->src, cryptlen, authsize, 0);

Dan, I think I would just leave it unchanged for a case like this, but I
can send a patch if you think it's worth addressing the warning.

     Arnd