crypto: x86/polyval - Fix crashes when keys are not 16-byte aligned

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 <[email protected]>

Signed-off-by Nathan Huckleberry <[email protected]>

---
 arch/x86/crypto/polyval-clmulni_asm.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/crypto/polyval-clmulni_asm.S +0 -0

index a6ebe4e7dd2b..32b98cb53ddf 100644

Quoted 17 lines · Nathan Huckleberry

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 <[email protected]>

Signed-off-by Nathan Huckleberry <[email protected]>

---
 arch/x86/crypto/polyval-clmulni_asm.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/crypto/polyval-clmulni_asm.S +0 -0

index a6ebe4e7dd2b..32b98cb53ddf 100644

arch/x86/crypto/polyval-clmulni_asm.S line 240

Quoted 8 lines

arch/x86/crypto/polyval-clmulni_asm.S +1 -1

@@ -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

Quoted 34 lines · Eric Biggers

Quoted 17 lines · Nathan Huckleberry

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 <[email protected]>

Signed-off-by Nathan Huckleberry <[email protected]>

---
 arch/x86/crypto/polyval-clmulni_asm.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/crypto/polyval-clmulni_asm.S +0 -0

index a6ebe4e7dd2b..32b98cb53ddf 100644

arch/x86/crypto/polyval-clmulni_asm.S line 240

Quoted 8 lines

arch/x86/crypto/polyval-clmulni_asm.S +1 -1

@@ -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.

Quoted 1 line

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.

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 <[email protected]>

Signed-off-by Nathan Huckleberry <[email protected]>

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 <[email protected]>

Signed-off-by Nathan Huckleberry <[email protected]>

---
 arch/x86/crypto/polyval-clmulni_glue.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

arch/x86/crypto/polyval-clmulni_glue.c +14 -6

index b7664d018851..8fa58b0f3cb3 100644
@@ -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

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 <[email protected]>

Signed-off-by Nathan Huckleberry <[email protected]>

---
 arch/x86/crypto/polyval-clmulni_glue.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)