Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 October 2018 at 04:01, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2018-10-22 at 19:19 -0300, Ard Biesheuvel wrote:
> [...]
>> > +static void hmac_init(struct shash_desc *desc, u8 *key, int
>> > keylen)
>> > +{
>> > +       u8 pad[SHA256_BLOCK_SIZE];
>> > +       int i;
>> > +
>> > +       desc->tfm = sha256_hash;
>> > +       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>>
>> I don't think this actually does anything in the shash API
>> implementation, so you can drop this.
>
> OK, I find crypto somewhat hard to follow.  There were bits I had to
> understand, like when I wrote the CFB implementation or when I fixed
> the ECDH scatterlist handling, but I've got to confess, in time
> honoured tradition I simply copied this from EVM crypto without
> actually digging into the code to understand why.
>

Yeah, it is notoriously hard to use, and we should try to improve that.

>>  However, I take it this means that hmac_init() is never called in
>> contexts where sleeping is not allowed? For the relevance of that,
>> please see below.
>
> Right, these routines are always called as an adjunct to a TPM
> operation and TPM operations can sleep, so we must at least have kernel
> thread context.
>
> [...]
>> > +       /* encrypt before HMAC */
>> > +       if (auth->attrs & TPM2_SA_DECRYPT) {
>> > +               struct scatterlist sg[1];
>> > +               u16 len;
>> > +               SKCIPHER_REQUEST_ON_STACK(req, auth->aes);
>> > +
>> > +               skcipher_request_set_tfm(req, auth->aes);
>> > +               skcipher_request_set_callback(req,
>> > CRYPTO_TFM_REQ_MAY_SLEEP,
>> > +                                             NULL, NULL);
>> > +
>>
>> Your crypto_alloc_skcipher() call [further down] does not mask out
>> CRYPTO_ALG_ASYNC, and so it permits asynchronous implementations to
>> be selected. Your generic cfb template only permits synchronous
>> implementations, since it wraps the cipher directly (which is always
>> synchronous). However, we have support in the tree for some
>> accelerators that implement cfb(aes), and those will return
>> -EINPROGRESS when invoking crypto_skcipher_en/decrypt(req), which you
>> are not set up to handle.
>>
>> So the simple solution is to call 'crypto_alloc_skcipher("cfb(aes)",
>> 0, CRYPTO_ALG_ASYNC)' below instead.
>>
>> However, I would prefer it if we permit asynchronous skciphers here.
>> The reason is that, on a given platform, the accelerator may be the
>> only truly time invariant AES implementation that is available, and
>> since we are dealing with the TPM, a bit of paranoia does not hurt.
>> It also makes it easier to implement it as a [time invariant] SIMD
>> based asynchronous skcipher, which are simpler than synchronous ones
>> since they don't require a non-SIMD fallback path for calls from
>> contexts where the SIMD unit may not be used.
>>
>> I have already implemented cfb(aes) for arm64/NEON. I will post the
>> patch after the merge window closes.
>>
>> > +               /* need key and IV */
>> > +               KDFa(auth->session_key, SHA256_DIGEST_SIZE
>> > +                    + auth->passphraselen, "CFB", auth->our_nonce,
>> > +                    auth->tpm_nonce, AES_KEYBYTES +
>> > AES_BLOCK_SIZE,
>> > +                    auth->scratch);
>> > +               crypto_skcipher_setkey(auth->aes, auth->scratch,
>> > AES_KEYBYTES);
>> > +               len = tpm_get_inc_u16(&p);
>> > +               sg_init_one(sg, p, len);
>> > +               skcipher_request_set_crypt(req, sg, sg, len,
>> > +                                          auth->scratch +
>> > AES_KEYBYTES);
>> > +               crypto_skcipher_encrypt(req);
>>
>> So please consider replacing this with something like.
>>
>> DECLARE_CRYPTO_WAIT(wait); [further up]
>> skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
>>                               crypto_req_done, &wait);
>> crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
>
> Sure, I can do this ... the crypto skcipher handling was also cut and
> paste, but I forget where from this time.  So what I think you're
> asking for is below as the incremental diff?  I've tested this out and
> it all works fine in my session testing environment (and on my real
> laptop) ... although since I'm fully sync, I won't really have tested
> the -EINPROGRESS do the wait case.
>

