Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Wed, 24 Oct 2018, James Bottomley wrote:
+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).
I care more about tracing and debugging than naming and having 'tpm_' in
front of every TPM function makes tracing a lean process. AFAIK using
upper case letters is against kernel coding conventions. I'm not sure
why this would make an exception on that.
Why doesn't it matter here?
Because, as the comment says, it eventually gets overwritten by running
ecdh to derive the two co-ordinates. (pointers to these two
uninitialized areas are passed into the ecdh destination sg list).
Oh, I just misunderstood the comment. Wouldn't it be easier to say that
the data is initialized later?
+ buf_len = crypto_ecdh_key_len();
+ if (sizeof(encoded_key) < buf_len) {
+ dev_err(>dev, "salt buffer too small needs
%d\n",
+ buf_len);
+ goto out;
+ }
In what situation this can happen? Can sizeof(encoded_key) >=
buf_len?
Yes, but only if someone is trying to crack your ecdh. One of the
security issues in ecdh is if someone makes a very specific point
choice (usually in the cofactor space) that has a very short period,
the attacker can guess the input to KDFe. In this case if TPM genie
provided a specially crafted attack EC point, we'd detect it here
because the resulting buffer would be too short.
Right. Thank you for the explanation. Here some kind of comment might
not be a bad idea...
In general this function should have a clear explanation what it does
and maybe less these one character variables but instead variables
with more documenting names. Explain in high level what algorithms
are used and how the salt is calculated.
I'll try, but this is a rather complex function.
Understood. I do not expect perfection here and we can improve
documetation later on.
For anyone wanting to review James' patches and w/o much experience on
EC, I recommend reading this article:
https://arstechnica.com/information-technology/2013/10/a-relatively-easy-to-understand-primer-on-elliptic-curve-cryptography/
I read it few years ago and refreshed my memory few days ago by
re-reading it.
+
+/**
+ * tpm_buf_append_hmac_session() append a TPM session element
+ * @buf: The buffer to be appended
+ * @auth: the auth structure allocated by
tpm2_start_auth_session()
+ * @attributes: The session attributes
+ * @passphrase: The session authority (NULL if none)
+ * @passphraselen: The length of the session authority (0 if none)
The alignment.
the alignment of what?
We generally have parameter descriptions tab-aligned.
Why there would be trailing zeros?
Because TPM 1.2 mandated zero padded fixed size passphrases so the TPM
2.0 standard specifies a way of converting these to variable size
strings by eliminating the zero padding.
Ok.
James
I'm also looking forward for the CONTEXT_GAP patch based on the
yesterdays discussion. We do want it and I was stupid not to take it
couple years ago :-) Thanks.
/Jarkko
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
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
> > +*/
>
>
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Tue, 23 Oct 2018, Ard Biesheuvel wrote:
On 23 October 2018 at 04:01, James Bottomley
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.
James,
I would hope (already said in my review) to use longer than one
character variable names for most of the stuff. I did not quite
understand why you decided to use 'counter' for obvious counter
variable and one character names for non-obvious stuff :-)
I'm not sure where the 'encoded' exactly comes in the variable
name 'encoded_key' especially in the context of these cryptic
names.
/Jarkko
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Mon, 22 Oct 2018, James Bottomley wrote: This code adds true session based HMAC authentication plus parameter decryption and response encryption using AES. In order to reduce complexity it would make sense to split into two commits: authentication and parameter encryption. The basic design of this code is to segregate all the nasty crypto, hash and hmac code into tpm2-sessions.c and export a usable API. The API first of all starts off by gaining a session with tpm2_start_auth_session() Which initiates a session with the TPM and allocates an opaque tpm2_auth structure to handle the session parameters. Then the use is simply: * tpm_buf_append_name() in place of the tpm_buf_append_u32 for the handles Do not understand the description of this function. * tpm_buf_append_hmac_session() where tpm2_append_auth() would go Another fuzzy description. * tpm_buf_fill_hmac_session() called after the entire command buffer is finished but before tpm_transmit_cmd() is called which computes the correct HMAC and places it in the command at the correct location. I would call this simply tpm_buf_insert_hmac(). It is clear and precise name what it does. 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). Finally, after tpm_transmit_cmd() is called, tpm_buf_check_hmac_response() is called to check that the returned HMAC matched and collect the new state for the next use of the session, if any. The features of the session is controlled by the session attributes set in tpm_buf_append_hmac_session(). If TPM2_SA_CONTINUE_SESSION is not specified, the session will be flushed and the tpm2_auth structure freed in tpm_buf_check_hmac_response(); otherwise the session may be used again. Parameter encryption is specified by or'ing the flag TPM2_SA_DECRYPT and response encryption by or'ing the flag TPM2_SA_ENCRYPT. the various encryptions will be taken care of by tpm_buf_fill_hmac_session() and tpm_buf_check_hmac_response() respectively. To get all of this to work securely, the Kernel now needs a primary key to encrypt the session salt to, so we derive an EC key from the NULL seed and store it in the tpm_chip structure. We also make sure that this seed remains for the kernel by using a kernel space to take it out of the TPM when userspace wants to use it. Signed-off-by: James Bottomley --- v2: Added docbook and improved response check API v3: Add readpublic, fix hmac length, add API for close on error allow for the hmac session not being first in the sessions v4: Make NULL seed template exactly match the SRK ECC template. Also check the NULL primary key name is what getpublic returns to prevent spoofing. Also parametrise the name size for reuse --- drivers/char/tpm/Kconfig |3 + drivers/char/tpm/Makefile|2 +- drivers/char/tpm/tpm.h | 32 + drivers/char/tpm/tpm2-cmd.c | 34 +- drivers/char/tpm/tpm2-sessions.c | 1188 ++ drivers/char/tpm/tpm2-sessions.h | 57 ++ 6 files changed, 1300 insertions(+), 16 deletions(-) create mode 100644 drivers/char/tpm/tpm2-sessions.c create mode 100644 drivers/char/tpm/tpm2-sessions.h tpm2-cmd.c changes should be in their own commit e.g.: "tpm: add own space for in-kernel TPM communication" diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 0aee88df98d1..8c714d8550c4 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -8,6 +8,9 @@ menuconfig TCG_TPM select SECURITYFS select CRYPTO select CRYPTO_HASH_INFO + select CRYPTO_ECDH + select CRYPTO_AES + select CRYPTO_CFB ---help--- If you have a TPM security chip in your system, which implements the Trusted Computing Group's specification, diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 65d165cce530..1b5f6ccbb86d 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -5,7 +5,7 @@ obj-$(CONFIG_TCG_TPM) += tpm.o tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \ tpm-dev-common.o tpmrm-dev.o eventlog/common.o eventlog/tpm1.o \ -eventlog/tpm2.o tpm2-space.o tpm-buf.o +eventlog/tpm2.o tpm2-space.o tpm-buf.o tpm2-sessions.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.o tpm-$(CONFIG_OF) += eventlog/of.o diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index d73701e36eba..d39065d9995d 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -42,6 +42,15 @@ #include #endif +/* fixed define for the curve we use which is NIST_P256 */ +#define EC_PT_SZ 32 + +/* + * fixed define for the size of a name. This is actually HASHALG size + * plus 2, so 32 for SHA256 + */ +#define TPM2_NAME_SIZE 34 Please, remove this definition completely. It only
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On 23 October 2018 at 04:01, James Bottomley
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();
>> > + 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, );
>> crypto_wait_req(crypto_skcipher_encrypt(req), );
>
> 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 # 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++) {
>
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
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.
> 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();
> > + 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, );
> crypto_wait_req(crypto_skcipher_encrypt(req), );
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.
James
---
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;
Re: [PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
Hi James,
Some comments below on how you are using the crypto API.
On 22 October 2018 at 04:36, James Bottomley
wrote:
> This code adds true session based HMAC authentication plus parameter
> decryption and response encryption using AES.
>
> The basic design of this code is to segregate all the nasty crypto,
> hash and hmac code into tpm2-sessions.c and export a usable API.
>
> The API first of all starts off by gaining a session with
>
> tpm2_start_auth_session()
>
> Which initiates a session with the TPM and allocates an opaque
> tpm2_auth structure to handle the session parameters. Then the use is
> simply:
>
> * tpm_buf_append_name() in place of the tpm_buf_append_u32 for the
> handles
>
> * tpm_buf_append_hmac_session() where tpm2_append_auth() would go
>
> * tpm_buf_fill_hmac_session() called after the entire command buffer
> is finished but before tpm_transmit_cmd() is called which computes
> the correct HMAC and places it in the command at the correct
> location.
>
> Finally, after tpm_transmit_cmd() is called,
> tpm_buf_check_hmac_response() is called to check that the returned
> HMAC matched and collect the new state for the next use of the
> session, if any.
>
> The features of the session is controlled by the session attributes
> set in tpm_buf_append_hmac_session(). If TPM2_SA_CONTINUE_SESSION is
> not specified, the session will be flushed and the tpm2_auth structure
> freed in tpm_buf_check_hmac_response(); otherwise the session may be
> used again. Parameter encryption is specified by or'ing the flag
> TPM2_SA_DECRYPT and response encryption by or'ing the flag
> TPM2_SA_ENCRYPT. the various encryptions will be taken care of by
> tpm_buf_fill_hmac_session() and tpm_buf_check_hmac_response()
> respectively.
>
> To get all of this to work securely, the Kernel now needs a primary
> key to encrypt the session salt to, so we derive an EC key from the
> NULL seed and store it in the tpm_chip structure. We also make sure
> that this seed remains for the kernel by using a kernel space to take
> it out of the TPM when userspace wants to use it.
>
> Signed-off-by: James Bottomley
>
> ---
>
> v2: Added docbook and improved response check API
> v3: Add readpublic, fix hmac length, add API for close on error
> allow for the hmac session not being first in the sessions
> v4: Make NULL seed template exactly match the SRK ECC template.
> Also check the NULL primary key name is what getpublic returns
> to prevent spoofing. Also parametrise the name size for reuse
> ---
> drivers/char/tpm/Kconfig |3 +
> drivers/char/tpm/Makefile|2 +-
> drivers/char/tpm/tpm.h | 32 +
> drivers/char/tpm/tpm2-cmd.c | 34 +-
> drivers/char/tpm/tpm2-sessions.c | 1188
> ++
> drivers/char/tpm/tpm2-sessions.h | 57 ++
> 6 files changed, 1300 insertions(+), 16 deletions(-)
> create mode 100644 drivers/char/tpm/tpm2-sessions.c
> create mode 100644 drivers/char/tpm/tpm2-sessions.h
>
...
> diff --git a/drivers/char/tpm/tpm2-sessions.c
> b/drivers/char/tpm/tpm2-sessions.c
> new file mode 100644
> index ..422c3ec64f8c
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -0,0 +1,1188 @@
...
> +/*
> + * this is our static crypto shash. This is possible because the hash
> + * is multi-threaded and all the state stored in the desc
> + */
> +static struct crypto_shash *sha256_hash;
> +
> +/*
> + * It turns out the crypto hmac(sha256) is hard for us to consume
> + * because it assumes a fixed key and the TPM seems to change the key
> + * on every operation, so we weld the hmac init and final functions in
> + * here to give it the same usage characteristics as a regular hash
> + */
> +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. 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.
> + crypto_shash_init(desc);
> + for (i = 0; i < sizeof(pad); i++) {
> + if (i < keylen)
> + pad[i] = key[i];
> + else
> + pad[i] = 0;
> + pad[i] ^= HMAC_IPAD_VALUE;
> + }
> + crypto_shash_update(desc, pad, sizeof(pad));
> +}
> +
> +static void hmac_final(struct shash_desc *desc, u8 *key, int keylen, u8 *out)
> +{
> + u8 pad[SHA256_BLOCK_SIZE];
> + int i;
> +
> + for (i = 0; i < sizeof(pad); i++) {
> + if (i < keylen)
> + pad[i] = key[i];
> + else
> + pad[i] = 0;
> + pad[i] ^= HMAC_OPAD_VALUE;
> + }
> +
> + /* collect the final hash; use out as
[PATCH v4 2/7] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
This code adds true session based HMAC authentication plus parameter
decryption and response encryption using AES.
The basic design of this code is to segregate all the nasty crypto,
hash and hmac code into tpm2-sessions.c and export a usable API.
The API first of all starts off by gaining a session with
tpm2_start_auth_session()
Which initiates a session with the TPM and allocates an opaque
tpm2_auth structure to handle the session parameters. Then the use is
simply:
* tpm_buf_append_name() in place of the tpm_buf_append_u32 for the
handles
* tpm_buf_append_hmac_session() where tpm2_append_auth() would go
* tpm_buf_fill_hmac_session() called after the entire command buffer
is finished but before tpm_transmit_cmd() is called which computes
the correct HMAC and places it in the command at the correct
location.
Finally, after tpm_transmit_cmd() is called,
tpm_buf_check_hmac_response() is called to check that the returned
HMAC matched and collect the new state for the next use of the
session, if any.
The features of the session is controlled by the session attributes
set in tpm_buf_append_hmac_session(). If TPM2_SA_CONTINUE_SESSION is
not specified, the session will be flushed and the tpm2_auth structure
freed in tpm_buf_check_hmac_response(); otherwise the session may be
used again. Parameter encryption is specified by or'ing the flag
TPM2_SA_DECRYPT and response encryption by or'ing the flag
TPM2_SA_ENCRYPT. the various encryptions will be taken care of by
tpm_buf_fill_hmac_session() and tpm_buf_check_hmac_response()
respectively.
To get all of this to work securely, the Kernel now needs a primary
key to encrypt the session salt to, so we derive an EC key from the
NULL seed and store it in the tpm_chip structure. We also make sure
that this seed remains for the kernel by using a kernel space to take
it out of the TPM when userspace wants to use it.
Signed-off-by: James Bottomley
---
v2: Added docbook and improved response check API
v3: Add readpublic, fix hmac length, add API for close on error
allow for the hmac session not being first in the sessions
v4: Make NULL seed template exactly match the SRK ECC template.
Also check the NULL primary key name is what getpublic returns
to prevent spoofing. Also parametrise the name size for reuse
---
drivers/char/tpm/Kconfig |3 +
drivers/char/tpm/Makefile|2 +-
drivers/char/tpm/tpm.h | 32 +
drivers/char/tpm/tpm2-cmd.c | 34 +-
drivers/char/tpm/tpm2-sessions.c | 1188 ++
drivers/char/tpm/tpm2-sessions.h | 57 ++
6 files changed, 1300 insertions(+), 16 deletions(-)
create mode 100644 drivers/char/tpm/tpm2-sessions.c
create mode 100644 drivers/char/tpm/tpm2-sessions.h
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 0aee88df98d1..8c714d8550c4 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -8,6 +8,9 @@ menuconfig TCG_TPM
select SECURITYFS
select CRYPTO
select CRYPTO_HASH_INFO
+ select CRYPTO_ECDH
+ select CRYPTO_AES
+ select CRYPTO_CFB
---help---
If you have a TPM security chip in your system, which
implements the Trusted Computing Group's specification,
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 65d165cce530..1b5f6ccbb86d 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -5,7 +5,7 @@
obj-$(CONFIG_TCG_TPM) += tpm.o
tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
tpm-dev-common.o tpmrm-dev.o eventlog/common.o eventlog/tpm1.o \
-eventlog/tpm2.o tpm2-space.o tpm-buf.o
+eventlog/tpm2.o tpm2-space.o tpm-buf.o tpm2-sessions.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
tpm-$(CONFIG_EFI) += eventlog/efi.o
tpm-$(CONFIG_OF) += eventlog/of.o
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index d73701e36eba..d39065d9995d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -42,6 +42,15 @@
#include
#endif
+/* fixed define for the curve we use which is NIST_P256 */
+#define EC_PT_SZ 32
+
+/*
+ * fixed define for the size of a name. This is actually HASHALG size
+ * plus 2, so 32 for SHA256
+ */
+#define TPM2_NAME_SIZE 34
+
enum tpm_const {
TPM_MINOR = 224,/* officially assigned */
TPM_BUFSIZE = 4096,
@@ -103,6 +112,7 @@ enum tpm2_timeouts {
enum tpm2_structures {
TPM2_ST_NO_SESSIONS = 0x8001,
TPM2_ST_SESSIONS= 0x8002,
+ TPM2_ST_CREATION= 0x8021,
};
/* Indicates from what layer of the software stack the error comes from */
@@ -125,12 +135,20 @@ enum tpm2_return_codes {
enum tpm2_algorithms {
TPM2_ALG_ERROR = 0x,
TPM2_ALG_SHA1 = 0x0004,
+ TPM2_ALG_AES= 0x0006,
TPM2_ALG_KEYEDHASH = 0x0008,
TPM2_ALG_SHA256 = 0x000B,
TPM2_ALG_SHA384
