Re: [PATCH v6 10/12] crypto: x86/aes - Prepare for a new AES implementation

Chang S. Bae
2023-05-11T23:20:16Z

On 5/11/2023 2:39 PM, Eric Biggers wrote:
> On Thu, May 11, 2023 at 12:05:17PM -0700, Chang S. Bae wrote:
>> +
>> +struct aes_xts_ctx {
>> + struct crypto_aes_ctx tweak_ctx AES_ALIGN_ATTR;
>> + struct crypto_aes_ctx crypt_ctx AES_ALIGN_ATTR;
>> +};
>> +
>> +static inline struct crypto_aes_ctx *aes_ctx(void *raw_ctx)
>> +{
>> + unsigned long addr = (unsigned long)raw_ctx;
>> + unsigned long align = AES_ALIGN;
>> +
>> + if (align <= crypto_tfm_ctx_alignment())
>> + align = 1;
>> +
>> + return (struct crypto_aes_ctx *)ALIGN(addr, align);
>> +}
>
> It seems you took my suggestion to fix the definition of struct aes_xts_ctx, but
> you didn't make the corresponding change to the runtime alignment code.

Sigh. This particular change was unintentionally leaked here from my WIP
code. Yes, I'm aware of your comment:

> The runtime alignment to a 16-byte boundary should happen when
translating the
> raw crypto_skcipher_ctx() into the pointer to the aes_xts_ctx. It
should not
> happen when accessing each individual field in the aes_xts_ctx.

> There should be a helper function aes_xts_ctx() that is used like:
>
> struct aes_xts_ctx *ctx = aes_xts_ctx(tfm);
>
> It would do the runtime alignment. Then, aes_ctx() should be removed.

Yes, I could think of some changes like the one below. I guess the aeskl
code can live with it. The aesni glue code still wants aes_cts() as it
deals with other modes. Then, that can be left there as it is.

diff --git a/arch/x86/crypto/aes-intel_glue.h
b/arch/x86/crypto/aes-intel_glue.h
index 5877d0988e36..b22de77594fe 100644
--- a/arch/x86/crypto/aes-intel_glue.h
+++ b/arch/x86/crypto/aes-intel_glue.h
@@ -31,15 +31,15 @@ struct aes_xts_ctx {
struct crypto_aes_ctx crypt_ctx AES_ALIGN_ATTR;
};

-static inline struct crypto_aes_ctx *aes_ctx(void *raw_ctx)
+static inline struct aes_xts_ctx *aes_xts_ctx(struct crypto_skcipher *tfm)
{
- unsigned long addr = (unsigned long)raw_ctx;
+ unsigned long addr = (unsigned long)crypto_skcipher_ctx(tfm);
unsigned long align = AES_ALIGN;

if (align <= crypto_tfm_ctx_alignment())
align = 1;

- return (struct crypto_aes_ctx *)ALIGN(addr, align);
+ return (struct aes_xts_ctx *)ALIGN(addr, align);
}

static inline int
@@ -47,7 +47,7 @@ xts_setkey_common(struct crypto_skcipher *tfm, const
u8 *key, unsigned int keyle
int (*fn)(struct crypto_tfm *tfm, void *ctx, const u8
*in_key,
unsigned int key_len))
{
- struct aes_xts_ctx *ctx = crypto_skcipher_ctx(tfm);
+ struct aes_xts_ctx *ctx = aes_xts_ctx(tfm);
int err;

err = xts_verify_key(tfm, key, keylen);
@@ -72,7 +72,7 @@ xts_crypt_common(struct skcipher_request *req,
int (*crypt1_fn)(const void *ctx, u8 *out, const u8 *in))
{
struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
- struct aes_xts_ctx *ctx = crypto_skcipher_ctx(tfm);
+ struct aes_xts_ctx *ctx = aes_xts_ctx(tfm);
int tail = req->cryptlen % AES_BLOCK_SIZE;
struct skcipher_request subreq;
struct skcipher_walk walk;
@@ -108,7 +108,7 @@ xts_crypt_common(struct skcipher_request *req,
kernel_fpu_begin();

/* calculate first value of T */
- err = crypt1_fn(aes_ctx(&ctx->tweak_ctx), walk.iv, walk.iv);
+ err = crypt1_fn(&ctx->tweak_ctx, walk.iv, walk.iv);
if (err) {
kernel_fpu_end();
return err;
@@ -120,7 +120,7 @@ xts_crypt_common(struct skcipher_request *req,
if (nbytes < walk.total)
nbytes &= ~(AES_BLOCK_SIZE - 1);

- err = crypt_fn(aes_ctx(&ctx->crypt_ctx),
walk.dst.virt.addr, walk.src.virt.addr,
+ err = crypt_fn(&ctx->crypt_ctx, walk.dst.virt.addr,
walk.src.virt.addr,
nbytes, walk.iv);
kernel_fpu_end();
if (err)
@@ -148,7 +148,7 @@ xts_crypt_common(struct skcipher_request *req,
return err;

kernel_fpu_begin();
- err = crypt_fn(aes_ctx(&ctx->crypt_ctx),
walk.dst.virt.addr, walk.src.virt.addr,
+ err = crypt_fn(&ctx->crypt_ctx, walk.dst.virt.addr,
walk.src.virt.addr,
walk.nbytes, walk.iv);
kernel_fpu_end();
if (err)

Thanks,
Chang