Yes that looks fine. I will try to test this on one of my arm64
systems, but it may take me some time to get around to it.

In any case,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # crypto API parts

>
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index 422c3ec64f8c..bbd3e8580954 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -165,7 +165,6 @@ static void hmac_init(struct shash_desc *desc, u8 *key, int keylen)
>         int i;
>
>         desc->tfm = sha256_hash;
> -       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>         crypto_shash_init(desc);
>         for (i = 0; i < sizeof(pad); i++) {
>                 if (i < keylen)
> @@ -244,7 +243,6 @@ static void KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v,
>         __be32 c = cpu_to_be32(1);
>
>         desc->tfm = sha256_hash;
> -       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>
>         crypto_shash_init(desc);
>         /* counter (BE) */
> @@ -445,7 +443,6 @@ void tpm_buf_fill_hmac_session(struct tpm_buf *buf, struct tpm2_auth *auth)
>         auth->ordinal = head->ordinal;
>
>         desc->tfm = sha256_hash;
> -       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>
>         cc = be32_to_cpu(head->ordinal);
>
> @@ -514,10 +511,11 @@ void tpm_buf_fill_hmac_session(struct tpm_buf *buf, struct tpm2_auth *auth)
>                 struct scatterlist sg[1];
>                 u16 len;
>                 SKCIPHER_REQUEST_ON_STACK(req, auth->aes);
> +               DECLARE_CRYPTO_WAIT(wait);
>
>                 skcipher_request_set_tfm(req, auth->aes);
>                 skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
> -                                             NULL, NULL);
> +                                             crypto_req_done, &wait);
>
>                 /* need key and IV */
>                 KDFa(auth->session_key, SHA256_DIGEST_SIZE
> @@ -529,7 +527,7 @@ void tpm_buf_fill_hmac_session(struct tpm_buf *buf, struct tpm2_auth *auth)
>                 sg_init_one(sg, p, len);
>                 skcipher_request_set_crypt(req, sg, sg, len,
>                                            auth->scratch + AES_KEYBYTES);
> -               crypto_skcipher_encrypt(req);
> +               crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
>                 /* reset p to beginning of parameters for HMAC */
>                 p -= 2;
>         }
> @@ -755,7 +753,6 @@ int tpm_buf_check_hmac_response(struct tpm_buf *buf, struct tpm2_auth *auth,
>          * with rphash
>          */
>         desc->tfm = sha256_hash;
> -       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>         crypto_shash_init(desc);
>         /* yes, I know this is now zero, but it's what the standard says */
>         crypto_shash_update(desc, (u8 *)&head->return_code,
> @@ -786,10 +783,11 @@ int tpm_buf_check_hmac_response(struct tpm_buf *buf, struct tpm2_auth *auth,
>         if (auth->attrs & TPM2_SA_ENCRYPT) {
>                 struct scatterlist sg[1];
>                 SKCIPHER_REQUEST_ON_STACK(req, auth->aes);
> +               DECLARE_CRYPTO_WAIT(wait);
>
>                 skcipher_request_set_tfm(req, auth->aes);
>                 skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
> -                                             NULL, NULL);
> +                                             crypto_req_done, &wait);
>
>                 /* need key and IV */
>                 KDFa(auth->session_key, SHA256_DIGEST_SIZE
> @@ -801,7 +799,7 @@ int tpm_buf_check_hmac_response(struct tpm_buf *buf, struct tpm2_auth *auth,
>                 sg_init_one(sg, p, len);
>                 skcipher_request_set_crypt(req, sg, sg, len,
>                                            auth->scratch + AES_KEYBYTES);
> -               crypto_skcipher_decrypt(req);
> +               crypto_wait_req(crypto_skcipher_decrypt(req), &wait);
>         }
>
>   out:
> @@ -965,7 +963,6 @@ static int parse_create_primary(struct tpm_chip *chip, u8 *data)
>         tmp = resp;
>         /* now we have the public area, compute the name of the object */
>         desc->tfm = sha256_hash;
> -       desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
>         put_unaligned_be16(TPM2_ALG_SHA256, chip->tpmkeyname);
>         crypto_shash_init(desc);
>         crypto_shash_update(desc, resp, len);