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

[Chang S. Bae]

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