| Message ID | CAKv+Gu-ZKVz7JKhW9iWuZeKiRnyS3Z4uakPn2_UMW_CpSBgXkw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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