Fwd: [crypto:master 60/60] arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64

Fwd: [crypto:master 60/60] arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64

[Ard Biesheuvel]

Greg,

This pertains to commit 8ceee72808d1 (crypto: ghash-clmulni-intel -
use C implementation for setkey()) that has been pulled by Linus
during the current merge window.

It is missing two things:
- a cc to stable annotation
- a fix for the sparse warning below (change cast from __be64 to __force __be64)

The reason for cc'ing stable on this patch is that it fixes a
potential data corruption issue where the ghash setkey() method uses
SSE registers without calling kernel_fpu_begin() first. This issue was
introduced by 0e1227d356e9b (crypto: ghash - Add PCLMULQDQ accelerated
implementation).

So how would you like to proceed with this? Should I propose a new
patch somewhere?

Regards,
Ard.


---------- Forwarded message ----------
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: 1 April 2014 14:48
Subject: Re: [crypto:master 60/60]
arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to
restricted __be64
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
kbuild test robot <fengguang.wu@intel.com>


On Tue, Apr 01, 2014 at 02:37:20PM +0200, Ard Biesheuvel wrote:
> On 1 April 2014 13:23, kbuild test robot <fengguang.wu@intel.com> wrote:
> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git master
> > head:   8ceee72808d1ae3fb191284afc2257a2be964725
> > commit: 8ceee72808d1ae3fb191284afc2257a2be964725 [60/60] crypto: ghash-clmulni-intel - use C implementation for setkey()
> > reproduce: make C=1 CF=-D__CHECK_ENDIAN__
> >
> >
> > sparse warnings: (new ones prefixed by >>)
> >
> >>> arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64
> >>> arch/x86/crypto/ghash-clmulni-intel_glue.c:72:25: sparse: cast to restricted __be64
> >
>
> Sigh.
>
> The sparse warnings /without/ the be64 casts are even worse.
>
> The obvious fix is not to use a be128 for the key, as it is obviously
> an opaque type that just represents a byte array.
> So, Herbert, if you prefer, I can rework this patch to use be128
> instead of u128 inside struct ghash_ctx, but it will have some fallout
> throughout the file. Or instead, we cast to '__force __be64',
> basically just telling sparse to shut up ...

I'll add the __force to shut it up.  Thanks!
--
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: Fwd: [crypto:master 60/60] arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64

[gregkh]

On Fri, Apr 04, 2014 at 10:11:19AM +0200, Ard Biesheuvel wrote:
> Greg,
> 
> This pertains to commit 8ceee72808d1 (crypto: ghash-clmulni-intel -
> use C implementation for setkey()) that has been pulled by Linus
> during the current merge window.
> 
> It is missing two things:
> - a cc to stable annotation
> - a fix for the sparse warning below (change cast from __be64 to __force __be64)
> 
> The reason for cc'ing stable on this patch is that it fixes a
> potential data corruption issue where the ghash setkey() method uses
> SSE registers without calling kernel_fpu_begin() first. This issue was
> introduced by 0e1227d356e9b (crypto: ghash - Add PCLMULQDQ accelerated
> implementation).
> 
> So how would you like to proceed with this? Should I propose a new
> patch somewhere?

No problem, I'll apply this as-is.  But it doesn't apply to the
3.4-stable tree cleanly, can you send me a backported version if it's
still needed there as well?

thanks,

greg k-h

Re: Fwd: [crypto:master 60/60] arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64

[Ard Biesheuvel]

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

On 11 April 2014 18:03, gregkh@linuxfoundation.org
<gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 04, 2014 at 10:11:19AM +0200, Ard Biesheuvel wrote:
>> Greg,
>>
>> This pertains to commit 8ceee72808d1 (crypto: ghash-clmulni-intel -
>> use C implementation for setkey()) that has been pulled by Linus
>> during the current merge window.
>>
>> It is missing two things:
>> - a cc to stable annotation
>> - a fix for the sparse warning below (change cast from __be64 to __force __be64)
>>
>> The reason for cc'ing stable on this patch is that it fixes a
>> potential data corruption issue where the ghash setkey() method uses
>> SSE registers without calling kernel_fpu_begin() first. This issue was
>> introduced by 0e1227d356e9b (crypto: ghash - Add PCLMULQDQ accelerated
>> implementation).
>>
>> So how would you like to proceed with this? Should I propose a new
>> patch somewhere?
>
> No problem, I'll apply this as-is.  But it doesn't apply to the
> 3.4-stable tree cleanly, can you send me a backported version if it's
> still needed there as well?
>

