* [PATCH] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
@ 2022-10-17 22:26 Nathan Huckleberry
2022-10-17 23:02 ` Eric Biggers
0 siblings, 1 reply; 11+ messages in thread
From: Nathan Huckleberry @ 2022-10-17 22:26 UTC (permalink / raw)
Cc: Nathan Huckleberry, Bruno Goncalves, Herbert Xu, David S. Miller,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Eric Biggers, Ard Biesheuvel, linux-crypto,
linux-kernel
The key_powers array is not guaranteed to be 16-byte aligned, so using
movaps to operate on key_powers is not allowed.
Switch movaps to movups.
Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
arch/x86/crypto/polyval-clmulni_asm.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/crypto/polyval-clmulni_asm.S b/arch/x86/crypto/polyval-clmulni_asm.S
index a6ebe4e7dd2b..32b98cb53ddf 100644
--- a/arch/x86/crypto/polyval-clmulni_asm.S
+++ b/arch/x86/crypto/polyval-clmulni_asm.S
@@ -234,7 +234,7 @@
movups (MSG), %xmm0
pxor SUM, %xmm0
- movaps (KEY_POWERS), %xmm1
+ movups (KEY_POWERS), %xmm1
schoolbook1_noload
dec BLOCKS_LEFT
addq $16, MSG
--
2.38.0.413.g74048e4d9e-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-17 22:26 [PATCH] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned Nathan Huckleberry
@ 2022-10-17 23:02 ` Eric Biggers
2022-10-17 23:38 ` Nathan Huckleberry
0 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2022-10-17 23:02 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: Bruno Goncalves, Herbert Xu, David S. Miller, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Ard Biesheuvel, linux-crypto, linux-kernel
On Mon, Oct 17, 2022 at 03:26:20PM -0700, Nathan Huckleberry wrote:
> The key_powers array is not guaranteed to be 16-byte aligned, so using
> movaps to operate on key_powers is not allowed.
>
> Switch movaps to movups.
>
> Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
> Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> ---
> arch/x86/crypto/polyval-clmulni_asm.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/crypto/polyval-clmulni_asm.S b/arch/x86/crypto/polyval-clmulni_asm.S
> index a6ebe4e7dd2b..32b98cb53ddf 100644
> --- a/arch/x86/crypto/polyval-clmulni_asm.S
> +++ b/arch/x86/crypto/polyval-clmulni_asm.S
> @@ -234,7 +234,7 @@
>
> movups (MSG), %xmm0
> pxor SUM, %xmm0
> - movaps (KEY_POWERS), %xmm1
> + movups (KEY_POWERS), %xmm1
> schoolbook1_noload
> dec BLOCKS_LEFT
> addq $16, MSG
I thought that crypto_tfm::__crt_ctx is guaranteed to be 16-byte aligned,
and that the x86 AES code relies on that property.
But now I see that actually the x86 AES code manually aligns the context.
See aes_ctx() in arch/x86/crypto/aesni-intel_glue.c.
Did you consider doing the same for polyval?
If you do prefer this way, it would be helpful to leave a comment for
schoolbook1_iteration that mentions that the unaligned access support of
vpclmulqdq is being relied on, i.e. pclmulqdq wouldn't work.
- Eric
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-17 23:02 ` Eric Biggers
@ 2022-10-17 23:38 ` Nathan Huckleberry
2022-10-18 0:12 ` Eric Biggers
0 siblings, 1 reply; 11+ messages in thread
From: Nathan Huckleberry @ 2022-10-17 23:38 UTC (permalink / raw)
To: Eric Biggers
Cc: Bruno Goncalves, Herbert Xu, David S. Miller, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Ard Biesheuvel, linux-crypto, linux-kernel
On Mon, Oct 17, 2022 at 4:02 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Mon, Oct 17, 2022 at 03:26:20PM -0700, Nathan Huckleberry wrote:
> > The key_powers array is not guaranteed to be 16-byte aligned, so using
> > movaps to operate on key_powers is not allowed.
> >
> > Switch movaps to movups.
> >
> > Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
> > Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
> > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > ---
> > arch/x86/crypto/polyval-clmulni_asm.S | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/crypto/polyval-clmulni_asm.S b/arch/x86/crypto/polyval-clmulni_asm.S
> > index a6ebe4e7dd2b..32b98cb53ddf 100644
> > --- a/arch/x86/crypto/polyval-clmulni_asm.S
> > +++ b/arch/x86/crypto/polyval-clmulni_asm.S
> > @@ -234,7 +234,7 @@
> >
> > movups (MSG), %xmm0
> > pxor SUM, %xmm0
> > - movaps (KEY_POWERS), %xmm1
> > + movups (KEY_POWERS), %xmm1
> > schoolbook1_noload
> > dec BLOCKS_LEFT
> > addq $16, MSG
>
> I thought that crypto_tfm::__crt_ctx is guaranteed to be 16-byte aligned,
> and that the x86 AES code relies on that property.
>
> But now I see that actually the x86 AES code manually aligns the context.
> See aes_ctx() in arch/x86/crypto/aesni-intel_glue.c.
>
> Did you consider doing the same for polyval?
I'll submit a v2 aligning the tfm_ctx. I think that makes more sense
than working on unaligned keys.
Is there a need to do the same changes on arm64? The keys are also
unaligned there.
>
> If you do prefer this way, it would be helpful to leave a comment for
> schoolbook1_iteration that mentions that the unaligned access support of
> vpclmulqdq is being relied on, i.e. pclmulqdq wouldn't work.
>
> - Eric
Thanks,
Huck
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-17 23:38 ` Nathan Huckleberry
@ 2022-10-18 0:12 ` Eric Biggers
2022-10-18 4:03 ` Herbert Xu
0 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2022-10-18 0:12 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: Bruno Goncalves, Herbert Xu, David S. Miller, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Ard Biesheuvel, linux-crypto, linux-kernel
On Mon, Oct 17, 2022 at 04:38:25PM -0700, Nathan Huckleberry wrote:
> On Mon, Oct 17, 2022 at 4:02 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Mon, Oct 17, 2022 at 03:26:20PM -0700, Nathan Huckleberry wrote:
> > > The key_powers array is not guaranteed to be 16-byte aligned, so using
> > > movaps to operate on key_powers is not allowed.
> > >
> > > Switch movaps to movups.
> > >
> > > Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
> > > Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
> > > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > > ---
> > > arch/x86/crypto/polyval-clmulni_asm.S | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/crypto/polyval-clmulni_asm.S b/arch/x86/crypto/polyval-clmulni_asm.S
> > > index a6ebe4e7dd2b..32b98cb53ddf 100644
> > > --- a/arch/x86/crypto/polyval-clmulni_asm.S
> > > +++ b/arch/x86/crypto/polyval-clmulni_asm.S
> > > @@ -234,7 +234,7 @@
> > >
> > > movups (MSG), %xmm0
> > > pxor SUM, %xmm0
> > > - movaps (KEY_POWERS), %xmm1
> > > + movups (KEY_POWERS), %xmm1
> > > schoolbook1_noload
> > > dec BLOCKS_LEFT
> > > addq $16, MSG
> >
> > I thought that crypto_tfm::__crt_ctx is guaranteed to be 16-byte aligned,
> > and that the x86 AES code relies on that property.
> >
> > But now I see that actually the x86 AES code manually aligns the context.
> > See aes_ctx() in arch/x86/crypto/aesni-intel_glue.c.
> >
> > Did you consider doing the same for polyval?
>
> I'll submit a v2 aligning the tfm_ctx. I think that makes more sense
> than working on unaligned keys.
>
> Is there a need to do the same changes on arm64? The keys are also
> unaligned there.
>
arm64 defines ARCH_DMA_MINALIGN to 128, so I don't think the same issue applies
there. Also the instructions used don't assume aligned addresses.
- Eric
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-18 0:12 ` Eric Biggers
@ 2022-10-18 4:03 ` Herbert Xu
2022-10-18 21:56 ` [PATCH v2] " Nathan Huckleberry
0 siblings, 1 reply; 11+ messages in thread
From: Herbert Xu @ 2022-10-18 4:03 UTC (permalink / raw)
To: Eric Biggers
Cc: Nathan Huckleberry, Bruno Goncalves, David S. Miller,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Ard Biesheuvel, linux-crypto, linux-kernel
On Mon, Oct 17, 2022 at 05:12:16PM -0700, Eric Biggers wrote:
>
> arm64 defines ARCH_DMA_MINALIGN to 128, so I don't think the same issue applies
> there. Also the instructions used don't assume aligned addresses.
Note that arm64 may lower ARCH_KMALLOC_MINALIGN to 8 soon.
Cheers,
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-18 4:03 ` Herbert Xu
@ 2022-10-18 21:56 ` Nathan Huckleberry
2022-10-18 22:39 ` Eric Biggers
0 siblings, 1 reply; 11+ messages in thread
From: Nathan Huckleberry @ 2022-10-18 21:56 UTC (permalink / raw)
To: herbert
Cc: ardb, bgoncalv, bp, dave.hansen, davem, ebiggers, hpa,
linux-crypto, linux-kernel, mingo, nhuck, tglx, x86
crypto_tfm::__crt_ctx is not guaranteed to be 16-byte aligned on x86-64.
This causes crashes due to movaps instructions in clmul_polyval_update.
Add logic to align polyval_tfm_ctx to 16 bytes if required.
Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
arch/x86/crypto/polyval-clmulni_glue.c | 29 ++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/arch/x86/crypto/polyval-clmulni_glue.c b/arch/x86/crypto/polyval-clmulni_glue.c
index b7664d018851..79ad497c32b5 100644
--- a/arch/x86/crypto/polyval-clmulni_glue.c
+++ b/arch/x86/crypto/polyval-clmulni_glue.c
@@ -27,13 +27,17 @@
#include <asm/cpu_device_id.h>
#include <asm/simd.h>
+#define POLYVAL_ALIGN 16
+#define POLYVAL_ALIGN_ATTR __aligned(POLYVAL_ALIGN)
+#define POLYVAL_ALIGN_EXTRA ((POLYVAL_ALIGN - 1) & ~(CRYPTO_MINALIGN - 1))
+#define POLYVAL_CTX_SIZE (sizeof(struct polyval_tfm_ctx) + POLYVAL_ALIGN_EXTRA)
#define NUM_KEY_POWERS 8
struct polyval_tfm_ctx {
/*
* These powers must be in the order h^8, ..., h^1.
*/
- u8 key_powers[NUM_KEY_POWERS][POLYVAL_BLOCK_SIZE];
+ u8 key_powers[NUM_KEY_POWERS][POLYVAL_BLOCK_SIZE] POLYVAL_ALIGN_ATTR;
};
struct polyval_desc_ctx {
@@ -45,9 +49,20 @@ asmlinkage void clmul_polyval_update(const struct polyval_tfm_ctx *keys,
const u8 *in, size_t nblocks, u8 *accumulator);
asmlinkage void clmul_polyval_mul(u8 *op1, const u8 *op2);
-static void internal_polyval_update(const struct polyval_tfm_ctx *keys,
+static inline struct polyval_tfm_ctx *polyval_tfm_ctx(const void *raw_ctx)
+{
+ unsigned long addr = (unsigned long)raw_ctx;
+ unsigned long align = POLYVAL_ALIGN;
+
+ if (align <= crypto_tfm_ctx_alignment())
+ align = 1;
+ return (struct polyval_tfm_ctx *)ALIGN(addr, align);
+}
+
+static void internal_polyval_update(const void *raw_keys,
const u8 *in, size_t nblocks, u8 *accumulator)
{
+ const struct polyval_tfm_ctx *keys = polyval_tfm_ctx(raw_keys);
if (likely(crypto_simd_usable())) {
kernel_fpu_begin();
clmul_polyval_update(keys, in, nblocks, accumulator);
@@ -72,7 +87,7 @@ static void internal_polyval_mul(u8 *op1, const u8 *op2)
static int polyval_x86_setkey(struct crypto_shash *tfm,
const u8 *key, unsigned int keylen)
{
- struct polyval_tfm_ctx *tctx = crypto_shash_ctx(tfm);
+ struct polyval_tfm_ctx *tctx = polyval_tfm_ctx(crypto_shash_ctx(tfm));
int i;
if (keylen != POLYVAL_BLOCK_SIZE)
@@ -102,7 +117,8 @@ static int polyval_x86_update(struct shash_desc *desc,
const u8 *src, unsigned int srclen)
{
struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
- const struct polyval_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
+ const struct polyval_tfm_ctx *tctx =
+ polyval_tfm_ctx(crypto_shash_ctx(desc->tfm));
u8 *pos;
unsigned int nblocks;
unsigned int n;
@@ -143,7 +159,8 @@ static int polyval_x86_update(struct shash_desc *desc,
static int polyval_x86_final(struct shash_desc *desc, u8 *dst)
{
struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
- const struct polyval_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
+ const struct polyval_tfm_ctx *tctx =
+ polyval_tfm_ctx(crypto_shash_ctx(desc->tfm));
if (dctx->bytes) {
internal_polyval_mul(dctx->buffer,
@@ -167,7 +184,7 @@ static struct shash_alg polyval_alg = {
.cra_driver_name = "polyval-clmulni",
.cra_priority = 200,
.cra_blocksize = POLYVAL_BLOCK_SIZE,
- .cra_ctxsize = sizeof(struct polyval_tfm_ctx),
+ .cra_ctxsize = POLYVAL_CTX_SIZE,
.cra_module = THIS_MODULE,
},
};
--
2.38.0.413.g74048e4d9e-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-18 21:56 ` [PATCH v2] " Nathan Huckleberry
@ 2022-10-18 22:39 ` Eric Biggers
2022-10-18 23:04 ` [PATCH v3] " Nathan Huckleberry
0 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2022-10-18 22:39 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: herbert, ardb, bgoncalv, bp, dave.hansen, davem, hpa,
linux-crypto, linux-kernel, mingo, tglx, x86
On Tue, Oct 18, 2022 at 02:56:23PM -0700, Nathan Huckleberry wrote:
> -static void internal_polyval_update(const struct polyval_tfm_ctx *keys,
> +static inline struct polyval_tfm_ctx *polyval_tfm_ctx(const void *raw_ctx)
> +{
> + unsigned long addr = (unsigned long)raw_ctx;
> + unsigned long align = POLYVAL_ALIGN;
> +
> + if (align <= crypto_tfm_ctx_alignment())
> + align = 1;
> + return (struct polyval_tfm_ctx *)ALIGN(addr, align);
> +}
This could just use PTR_ALIGN. Also, checking for POLYVAL_ALIGN <=
crypto_tfm_ctx_alignment() isn't necessary.
> +
> +static void internal_polyval_update(const void *raw_keys,
> const u8 *in, size_t nblocks, u8 *accumulator)
> {
> + const struct polyval_tfm_ctx *keys = polyval_tfm_ctx(raw_keys);
This is being passed a struct polyval_tfm_ctx. There's no need to cast it back
to a void pointer and align it again redundantly.
> if (likely(crypto_simd_usable())) {
> kernel_fpu_begin();
> clmul_polyval_update(keys, in, nblocks, accumulator);
> @@ -102,7 +117,8 @@ static int polyval_x86_update(struct shash_desc *desc,
> const u8 *src, unsigned int srclen)
> {
> struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
> - const struct polyval_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
> + const struct polyval_tfm_ctx *tctx =
> + polyval_tfm_ctx(crypto_shash_ctx(desc->tfm));
> u8 *pos;
> unsigned int nblocks;
> unsigned int n;
It would make more sense to have the polyval_tfm_ctx() function take in the
struct crypto_shash.
How about using the following:
static inline struct polyval_tfm_ctx *polyval_tfm_ctx(struct crypto_shash *tfm)
{
return PTR_ALIGN(crypto_shash_ctx(tfm), POLYVAL_ALIGN);
}
- Eric
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-18 22:39 ` Eric Biggers
@ 2022-10-18 23:04 ` Nathan Huckleberry
2022-10-18 23:12 ` Eric Biggers
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Nathan Huckleberry @ 2022-10-18 23:04 UTC (permalink / raw)
To: ebiggers
Cc: ardb, bgoncalv, bp, dave.hansen, davem, herbert, hpa,
linux-crypto, linux-kernel, mingo, nhuck, tglx, x86
crypto_tfm::__crt_ctx is not guaranteed to be 16-byte aligned on x86-64.
This causes crashes due to movaps instructions in clmul_polyval_update.
Add logic to align polyval_tfm_ctx to 16 bytes.
Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
arch/x86/crypto/polyval-clmulni_glue.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/arch/x86/crypto/polyval-clmulni_glue.c b/arch/x86/crypto/polyval-clmulni_glue.c
index b7664d018851..8fa58b0f3cb3 100644
--- a/arch/x86/crypto/polyval-clmulni_glue.c
+++ b/arch/x86/crypto/polyval-clmulni_glue.c
@@ -27,13 +27,17 @@
#include <asm/cpu_device_id.h>
#include <asm/simd.h>
+#define POLYVAL_ALIGN 16
+#define POLYVAL_ALIGN_ATTR __aligned(POLYVAL_ALIGN)
+#define POLYVAL_ALIGN_EXTRA ((POLYVAL_ALIGN - 1) & ~(CRYPTO_MINALIGN - 1))
+#define POLYVAL_CTX_SIZE (sizeof(struct polyval_tfm_ctx) + POLYVAL_ALIGN_EXTRA)
#define NUM_KEY_POWERS 8
struct polyval_tfm_ctx {
/*
* These powers must be in the order h^8, ..., h^1.
*/
- u8 key_powers[NUM_KEY_POWERS][POLYVAL_BLOCK_SIZE];
+ u8 key_powers[NUM_KEY_POWERS][POLYVAL_BLOCK_SIZE] POLYVAL_ALIGN_ATTR;
};
struct polyval_desc_ctx {
@@ -45,6 +49,11 @@ asmlinkage void clmul_polyval_update(const struct polyval_tfm_ctx *keys,
const u8 *in, size_t nblocks, u8 *accumulator);
asmlinkage void clmul_polyval_mul(u8 *op1, const u8 *op2);
+static inline struct polyval_tfm_ctx *polyval_tfm_ctx(struct crypto_shash *tfm)
+{
+ return PTR_ALIGN(crypto_shash_ctx(tfm), POLYVAL_ALIGN);
+}
+
static void internal_polyval_update(const struct polyval_tfm_ctx *keys,
const u8 *in, size_t nblocks, u8 *accumulator)
{
@@ -72,7 +81,7 @@ static void internal_polyval_mul(u8 *op1, const u8 *op2)
static int polyval_x86_setkey(struct crypto_shash *tfm,
const u8 *key, unsigned int keylen)
{
- struct polyval_tfm_ctx *tctx = crypto_shash_ctx(tfm);
+ struct polyval_tfm_ctx *tctx = polyval_tfm_ctx(tfm);
int i;
if (keylen != POLYVAL_BLOCK_SIZE)
@@ -102,7 +111,7 @@ static int polyval_x86_update(struct shash_desc *desc,
const u8 *src, unsigned int srclen)
{
struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
- const struct polyval_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
+ const struct polyval_tfm_ctx *tctx = polyval_tfm_ctx(desc->tfm);
u8 *pos;
unsigned int nblocks;
unsigned int n;
@@ -143,7 +152,7 @@ static int polyval_x86_update(struct shash_desc *desc,
static int polyval_x86_final(struct shash_desc *desc, u8 *dst)
{
struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
- const struct polyval_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
+ const struct polyval_tfm_ctx *tctx = polyval_tfm_ctx(desc->tfm);
if (dctx->bytes) {
internal_polyval_mul(dctx->buffer,
@@ -167,7 +176,7 @@ static struct shash_alg polyval_alg = {
.cra_driver_name = "polyval-clmulni",
.cra_priority = 200,
.cra_blocksize = POLYVAL_BLOCK_SIZE,
- .cra_ctxsize = sizeof(struct polyval_tfm_ctx),
+ .cra_ctxsize = POLYVAL_CTX_SIZE,
.cra_module = THIS_MODULE,
},
};
--
2.38.0.413.g74048e4d9e-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-18 23:04 ` [PATCH v3] " Nathan Huckleberry
@ 2022-10-18 23:12 ` Eric Biggers
2022-10-19 12:13 ` Bruno Goncalves
2022-10-21 11:39 ` Herbert Xu
2 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2022-10-18 23:12 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: ardb, bgoncalv, bp, dave.hansen, davem, herbert, hpa,
linux-crypto, linux-kernel, mingo, tglx, x86
On Tue, Oct 18, 2022 at 04:04:12PM -0700, Nathan Huckleberry wrote:
> crypto_tfm::__crt_ctx is not guaranteed to be 16-byte aligned on x86-64.
> This causes crashes due to movaps instructions in clmul_polyval_update.
>
> Add logic to align polyval_tfm_ctx to 16 bytes.
>
> Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
> Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
Reviewed-by: Eric Biggers <ebiggers@google.com>
Please add 'Cc: stable@vger.kernel.org' as well. (Herbert can do it when
applying this patch, if you don't happen to send another version.)
- Eric
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-18 23:04 ` [PATCH v3] " Nathan Huckleberry
2022-10-18 23:12 ` Eric Biggers
@ 2022-10-19 12:13 ` Bruno Goncalves
2022-10-21 11:39 ` Herbert Xu
2 siblings, 0 replies; 11+ messages in thread
From: Bruno Goncalves @ 2022-10-19 12:13 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: ebiggers, ardb, bp, dave.hansen, davem, herbert, hpa,
linux-crypto, linux-kernel, mingo, tglx, x86
On Wed, 19 Oct 2022 at 01:04, Nathan Huckleberry <nhuck@google.com> wrote:
>
> crypto_tfm::__crt_ctx is not guaranteed to be 16-byte aligned on x86-64.
> This causes crashes due to movaps instructions in clmul_polyval_update.
>
> Add logic to align polyval_tfm_ctx to 16 bytes.
>
> Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
> Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> ---
> arch/x86/crypto/polyval-clmulni_glue.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/crypto/polyval-clmulni_glue.c b/arch/x86/crypto/polyval-clmulni_glue.c
> index b7664d018851..8fa58b0f3cb3 100644
> --- a/arch/x86/crypto/polyval-clmulni_glue.c
> +++ b/arch/x86/crypto/polyval-clmulni_glue.c
> @@ -27,13 +27,17 @@
> #include <asm/cpu_device_id.h>
> #include <asm/simd.h>
>
> +#define POLYVAL_ALIGN 16
> +#define POLYVAL_ALIGN_ATTR __aligned(POLYVAL_ALIGN)
> +#define POLYVAL_ALIGN_EXTRA ((POLYVAL_ALIGN - 1) & ~(CRYPTO_MINALIGN - 1))
> +#define POLYVAL_CTX_SIZE (sizeof(struct polyval_tfm_ctx) + POLYVAL_ALIGN_EXTRA)
> #define NUM_KEY_POWERS 8
>
> struct polyval_tfm_ctx {
> /*
> * These powers must be in the order h^8, ..., h^1.
> */
> - u8 key_powers[NUM_KEY_POWERS][POLYVAL_BLOCK_SIZE];
> + u8 key_powers[NUM_KEY_POWERS][POLYVAL_BLOCK_SIZE] POLYVAL_ALIGN_ATTR;
> };
>
> struct polyval_desc_ctx {
> @@ -45,6 +49,11 @@ asmlinkage void clmul_polyval_update(const struct polyval_tfm_ctx *keys,
> const u8 *in, size_t nblocks, u8 *accumulator);
> asmlinkage void clmul_polyval_mul(u8 *op1, const u8 *op2);
>
> +static inline struct polyval_tfm_ctx *polyval_tfm_ctx(struct crypto_shash *tfm)
> +{
> + return PTR_ALIGN(crypto_shash_ctx(tfm), POLYVAL_ALIGN);
> +}
> +
> static void internal_polyval_update(const struct polyval_tfm_ctx *keys,
> const u8 *in, size_t nblocks, u8 *accumulator)
> {
> @@ -72,7 +81,7 @@ static void internal_polyval_mul(u8 *op1, const u8 *op2)
> static int polyval_x86_setkey(struct crypto_shash *tfm,
> const u8 *key, unsigned int keylen)
> {
> - struct polyval_tfm_ctx *tctx = crypto_shash_ctx(tfm);
> + struct polyval_tfm_ctx *tctx = polyval_tfm_ctx(tfm);
> int i;
>
> if (keylen != POLYVAL_BLOCK_SIZE)
> @@ -102,7 +111,7 @@ static int polyval_x86_update(struct shash_desc *desc,
> const u8 *src, unsigned int srclen)
> {
> struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
> - const struct polyval_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
> + const struct polyval_tfm_ctx *tctx = polyval_tfm_ctx(desc->tfm);
> u8 *pos;
> unsigned int nblocks;
> unsigned int n;
> @@ -143,7 +152,7 @@ static int polyval_x86_update(struct shash_desc *desc,
> static int polyval_x86_final(struct shash_desc *desc, u8 *dst)
> {
> struct polyval_desc_ctx *dctx = shash_desc_ctx(desc);
> - const struct polyval_tfm_ctx *tctx = crypto_shash_ctx(desc->tfm);
> + const struct polyval_tfm_ctx *tctx = polyval_tfm_ctx(desc->tfm);
>
> if (dctx->bytes) {
> internal_polyval_mul(dctx->buffer,
> @@ -167,7 +176,7 @@ static struct shash_alg polyval_alg = {
> .cra_driver_name = "polyval-clmulni",
> .cra_priority = 200,
> .cra_blocksize = POLYVAL_BLOCK_SIZE,
> - .cra_ctxsize = sizeof(struct polyval_tfm_ctx),
> + .cra_ctxsize = POLYVAL_CTX_SIZE,
> .cra_module = THIS_MODULE,
> },
> };
> --
> 2.38.0.413.g74048e4d9e-goog
>
Thanks, this patch worked well for me.
Bruno
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned
2022-10-18 23:04 ` [PATCH v3] " Nathan Huckleberry
2022-10-18 23:12 ` Eric Biggers
2022-10-19 12:13 ` Bruno Goncalves
@ 2022-10-21 11:39 ` Herbert Xu
2 siblings, 0 replies; 11+ messages in thread
From: Herbert Xu @ 2022-10-21 11:39 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: ebiggers, ardb, bgoncalv, bp, dave.hansen, davem, hpa,
linux-crypto, linux-kernel, mingo, tglx, x86
On Tue, Oct 18, 2022 at 04:04:12PM -0700, Nathan Huckleberry wrote:
> crypto_tfm::__crt_ctx is not guaranteed to be 16-byte aligned on x86-64.
> This causes crashes due to movaps instructions in clmul_polyval_update.
>
> Add logic to align polyval_tfm_ctx to 16 bytes.
>
> Fixes: 34f7f6c30112 ("crypto: x86/polyval - Add PCLMULQDQ accelerated implementation of POLYVAL")
> Reported-by: Bruno Goncalves <bgoncalv@redhat.com>
> Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> ---
> arch/x86/crypto/polyval-clmulni_glue.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
Patch applied. 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-10-21 11:40 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-17 22:26 [PATCH] crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned Nathan Huckleberry
2022-10-17 23:02 ` Eric Biggers
2022-10-17 23:38 ` Nathan Huckleberry
2022-10-18 0:12 ` Eric Biggers
2022-10-18 4:03 ` Herbert Xu
2022-10-18 21:56 ` [PATCH v2] " Nathan Huckleberry
2022-10-18 22:39 ` Eric Biggers
2022-10-18 23:04 ` [PATCH v3] " Nathan Huckleberry
2022-10-18 23:12 ` Eric Biggers
2022-10-19 12:13 ` Bruno Goncalves
2022-10-21 11:39 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).