On Wed, 2018-10-24 at 02:48 +0300, Jarkko Sakkinen wrote:
> On Mon, 22 Oct 2018, James Bottomley wrote:
> > [...]
I'll tidy up the descriptions.
> These all sould be combined with the existing session stuff inside
> tpm2-cmd.c and not have duplicate infrastructures. The file name
> should be tpm2-session.c (we neither have tpm2-cmds.c).
You mean move tpm2_buf_append_auth() into the new sessions file as well
... sure, I can do that.
[...]
> > +
> > +/*
> > + * assume hash sha256 and nonces u, v of size SHA256_DIGEST_SIZE
> > but
> > + * otherwise standard KDFa. Note output is in bytes not bits.
> > + */
> > +static void KDFa(u8 *key, int keylen, const char *label, u8 *u,
> > +u8 *v, int bytes, u8 *out)
>
> Should this be in lower case? I would rename it as tpm_kdfa().
This one is defined as KDFa() in the standards and it's not TPM
specific (although some standards refer to it as KDFA). I'm not averse
to making them tpm_kdfe() and tpm_kdfa() but I was hoping that one day
the crypto subsystem would need them and we could move them in there
because KDFs are the new shiny in crypto primitives (TLS 1.2 started
using them, for instance).
> > +{
> > + u32 counter;
> > + const __be32 bits = cpu_to_be32(bytes * 8);
> > +
> > + for (counter = 1; bytes > 0; bytes -= SHA256_DIGEST_SIZE,
> > counter++,
> > +out += SHA256_DIGEST_SIZE) {
>
> Only one counter is actually used for anything so this is overly
> complicated and IMHO it is ok to call the counter just 'i'. Maybe
> just:
>
> for (i = 1; (bytes - (i - 1) * SHA256_DIGEST_SIZE) > 0; i++) {
>
> > + SHASH_DESC_ON_STACK(desc, sha256_hash);
> > + __be32 c = cpu_to_be32(counter);
> > +
> > + hmac_init(desc, key, keylen);
> > + crypto_shash_update(desc, (u8 *), sizeof(c));
> > + crypto_shash_update(desc, label, strlen(label)+1);
> > + crypto_shash_update(desc, u, SHA256_DIGEST_SIZE);
> > + crypto_shash_update(desc, v, SHA256_DIGEST_SIZE);
> > + crypto_shash_update(desc, (u8 *),
> > sizeof(bits));
> > + hmac_final(desc, key, keylen, out);
> > + }
> > +}
> > +
> > +/*
> > + * Somewhat of a bastardization of the real KDFe. We're assuming
> > + * we're working with known point sizes for the input parameters
> > and
> > + * the hash algorithm is fixed at sha256. Because we know that
> > the
> > + * point size is 32 bytes like the hash size, there's no need to
> > loop
> > + * in this KDF.
> > + */
> > +static void KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8
> > *pt_v,
> > +u8 *keyout)
> > +{
> > + SHASH_DESC_ON_STACK(desc, sha256_hash);
> > + /*
> > +* this should be an iterative counter, but because we
> > know
> > +* we're only taking 32 bytes for the point using a
> > sha256
> > +* hash which is also 32 bytes, there's only one loop
> > +*/
> > + __be32 c = cpu_to_be32(1);
> > +
> > + desc->tfm = sha256_hash;
> > + desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > +
> > + crypto_shash_init(desc);
> > + /* counter (BE) */
> > + crypto_shash_update(desc, (u8 *), sizeof(c));
> > + /* secret value */
> > + crypto_shash_update(desc, z, EC_PT_SZ);
> > + /* string including trailing zero */
> > + crypto_shash_update(desc, str, strlen(str)+1);
> > + crypto_shash_update(desc, pt_u, EC_PT_SZ);
> > + crypto_shash_update(desc, pt_v, EC_PT_SZ);
> > + crypto_shash_final(desc, keyout);
> > +}
> > +
> > +static void tpm_buf_append_salt(struct tpm_buf *buf, struct
> > tpm_chip *chip,
> > + struct tpm2_auth *auth)
>
> Given the complexity of this function and some not that obvious
> choices in the implementation (coordinates), it would make sense to
> document this function.
I'll try to beef up the salting description
> > +{
> > + struct crypto_kpp *kpp;
> > + struct kpp_request *req;
> > + struct scatterlist s[2], d[1];
> > + struct ecdh p = {0};
> > + u8 encoded_key[EC_PT_SZ], *x, *y;
>
> Why you use one character variable name 'p' and longer name
> 'encoded_key'?
>
> > + unsigned int buf_len;
> > + u8 *secret;
> > +
> > + secret = kmalloc(EC_PT_SZ, GFP_KERNEL);
> > + if (!secret)
> > + return;
> > +
> > + p.curve_id = ECC_CURVE_NIST_P256;
>
> Could this be set already in the initialization?
I'm never sure about designated initializers, but I think, after
looking them up again, it will zero fill unmentioned elements.
> > +
> > + /* secret is two sized points */
> > + tpm_buf_append_u16(buf, (EC_PT_SZ + 2)*2);
>
> White space missing. Should be "(EC_PT_SZ + 2) * 2". The comment is a
> bit obscure (maybe, do not have any specific suggestion how to make
> it less obscure).
>
> > + /*
> > +* we cheat here and append uninitialized data to form
> > +* the points. All we care about is getting the two
> > +* co-ordinate pointers, which will be used to overwrite
> > +* the uninitialized data
> > +*/
>
>
