Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 25 Feb 2020 12:02:44 -0800
From:   Eric Biggers <ebiggers@kernel.org>
To:     Gilad Ben-Yossef <gilad@benyossef.com>
Cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Ofir Drang <ofir.drang@arm.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] crypto: testmgr - sync both RFC4106 IV copies
Message-ID: <20200225200244.GB114977@gmail.com>
References: <20200225154834.25108-1-gilad@benyossef.com>
 <20200225154834.25108-3-gilad@benyossef.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200225154834.25108-3-gilad@benyossef.com>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 25, 2020 at 05:48:34PM +0200, Gilad Ben-Yossef wrote:
> RFC4106 AEAD ciphers the AAD is the concatenation of associated
> authentication data || IV || plaintext or ciphertext but the
> random AEAD message generation in testmgr extended tests did
> not obey this requirements producing messages with undefined
> behaviours. Fix it by syncing the copies if needed.
> 
> Since this only relevant for developer only extended tests any
> additional cycles/run time costs are negligible.
> 
> This fixes extended AEAD test failures with the ccree driver
> caused by illegal input.
> 
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Eric Biggers <ebiggers@kernel.org>
> ---
>  crypto/testmgr.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index cf565b063cdf..288f349a0cae 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -95,6 +95,11 @@ struct aead_test_suite {
>       /*
>        * Set if the algorithm intentionally ignores the last 8 bytes of the
>        * AAD buffer during decryption.
>        */
>       unsigned int esp_aad : 1;
> +
> +	/*
> +	 * Set if the algorithm requires the IV to trail the AAD buffer.
> +	 */
> +	unsigned int iv_aad : 1;
>  };

What's the difference between esp_aad and iv_aad?  Are you sure we need another
flag and not just use the existing flag?

If they're both needed, please document them properly.  You're currently setting
them both on some algorithms, which based on the current comments is a logical
contradiction because esp_aad is documented to mean that the last 8 bytes are
ignored while iv_aad is documented to mean that these bytes must be the IV.

>  
>  struct cipher_test_suite {
> @@ -2207,6 +2212,10 @@ static void generate_aead_message(struct aead_request *req,
>  
>  	/* Generate the AAD. */
>  	generate_random_bytes((u8 *)vec->assoc, vec->alen);
> +	/* For RFC4106 algs, a copy of the IV is part of the AAD */
> +	if (suite->iv_aad)
> +		memcpy(((u8 *)vec->assoc + vec->alen - ivsize), vec->iv,
> +		       ivsize);

What guarantees that vec->alen >= ivsize?

- Eric