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;
Subject: Re: [PATCH] crypto: ghash - fix unaligned memory access in
 ghash_setkey()
To:     Eric Biggers <ebiggers@kernel.org>, linux-crypto@vger.kernel.org
Cc:     linux-arm-kernel@lists.infradead.org,
        Peter Robinson <pbrobinson@gmail.com>, stable@vger.kernel.org
References: <20190530175039.195574-1-ebiggers@kernel.org>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <0f7e6d3d-aa27-30c3-5c82-67d484bf667c@c-s.fr>
Date:   Mon, 3 Jun 2019 09:27:24 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <20190530175039.195574-1-ebiggers@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk


Le 30/05/2019 à 19:50, Eric Biggers a écrit :
> From: Eric Biggers <ebiggers@google.com>
> 
> Changing ghash_mod_init() to be subsys_initcall made it start running
> before the alignment fault handler has been installed on ARM.  In kernel
> builds where the keys in the ghash test vectors happened to be
> misaligned in the kernel image, this exposed the longstanding bug that
> ghash_setkey() is incorrectly casting the key buffer (which can have any
> alignment) to be128 for passing to gf128mul_init_4k_lle().
> 
> Fix this by memcpy()ing the key to a temporary buffer.

Shouldn't we make it dependent on CONFIG_HAVE_64BIT_ALIGNED_ACCESS or 
!CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS ?

Christophe

> 
> Don't fix it by setting an alignmask on the algorithm instead because
> that would unnecessarily force alignment of the data too.
> 
> Fixes: 2cdc6899a88e ("crypto: ghash - Add GHASH digest algorithm for GCM")
> Reported-by: Peter Robinson <pbrobinson@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>   crypto/ghash-generic.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/ghash-generic.c b/crypto/ghash-generic.c
> index e6307935413c1..c8a347798eae6 100644
> --- a/crypto/ghash-generic.c
> +++ b/crypto/ghash-generic.c
> @@ -34,6 +34,7 @@ static int ghash_setkey(struct crypto_shash *tfm,
>   			const u8 *key, unsigned int keylen)
>   {
>   	struct ghash_ctx *ctx = crypto_shash_ctx(tfm);
> +	be128 k;
>   
>   	if (keylen != GHASH_BLOCK_SIZE) {
>   		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
> @@ -42,7 +43,12 @@ static int ghash_setkey(struct crypto_shash *tfm,
>   
>   	if (ctx->gf128)
>   		gf128mul_free_4k(ctx->gf128);
> -	ctx->gf128 = gf128mul_init_4k_lle((be128 *)key);
> +
> +	BUILD_BUG_ON(sizeof(k) != GHASH_BLOCK_SIZE);
> +	memcpy(&k, key, GHASH_BLOCK_SIZE); /* avoid violating alignment rules */
> +	ctx->gf128 = gf128mul_init_4k_lle(&k);
> +	memzero_explicit(&k, GHASH_BLOCK_SIZE);
> +
>   	if (!ctx->gf128)
>   		return -ENOMEM;
>   
>