Yes, the code was broken from the start. 3.4 version is attached, the
only difference is the missing ENDPROC() at the end of the asm file.

In the mean time, Herbert has submitted a fix for the sparse warning,
but we settled on a different fix than I had suggested before.
https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=0ea481466d1c

Note that this code has not been tested (not by me, at least), so I
wouldn't suggest you take it straight away, but if you care about the
sparse warning, we could add a cc stable to it as well, I suppose.

Regards,
Ard.

[-- Attachment #2: 0001-crypto-ghash-clmulni-intel-use-C-implementation-for-.patch --]
[-- Type: text/x-patch, Size: 3386 bytes --]

From db9c70e8f3291490fec0da56dce2bfa7837e99f2 Mon Sep 17 00:00:00 2001
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Fri, 11 Apr 2014 20:37:37 +0200
Subject: [PATCH] crypto: ghash-clmulni-intel - use C implementation for
 setkey()

commit 8ceee72808d1ae3fb191284afc2257a2be964725 upstream.

The GHASH setkey() function uses SSE registers but fails to call
kernel_fpu_begin()/kernel_fpu_end(). Instead of adding these calls, and
then having to deal with the restriction that they cannot be called from
interrupt context, move the setkey() implementation to the C domain.

Note that setkey() does not use any particular SSE features and is not
expected to become a performance bottleneck.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: H. Peter Anvin <hpa@linux.intel.com>
Fixes: 0e1227d356e9b (crypto: ghash - Add PCLMULQDQ accelerated implementation)
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 arch/x86/crypto/ghash-clmulni-intel_asm.S  | 28 ----------------------------
 arch/x86/crypto/ghash-clmulni-intel_glue.c | 14 +++++++++++---
 2 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/arch/x86/crypto/ghash-clmulni-intel_asm.S b/arch/x86/crypto/ghash-clmulni-intel_asm.S
index 1eb7f90cb7b9..eb4d2a254b35 100644
--- a/arch/x86/crypto/ghash-clmulni-intel_asm.S
+++ b/arch/x86/crypto/ghash-clmulni-intel_asm.S
@@ -24,10 +24,6 @@
 .align 16
 .Lbswap_mask:
 	.octa 0x000102030405060708090a0b0c0d0e0f
-.Lpoly:
-	.octa 0xc2000000000000000000000000000001
-.Ltwo_one:
-	.octa 0x00000001000000000000000000000001
 
 #define DATA	%xmm0
 #define SHASH	%xmm1
@@ -131,27 +127,3 @@ ENTRY(clmul_ghash_update)
 	movups DATA, (%rdi)
 .Lupdate_just_ret:
 	ret
-
-/*
- * void clmul_ghash_setkey(be128 *shash, const u8 *key);
- *
- * Calculate hash_key << 1 mod poly
- */
-ENTRY(clmul_ghash_setkey)
-	movaps .Lbswap_mask, BSWAP
-	movups (%rsi), %xmm0
-	PSHUFB_XMM BSWAP %xmm0
-	movaps %xmm0, %xmm1
-	psllq $1, %xmm0
-	psrlq $63, %xmm1
-	movaps %xmm1, %xmm2
-	pslldq $8, %xmm1
-	psrldq $8, %xmm2
-	por %xmm1, %xmm0
-	# reduction
-	pshufd $0b00100100, %xmm2, %xmm1
-	pcmpeqd .Ltwo_one, %xmm1
-	pand .Lpoly, %xmm1
-	pxor %xmm1, %xmm0
-	movups %xmm0, (%rdi)
-	ret
diff --git a/arch/x86/crypto/ghash-clmulni-intel_glue.c b/arch/x86/crypto/ghash-clmulni-intel_glue.c
index b4bf0a63b520..c07446d17463 100644
--- a/arch/x86/crypto/ghash-clmulni-intel_glue.c
+++ b/arch/x86/crypto/ghash-clmulni-intel_glue.c
@@ -30,8 +30,6 @@ void clmul_ghash_mul(char *dst, const be128 *shash);
 void clmul_ghash_update(char *dst, const char *src, unsigned int srclen,
 			const be128 *shash);
 
-void clmul_ghash_setkey(be128 *shash, const u8 *key);
-
 struct ghash_async_ctx {
 	struct cryptd_ahash *cryptd_tfm;
 };
@@ -58,13 +56,23 @@ static int ghash_setkey(struct crypto_shash *tfm,
 			const u8 *key, unsigned int keylen)
 {
 	struct ghash_ctx *ctx = crypto_shash_ctx(tfm);
+	be128 *x = (be128 *)key;
+	u64 a, b;
 
 	if (keylen != GHASH_BLOCK_SIZE) {
 		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
 		return -EINVAL;
 	}
 
-	clmul_ghash_setkey(&ctx->shash, key);
+	/* perform multiplication by 'x' in GF(2^128) */
+	a = be64_to_cpu(x->a);
+	b = be64_to_cpu(x->b);
+
+	ctx->shash.a = (__be64)((b << 1) | (a >> 63));
+	ctx->shash.b = (__be64)((a << 1) | (b >> 63));
+
+	if (a >> 63)
+		ctx->shash.b ^= cpu_to_be64(0xc2);
 
 	return 0;
 }
-- 
1.8.3.2

Re: Fwd: [crypto:master 60/60] arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64

[gregkh]

On Fri, Apr 11, 2014 at 09:48:42PM +0200, Ard Biesheuvel wrote:
> On 11 April 2014 18:03, gregkh@linuxfoundation.org
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Apr 04, 2014 at 10:11:19AM +0200, Ard Biesheuvel wrote:
> >> Greg,
> >>
> >> This pertains to commit 8ceee72808d1 (crypto: ghash-clmulni-intel -
> >> use C implementation for setkey()) that has been pulled by Linus
> >> during the current merge window.
> >>
> >> It is missing two things:
> >> - a cc to stable annotation
> >> - a fix for the sparse warning below (change cast from __be64 to __force __be64)
> >>
> >> The reason for cc'ing stable on this patch is that it fixes a
> >> potential data corruption issue where the ghash setkey() method uses
> >> SSE registers without calling kernel_fpu_begin() first. This issue was
> >> introduced by 0e1227d356e9b (crypto: ghash - Add PCLMULQDQ accelerated
> >> implementation).
> >>
> >> So how would you like to proceed with this? Should I propose a new
> >> patch somewhere?
> >
> > No problem, I'll apply this as-is.  But it doesn't apply to the
> > 3.4-stable tree cleanly, can you send me a backported version if it's
> > still needed there as well?
> >
> 
> Yes, the code was broken from the start. 3.4 version is attached, the
> only difference is the missing ENDPROC() at the end of the asm file.
> 
> In the mean time, Herbert has submitted a fix for the sparse warning,
> but we settled on a different fix than I had suggested before.
> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=0ea481466d1c
> 
> Note that this code has not been tested (not by me, at least), so I
> wouldn't suggest you take it straight away, but if you care about the
> sparse warning, we could add a cc stable to it as well, I suppose.

No, I don't care about a sparse warning, unless it's fixing a real bug.

I'll queue this up for the next 3.4 release after the one that happens
this weekend, thanks.

greg k-h

Re: Fwd: [crypto:master 60/60] arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64

[gregkh]

On Fri, Apr 11, 2014 at 09:48:42PM +0200, Ard Biesheuvel wrote:
> On 11 April 2014 18:03, gregkh@linuxfoundation.org
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Apr 04, 2014 at 10:11:19AM +0200, Ard Biesheuvel wrote:
> >> Greg,
> >>
> >> This pertains to commit 8ceee72808d1 (crypto: ghash-clmulni-intel -
> >> use C implementation for setkey()) that has been pulled by Linus
> >> during the current merge window.
> >>
> >> It is missing two things:
> >> - a cc to stable annotation
> >> - a fix for the sparse warning below (change cast from __be64 to __force __be64)
> >>
> >> The reason for cc'ing stable on this patch is that it fixes a
> >> potential data corruption issue where the ghash setkey() method uses
> >> SSE registers without calling kernel_fpu_begin() first. This issue was
> >> introduced by 0e1227d356e9b (crypto: ghash - Add PCLMULQDQ accelerated
> >> implementation).
> >>
> >> So how would you like to proceed with this? Should I propose a new
> >> patch somewhere?
> >
> > No problem, I'll apply this as-is.  But it doesn't apply to the
> > 3.4-stable tree cleanly, can you send me a backported version if it's
> > still needed there as well?
> >
> 
> Yes, the code was broken from the start. 3.4 version is attached, the
> only difference is the missing ENDPROC() at the end of the asm file.

Now applied, thanks.

> In the mean time, Herbert has submitted a fix for the sparse warning,
> but we settled on a different fix than I had suggested before.
> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=0ea481466d1c
> 
> Note that this code has not been tested (not by me, at least), so I
> wouldn't suggest you take it straight away, but if you care about the
> sparse warning, we could add a cc stable to it as well, I suppose.

If it's a real bugfix that people can hit, then yse, I'll take it.  Just
let me know when it hits Linus's tree.

greg k-h

Re: Fwd: [crypto:master 60/60] arch/x86/crypto/ghash-clmulni-intel_glue.c:71:25: sparse: cast to restricted __be64

[Ard Biesheuvel]

On 7 May 2014 00:42, gregkh@linuxfoundation.org
<gregkh@linuxfoundation.org> wrote:
> On Fri, Apr 11, 2014 at 09:48:42PM +0200, Ard Biesheuvel wrote:
>> On 11 April 2014 18:03, gregkh@linuxfoundation.org
>> <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Apr 04, 2014 at 10:11:19AM +0200, Ard Biesheuvel wrote:
>> >> Greg,
>> >>
>> >> This pertains to commit 8ceee72808d1 (crypto: ghash-clmulni-intel -
>> >> use C implementation for setkey()) that has been pulled by Linus
>> >> during the current merge window.
>> >>
>> >> It is missing two things:
>> >> - a cc to stable annotation
>> >> - a fix for the sparse warning below (change cast from __be64 to __force __be64)
>> >>
>> >> The reason for cc'ing stable on this patch is that it fixes a
>> >> potential data corruption issue where the ghash setkey() method uses
>> >> SSE registers without calling kernel_fpu_begin() first. This issue was
>> >> introduced by 0e1227d356e9b (crypto: ghash - Add PCLMULQDQ accelerated
>> >> implementation).
>> >>
>> >> So how would you like to proceed with this? Should I propose a new
>> >> patch somewhere?
>> >
>> > No problem, I'll apply this as-is.  But it doesn't apply to the
>> > 3.4-stable tree cleanly, can you send me a backported version if it's
>> > still needed there as well?
>> >
>>
>> Yes, the code was broken from the start. 3.4 version is attached, the
>> only difference is the missing ENDPROC() at the end of the asm file.
>
> Now applied, thanks.
>
>> In the mean time, Herbert has submitted a fix for the sparse warning,
>> but we settled on a different fix than I had suggested before.
>> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=0ea481466d1c
>>
>> Note that this code has not been tested (not by me, at least), so I
>> wouldn't suggest you take it straight away, but if you care about the
>> sparse warning, we could add a cc stable to it as well, I suppose.
>
> If it's a real bugfix that people can hit, then yse, I'll take it.  Just
> let me know when it hits Linus's tree.
>

Hardly. The only thing it fixes is a diagnostic related to the use of
be128 where u128 is more appropriate,  only this time, it changes the
type throughout the file rather than using a __force cast when
accessing the variable, as I did in my original fix.

-- 
Ard.