lkml.org 
[lkml]   [2023]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v7 12/12] crypto: x86/aes-kl - Implement the AES-XTS algorithm
    From
    On 5/26/2023 12:23 AM, Eric Biggers wrote:
    > On Wed, May 24, 2023 at 09:57:17AM -0700, Chang S. Bae wrote:
    >> == API Limitation ==
    >>
    >> The setkey() function transforms an AES key into a handle. But, an
    >> extended key is a usual outcome of setkey() in other AES cipher
    >> implementations. For this reason, a setkey() failure does not fall
    >> back to the other. So, expose AES-KL methods via synchronous
    >> interfaces only.
    >
    > I don't understand what this paragraph is trying to say.

    This text comes with this particular comment as I look back:

    > This basically implies that we cannot expose the cipher interface at
    > all, and so AES-KL can only be used by callers that use the
    > asynchronous interface, which rules out 802.11, s/w kTLS, macsec and
    > kerberos.

    https://lore.kernel.org/lkml/CAMj1kXGa4f21eH0mdxd1pQsZMUjUr1Btq+Dgw-gC=O-yYft7xw@mail.gmail.com/

    Then, I realize that at that moment the dm-crypt use model was not
    clearly out yet.

    This seems to be carried over the versions. But, now, it has XTS only.
    Then, this becomes less relevant which makes confusion I guess.

    I think this can go away as claiming the usage clearly now.

    >
    >> +/*
    >> + * The below wrappers for the encryption/decryption functions
    >> + * incorporate the feature availability check:
    >> + *
    >> + * In the rare event of hardware failure, the wrapping key can be lost
    >> + * after wake-up from a deep sleep state. Then, this check helps to
    >> + * avoid any subsequent misuse with populating a proper error code.
    >> + */
    >> +
    >> +static inline int aeskl_enc(const void *ctx, u8 *out, const u8 *in)
    >> +{
    >> + if (!valid_keylocker())
    >> + return -ENODEV;
    >> +
    >> + return __aeskl_enc(ctx, out, in);
    >> +}
    >
    > Is it not sufficient for the valid_keylocker() check to occur at the top level
    > (in xts_encrypt() and xts_decrypt()), which would seem to be a better place to
    > do it? Is this because valid_keylocker() needs to be checked in every
    > kernel_fpu_begin() section separately, to avoid a race condition? If that's
    > indeed the reason, can you explain that in the comment?

    Maybe something like this:

    /*
    * In the event of hardware failure, the wrapping key can be lost
    * from a sleep state. Then, the feature is not usable anymore. This
    * feature state can be found via valid_keylocker().
    *
    * Such disabling could be anywhere preemptible, outside
    * kernel_fpu_begin()/end(). So, to avoid the race condition, check
    * the feature availability on every use in the below wrappers.
    */

    >
    >> +static inline int xts_keylen(struct skcipher_request *req, u32 *keylen)
    >> +{
    >> + struct aes_xts_ctx *ctx = aes_xts_ctx(crypto_skcipher_reqtfm(req));
    >> +
    >> + if (ctx->crypt_ctx.key_length != ctx->tweak_ctx.key_length)
    >> + return -EINVAL;
    >> +
    >> + *keylen = ctx->crypt_ctx.key_length;
    >> + return 0;
    >> +}
    >
    > This is odd. Why would the key lengths be different here?

    I thought it was logical to do such sanity check. But, in practice, they
    are the same.

    Looks like this entire crypto code is treated as performance-critical or so.

    >
    >> + err = simd_register_skciphers_compat(aeskl_skciphers, ARRAY_SIZE(aeskl_skciphers),
    >> + aeskl_simd_skciphers);
    >> + if (err)
    >> + return err;
    >> +
    >> + return 0;
    >
    > This can be simplified to:
    >
    > return simd_register_skciphers_compat(aeskl_skciphers,
    > ARRAY_SIZE(aeskl_skciphers),
    > aeskl_simd_skciphers);

    Oh, obviously!

    Thanks,
    Chang

    \
     
     \ /
      Last update: 2023-05-30 22:51    [W:3.274 / U:0.424 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site