[PATCH RFC 0/4] RFC: in-kernel resource manager

[PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

This patch set adds support for TPM spaces that provide a context
for isolating and swapping transient objects. This patch set does
not yet include support for isolating policy and HMAC sessions but
it is trivial to add once the basic approach is settled (and that's
why I created an RFC patch set).

There's a test script for trying out TPM spaces in

  git://git.infradead.org/users/jjs/tpm2-scripts.git

A simple smoke test can be run by

  sudo python -m unittest -v tpm2_smoke.SpaceTest   

Jarkko Sakkinen (4):
  tpm: migrate struct tpm_buf to struct tpm_chip
  tpm: validate TPM 2.0 commands
  tpm: export tpm2_flush_context_cmd
  tpm: add the infrastructure for TPM space for TPM 2.0

 drivers/char/tpm/Makefile        |   2 +-
 drivers/char/tpm/tpm-chip.c      |  15 ++
 drivers/char/tpm/tpm-dev.c       |  80 ++++++++++-
 drivers/char/tpm/tpm-interface.c |  93 +++++++++----
 drivers/char/tpm/tpm-sysfs.c     |   2 +-
 drivers/char/tpm/tpm.h           | 106 ++++++++------
 drivers/char/tpm/tpm2-cmd.c      | 232 ++++++++++++++++---------------
 drivers/char/tpm/tpm2-space.c    | 288 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/tpm.h         |  23 ++++
 9 files changed, 662 insertions(+), 179 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2-space.c
 create mode 100644 include/uapi/linux/tpm.h

-- 
2.9.3

[PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip

[Jarkko Sakkinen]

Since there is only one thread using TPM chip at a time to transmit data
we can migrate struct tpm_buf to struct tpm_chip. This makes the use of
it more fail safe as the buffer is allocated from heap when the device
is created and not for every transaction.

This is needed characteristic for the resource manager so that we can
minimize the probability of failure for loading, saving and flushings
of TPM contexts.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-chip.c |   8 +++
 drivers/char/tpm/tpm.h      |  67 ++++++++++-------------
 drivers/char/tpm/tpm2-cmd.c | 128 ++++++++++++++++++--------------------------
 3 files changed, 87 insertions(+), 116 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index eefdc80..41e518e 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -128,6 +128,7 @@ static void tpm_dev_release(struct device *dev)
 	mutex_unlock(&idr_lock);
 
 	kfree(chip->log.bios_event_log);
+	kfree(chip->tr_buf.data);
 	kfree(chip);
 }
 
@@ -189,6 +190,13 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	chip->cdev.owner = THIS_MODULE;
 	chip->cdev.kobj.parent = &chip->dev.kobj;
 
+	chip->tr_buf.data = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
+	if (!chip->tr_buf.data) {
+		rc = -ENOMEM;
+		goto out;
+	}
+	chip->tr_buf.size = TPM_BUFSIZE;
+
 	return chip;
 
 out:
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 1ae9768..c74a663 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -153,6 +153,20 @@ struct tpm_chip_seqops {
 	const struct seq_operations *seqops;
 };
 
+/* A string buffer pointer type for constructing TPM commands. Does not own
+ * the data.
+ */
+
+enum tpm_buf_flags {
+	TPM_BUF_OVERFLOW        = BIT(0),
+};
+
+struct tpm_buf {
+	unsigned int flags;
+	u8 *data;
+	unsigned int size;
+};
+
 struct tpm_chip {
 	struct device dev;
 	struct cdev cdev;
@@ -191,6 +205,8 @@ struct tpm_chip {
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
 #endif /* CONFIG_ACPI */
+
+	struct tpm_buf tr_buf;
 };
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
@@ -387,57 +403,28 @@ struct tpm_cmd_t {
 	tpm_cmd_params	params;
 } __packed;
 
-/* A string buffer type for constructing TPM commands. This is based on the
- * ideas of string buffer code in security/keys/trusted.h but is heap based
- * in order to keep the stack usage minimal.
- */
-
-enum tpm_buf_flags {
-	TPM_BUF_OVERFLOW	= BIT(0),
-};
+/* A string buffer for constructing TPM commands. */
 
-struct tpm_buf {
-	struct page *data_page;
-	unsigned int flags;
-	u8 *data;
-};
-
-static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+static inline void tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
 {
-	struct tpm_input_header *head;
-
-	buf->data_page = alloc_page(GFP_HIGHUSER);
-	if (!buf->data_page)
-		return -ENOMEM;
-
-	buf->flags = 0;
-	buf->data = kmap(buf->data_page);
-
-	head = (struct tpm_input_header *) buf->data;
+	struct tpm_input_header *head = (struct tpm_input_header *)buf->data;
 
+	buf->flags &= ~TPM_BUF_OVERFLOW;
 	head->tag = cpu_to_be16(tag);
 	head->length = cpu_to_be32(sizeof(*head));
 	head->ordinal = cpu_to_be32(ordinal);
-
-	return 0;
-}
-
-static inline void tpm_buf_destroy(struct tpm_buf *buf)
-{
-	kunmap(buf->data_page);
-	__free_page(buf->data_page);
 }
 
 static inline u32 tpm_buf_length(struct tpm_buf *buf)
 {
-	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+	struct tpm_input_header *head = (struct tpm_input_header *)buf->data;
 
 	return be32_to_cpu(head->length);
 }
 
 static inline u16 tpm_buf_tag(struct tpm_buf *buf)
 {
-	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+	struct tpm_input_header *head = (struct tpm_input_header *)buf->data;
 
 	return be16_to_cpu(head->tag);
 }
@@ -446,14 +433,16 @@ static inline void tpm_buf_append(struct tpm_buf *buf,
 				  const unsigned char *new_data,
 				  unsigned int new_len)
 {
-	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+
+	struct tpm_input_header *head = (struct tpm_input_header *)buf->data;
+
 	u32 len = tpm_buf_length(buf);
 
 	/* Return silently if overflow has already happened. */
 	if (buf->flags & TPM_BUF_OVERFLOW)
 		return;
 
-	if ((len + new_len) > PAGE_SIZE) {
+	if ((len + new_len) > buf->size) {
 		WARN(1, "tpm_buf: overflow\n");
 		buf->flags |= TPM_BUF_OVERFLOW;
 		return;
@@ -472,14 +461,14 @@ static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
 {
 	__be16 value2 = cpu_to_be16(value);
 
-	tpm_buf_append(buf, (u8 *) &value2, 2);
+	tpm_buf_append(buf, (u8 *)&value2, 2);
 }
 
 static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 {
 	__be32 value2 = cpu_to_be32(value);
 
-	tpm_buf_append(buf, (u8 *) &value2, 4);
+	tpm_buf_append(buf, (u8 *)&value2, 4);
 }
 
 extern struct class *tpm_class;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 6eda239..f0e0807 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -444,7 +444,6 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 		      struct trusted_key_options *options)
 {
 	unsigned int blob_len;
-	struct tpm_buf buf;
 	u32 hash;
 	int i;
 	int rc;
@@ -459,73 +458,67 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	if (i == ARRAY_SIZE(tpm2_hash_map))
 		return -EINVAL;
 
-	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
-	if (rc)
-		return rc;
+	tpm_buf_init(&chip->tr_buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
 
-	tpm_buf_append_u32(&buf, options->keyhandle);
-	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+	tpm_buf_append_u32(&chip->tr_buf, options->keyhandle);
+	tpm2_buf_append_auth(&chip->tr_buf, TPM2_RS_PW,
 			     NULL /* nonce */, 0,
 			     0 /* session_attributes */,
 			     options->keyauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
 	/* sensitive */
-	tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
+	tpm_buf_append_u16(&chip->tr_buf,
+			   4 + TPM_DIGEST_SIZE + payload->key_len + 1);
 
-	tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
-	tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
-	tpm_buf_append_u16(&buf, payload->key_len + 1);
-	tpm_buf_append(&buf, payload->key, payload->key_len);
-	tpm_buf_append_u8(&buf, payload->migratable);
+	tpm_buf_append_u16(&chip->tr_buf, TPM_DIGEST_SIZE);
+	tpm_buf_append(&chip->tr_buf, options->blobauth, TPM_DIGEST_SIZE);
+	tpm_buf_append_u16(&chip->tr_buf, payload->key_len + 1);
+	tpm_buf_append(&chip->tr_buf, payload->key, payload->key_len);
+	tpm_buf_append_u8(&chip->tr_buf, payload->migratable);
 
 	/* public */
-	tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
-	tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
-	tpm_buf_append_u16(&buf, hash);
+	tpm_buf_append_u16(&chip->tr_buf, 14 + options->policydigest_len);
+	tpm_buf_append_u16(&chip->tr_buf, TPM2_ALG_KEYEDHASH);
+	tpm_buf_append_u16(&chip->tr_buf, hash);
 
 	/* policy */
 	if (options->policydigest_len) {
-		tpm_buf_append_u32(&buf, 0);
-		tpm_buf_append_u16(&buf, options->policydigest_len);
-		tpm_buf_append(&buf, options->policydigest,
+		tpm_buf_append_u32(&chip->tr_buf, 0);
+		tpm_buf_append_u16(&chip->tr_buf, options->policydigest_len);
+		tpm_buf_append(&chip->tr_buf, options->policydigest,
 			       options->policydigest_len);
 	} else {
-		tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
-		tpm_buf_append_u16(&buf, 0);
+		tpm_buf_append_u32(&chip->tr_buf, TPM2_OA_USER_WITH_AUTH);
+		tpm_buf_append_u16(&chip->tr_buf, 0);
 	}
 
 	/* public parameters */
-	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
-	tpm_buf_append_u16(&buf, 0);
+	tpm_buf_append_u16(&chip->tr_buf, TPM2_ALG_NULL);
+	tpm_buf_append_u16(&chip->tr_buf, 0);
 
 	/* outside info */
-	tpm_buf_append_u16(&buf, 0);
+	tpm_buf_append_u16(&chip->tr_buf, 0);
 
 	/* creation PCR */
-	tpm_buf_append_u32(&buf, 0);
+	tpm_buf_append_u32(&chip->tr_buf, 0);
 
-	if (buf.flags & TPM_BUF_OVERFLOW) {
-		rc = -E2BIG;
-		goto out;
-	}
+	if (chip->tr_buf.flags & TPM_BUF_OVERFLOW)
+		return -E2BIG;
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, 0, "sealing data");
+	rc = tpm_transmit_cmd(chip, &chip->tr_buf, TPM_BUFSIZE, 0,
+			      "sealing data");
 	if (rc)
-		goto out;
+		return rc;
 
-	blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
-	if (blob_len > MAX_BLOB_SIZE) {
-		rc = -E2BIG;
-		goto out;
-	}
+	blob_len = be32_to_cpup((__be32 *)&chip->tr_buf.data[TPM_HEADER_SIZE]);
+	if (blob_len > MAX_BLOB_SIZE)
+		return -E2BIG;
 
-	memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len);
+	memcpy(payload->blob, &chip->tr_buf.data[TPM_HEADER_SIZE + 4],
+	       blob_len);
 	payload->blob_len = blob_len;
 
-out:
-	tpm_buf_destroy(&buf);
-
 	if (rc > 0) {
 		if ((rc & TPM2_RC_HASH) == TPM2_RC_HASH)
 			rc = -EINVAL;
@@ -555,7 +548,6 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 			 struct trusted_key_options *options,
 			 u32 *blob_handle, unsigned int flags)
 {
-	struct tpm_buf buf;
 	unsigned int private_len;
 	unsigned int public_len;
 	unsigned int blob_len;
@@ -570,31 +562,25 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 	if (blob_len > payload->blob_len)
 		return -E2BIG;
 
-	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
-	if (rc)
-		return rc;
+	tpm_buf_init(&chip->tr_buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
 
-	tpm_buf_append_u32(&buf, options->keyhandle);
-	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+	tpm_buf_append_u32(&chip->tr_buf, options->keyhandle);
+	tpm2_buf_append_auth(&chip->tr_buf, TPM2_RS_PW,
 			     NULL /* nonce */, 0,
 			     0 /* session_attributes */,
 			     options->keyauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	tpm_buf_append(&buf, payload->blob, blob_len);
+	tpm_buf_append(&chip->tr_buf, payload->blob, blob_len);
 
-	if (buf.flags & TPM_BUF_OVERFLOW) {
-		rc = -E2BIG;
-		goto out;
-	}
+	if (chip->tr_buf.flags & TPM_BUF_OVERFLOW)
+		return -E2BIG;
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "loading blob");
+	rc = tpm_transmit_cmd(chip, &chip->tr_buf, TPM_BUFSIZE, flags,
+			      "loading blob");
 	if (!rc)
 		*blob_handle = be32_to_cpup(
-			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
-
-out:
-	tpm_buf_destroy(&buf);
+			(__be32 *)&chip->tr_buf.data[TPM_HEADER_SIZE]);
 
 	if (rc > 0)
 		rc = -EPERM;
@@ -614,25 +600,16 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 static void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 				   unsigned int flags)
 {
-	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
-	if (rc) {
-		dev_warn(&chip->dev, "0x%08x was not flushed, out of memory\n",
-			 handle);
-		return;
-	}
-
-	tpm_buf_append_u32(&buf, handle);
+	tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
+	tpm_buf_append_u32(&chip->tr_buf, handle);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags,
+	rc = tpm_transmit_cmd(chip, &chip->tr_buf, TPM_BUFSIZE, flags,
 			      "flushing context");
 	if (rc)
 		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
 			 rc);
-
-	tpm_buf_destroy(&buf);
 }
 
 /**
@@ -653,17 +630,14 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
 			   struct trusted_key_options *options,
 			   u32 blob_handle, unsigned int flags)
 {
-	struct tpm_buf buf;
 	u16 data_len;
 	u8 *data;
 	int rc;
 
-	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
-	if (rc)
-		return rc;
+	tpm_buf_init(&chip->tr_buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
 
-	tpm_buf_append_u32(&buf, blob_handle);
-	tpm2_buf_append_auth(&buf,
+	tpm_buf_append_u32(&chip->tr_buf, blob_handle);
+	tpm2_buf_append_auth(&chip->tr_buf,
 			     options->policyhandle ?
 			     options->policyhandle : TPM2_RS_PW,
 			     NULL /* nonce */, 0,
@@ -671,21 +645,21 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, flags, "unsealing");
+	rc = tpm_transmit_cmd(chip, chip->tr_buf.data, TPM_BUFSIZE, flags,
+			      "unsealing");
 	if (rc > 0)
 		rc = -EPERM;
 
 	if (!rc) {
 		data_len = be16_to_cpup(
-			(__be16 *) &buf.data[TPM_HEADER_SIZE + 4]);
-		data = &buf.data[TPM_HEADER_SIZE + 6];
+			(__be16 *)&chip->tr_buf.data[TPM_HEADER_SIZE + 4]);
+		data = &chip->tr_buf.data[TPM_HEADER_SIZE + 6];
 
 		memcpy(payload->key, data, data_len - 1);
 		payload->key_len = data_len - 1;
 		payload->migratable = data[data_len - 1];
 	}
 
-	tpm_buf_destroy(&buf);
 	return rc;
 }
 
-- 
2.9.3

Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip

[Jason Gunthorpe]

On Mon, Jan 02, 2017 at 03:22:07PM +0200, Jarkko Sakkinen wrote:
> Since there is only one thread using TPM chip at a time to transmit data
> we can migrate struct tpm_buf to struct tpm_chip. This makes the use of
> it more fail safe as the buffer is allocated from heap when the device
> is created and not for every transaction.

Eh? What? I don't think that is the case..

We don't serialize until we hit tramsit_cmd at which point the buffer
is already being used and cannot be shared between threads.

Only /dev/tpmX has any sort of locking, but even that is
designed to be optional (eg I patch it out of my kernels), and only
covers userspace, not contention with in-kernel threads.

Why would the resource manager need a single global tpm buffer? That
seems like a big regression from where we have been going. I don't
think this is a good idea to go down this road.

> -	tpm_buf_append(buf, (u8 *) &value2, 4);
> +	tpm_buf_append(buf, (u8 *)&value2, 4);

Please try and avoid this sort of churn in patches that change things..

Jason

Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip

[Jarkko Sakkinen]

On Mon, Jan 02, 2017 at 02:01:01PM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 02, 2017 at 03:22:07PM +0200, Jarkko Sakkinen wrote:
> > Since there is only one thread using TPM chip at a time to transmit data
> > we can migrate struct tpm_buf to struct tpm_chip. This makes the use of
> > it more fail safe as the buffer is allocated from heap when the device
> > is created and not for every transaction.
> 
> Eh? What? I don't think that is the case..
> 
> We don't serialize until we hit tramsit_cmd at which point the buffer
> is already being used and cannot be shared between threads.

There is a regression in the patch. All functions that use 'tr_buf'
should take tpm_mutex first and use TPM_TRANSMIT_UNLOCKED. There's
also a similar regression in TPM space patch that I have to correct.

> Why would the resource manager need a single global tpm buffer? That
> seems like a big regression from where we have been going. I don't
> think this is a good idea to go down this road.

What? 'tr_buf' is not specifically for resource manager. This commit
makes creating TPM commands more fail-safe because there is no need
to allocate page for every transmit.

For RM decorations this is really important because I rather would have
them fail as rarely as possible. If this would become a scalability
issue then the granularity could be reconsidered.

> > -	tpm_buf_append(buf, (u8 *) &value2, 4);
> > +	tpm_buf_append(buf, (u8 *)&value2, 4);
> 
> Please try and avoid this sort of churn in patches that change things..

It wasn't there on purpose. I do not know how these slipped. I can
clean these up.

> Jason

/Jarkko

Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip

[Jason Gunthorpe]

On Tue, Jan 03, 2017 at 02:57:37AM +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 02, 2017 at 02:01:01PM -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 02, 2017 at 03:22:07PM +0200, Jarkko Sakkinen wrote:
> > > Since there is only one thread using TPM chip at a time to transmit data
> > > we can migrate struct tpm_buf to struct tpm_chip. This makes the use of
> > > it more fail safe as the buffer is allocated from heap when the device
> > > is created and not for every transaction.
> > 
> > Eh? What? I don't think that is the case..
> > 
> > We don't serialize until we hit tramsit_cmd at which point the buffer
> > is already being used and cannot be shared between threads.
> 
> There is a regression in the patch. All functions that use 'tr_buf'
> should take tpm_mutex first and use TPM_TRANSMIT_UNLOCKED. There's
> also a similar regression in TPM space patch that I have to correct.

No, you can't steal TPM_TRANSMIT_UNLOCKED and tpm_mutex for this, that
is to allow a chain of commands to execute atomicly, so a new lock is
needed just for the tr_buf.

> > Why would the resource manager need a single global tpm buffer? That
> > seems like a big regression from where we have been going. I don't
> > think this is a good idea to go down this road.
> 
> What? 'tr_buf' is not specifically for resource manager. This commit
> makes creating TPM commands more fail-safe because there is no need
> to allocate page for every transmit.

That doesn't seem all that important, honestly. There kernel does not
fail single page allocations without a lot of duress.

> For RM decorations this is really important because I rather would have
> them fail as rarely as possible. If this would become a scalability
> issue then the granularity could be reconsidered.

Why? The RM design already seems to have the prepare/commit/abort
kind of model so it can already fail. What does it matter if the
caller can fail before getting that far?

It seems like alot of dangerous churn to introduce a new locking model
without a really good reason...

Jason

Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 12:13:28PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 03, 2017 at 02:57:37AM +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 02:01:01PM -0700, Jason Gunthorpe wrote:
> > > On Mon, Jan 02, 2017 at 03:22:07PM +0200, Jarkko Sakkinen wrote:
> > > > Since there is only one thread using TPM chip at a time to transmit data
> > > > we can migrate struct tpm_buf to struct tpm_chip. This makes the use of
> > > > it more fail safe as the buffer is allocated from heap when the device
> > > > is created and not for every transaction.
> > > 
> > > Eh? What? I don't think that is the case..
> > > 
> > > We don't serialize until we hit tramsit_cmd at which point the buffer
> > > is already being used and cannot be shared between threads.
> > 
> > There is a regression in the patch. All functions that use 'tr_buf'
> > should take tpm_mutex first and use TPM_TRANSMIT_UNLOCKED. There's
> > also a similar regression in TPM space patch that I have to correct.
> 
> No, you can't steal TPM_TRANSMIT_UNLOCKED and tpm_mutex for this, that
> is to allow a chain of commands to execute atomicly, so a new lock is
> needed just for the tr_buf.
> 
> > > Why would the resource manager need a single global tpm buffer? That
> > > seems like a big regression from where we have been going. I don't
> > > think this is a good idea to go down this road.
> > 
> > What? 'tr_buf' is not specifically for resource manager. This commit
> > makes creating TPM commands more fail-safe because there is no need
> > to allocate page for every transmit.
> 
> That doesn't seem all that important, honestly. There kernel does not
> fail single page allocations without a lot of duress.
> 
> > For RM decorations this is really important because I rather would have
> > them fail as rarely as possible. If this would become a scalability
> > issue then the granularity could be reconsidered.
> 
> Why? The RM design already seems to have the prepare/commit/abort
> kind of model so it can already fail. What does it matter if the
> caller can fail before getting that far?

Yeah, I just noticed it :-) That kind of formed by accident when I
experimented with various models of rolling back in an error situation.

> It seems like alot of dangerous churn to introduce a new locking model
> without a really good reason...

OK, thanks for the feedback. I understad your arguments but as this
was an RFC patch set I don't want to go more details like these but
I take your advice seriously.

I'll start preparing the first non-RFC version. I'm happy that the beef
(i.e. the stuff in tpm2-space.c) has been well accepted!

> Jason

/Jarkko

[PATCH RFC 2/4] tpm: validate TPM 2.0 commands

[Jarkko Sakkinen]

Check for every TPM 2.0 command that the command code is supported and
the command buffer has at least the length that can contain the header
and the handle area.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 32 +++++++++++++++++++++++++++++-
 drivers/char/tpm/tpm.h           | 15 +++++++++++++-
 drivers/char/tpm/tpm2-cmd.c      | 43 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 769d8b0..0794a5d3 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -328,6 +328,36 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip,
 }
 EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 
+static bool tpm_validate_command(struct tpm_chip *chip, const u8 *cmd,
+				 size_t len)
+{
+	const struct tpm_input_header *header = (const void *)cmd;
+	u32 cc;
+	size_t len_min = TPM_HEADER_SIZE;
+	u32 attrs;
+
+	if ((len >= len_min) && (chip->flags & TPM_CHIP_FLAG_TPM2) &&
+	    chip->nr_commands) {
+		cc = be32_to_cpu(header->ordinal);
+		if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
+			dev_dbg(&chip->dev, "0x%04x is an invalid command\n",
+				cc);
+			return false;
+		}
+		len_min +=
+			4 * ((attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0));
+	}
+
+	if (len < len_min) {
+		dev_dbg(&chip->dev,
+			"%s: insufficient command length %zu < %zu\n",
+			__func__, len, len_min);
+		return false;
+	}
+
+	return true;
+}
+
 /**
  * tmp_transmit - Internal kernel interface to transmit TPM commands.
  *
@@ -347,7 +377,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 	u32 count, ordinal;
 	unsigned long stop;
 
-	if (bufsiz < TPM_HEADER_SIZE)
+	if (!tpm_validate_command(chip, buf, bufsiz))
 		return -EINVAL;
 
 	if (bufsiz > TPM_BUFSIZE)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c74a663..ed21c2c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -127,7 +127,12 @@ enum tpm2_permanent_handles {
 };
 
 enum tpm2_capabilities {
-	TPM2_CAP_TPM_PROPERTIES = 6,
+	TPM2_CAP_COMMANDS	= 2,
+	TPM2_CAP_TPM_PROPERTIES	= 6,
+};
+
+enum tpm2_properties {
+	TPM_PT_TOTAL_COMMANDS	= 0x0129,
 };
 
 enum tpm2_startup_types {
@@ -135,6 +140,11 @@ enum tpm2_startup_types {
 	TPM2_SU_STATE	= 0x0001,
 };
 
+enum tpm2_cc_attrs {
+	TPM2_CC_ATTR_CHANDLES	= 25,
+	TPM2_CC_ATTR_RHANDLE	= 28,
+};
+
 #define TPM_VID_INTEL    0x8086
 #define TPM_VID_WINBOND  0x1050
 #define TPM_VID_STM      0x104A
@@ -207,6 +217,8 @@ struct tpm_chip {
 #endif /* CONFIG_ACPI */
 
 	struct tpm_buf tr_buf;
+	u32 nr_commands;
+	u32 *cc_attrs_tbl;
 };
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
@@ -534,4 +546,5 @@ int tpm2_auto_startup(struct tpm_chip *chip);
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
 int tpm2_probe(struct tpm_chip *chip);
+bool tpm2_find_cc_attrs(struct tpm_chip *chip, u32 cc, u32 *attrs);
 #endif
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index f0e0807..fa928c7 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe);
  */
 int tpm2_auto_startup(struct tpm_chip *chip)
 {
+	u32 nr_commands;
 	int rc;
+	int i;
 
 	rc = tpm_get_timeouts(chip);
 	if (rc)
@@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 		}
 	}
 
+	rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands, NULL);
+	if (rc)
+		return rc;
+
+	chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands,
+					  GFP_KERNEL);
+
+	tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS,
+		     TPM2_CC_GET_CAPABILITY);
+	tpm_buf_append_u32(&chip->tr_buf, TPM2_CAP_COMMANDS);
+	tpm_buf_append_u32(&chip->tr_buf, TPM2_CC_FIRST);
+	tpm_buf_append_u32(&chip->tr_buf, nr_commands);
+
+	rc = tpm_transmit_cmd(chip, chip->tr_buf.data, TPM_BUFSIZE, 0, NULL);
+	if (rc < 0)
+		goto out;
+
+	if (nr_commands !=
+	    be32_to_cpup((__be32 *)&chip->tr_buf.data[TPM_HEADER_SIZE + 5]))
+		return -EINVAL;
+
+	for (i = 0; i < nr_commands; i++)
+		chip->cc_attrs_tbl[i] = be32_to_cpup(
+			(u32 *)&chip->tr_buf.data[TPM_HEADER_SIZE + 9 + 4 * i]);
+
+	chip->nr_commands = nr_commands;
+
 out:
 	if (rc > 0)
 		rc = -ENODEV;
 	return rc;
 }
+
+bool tpm2_find_cc_attrs(struct tpm_chip *chip, u32 cc, u32 *attrs)
+{
+	int i;
+
+	for (i = 0; i < chip->nr_commands; i++) {
+		if (cc == (chip->cc_attrs_tbl[i] & GENMASK(15, 0))) {
+			*attrs = chip->cc_attrs_tbl[i];
+			return true;
+		}
+	}
+
+	return false;
+}
-- 
2.9.3

Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 commands

[James Bottomley]

On Wed, 2017-01-04 at 13:04 -0500, Stefan Berger wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 01/02/2017
> 08:22:08 AM:
> 
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe);
> >   */
> >  int tpm2_auto_startup(struct tpm_chip *chip)
> >  {
> > +   u32 nr_commands;
> >     int rc;
> > +   int i;
> > 
> >     rc = tpm_get_timeouts(chip);
> >     if (rc)
> > @@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip)
> >        }
> >     }
> > 
> > +   rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands,
> NULL);
> > +   if (rc)
> > +      return rc;
> > +
> > +   chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands,
> > +                 GFP_KERNEL);
> 
> For some reason this devm_kzalloc bombs for the vtpm proxy driver. 
> The only reason I could come up with is that it's being called before
> tpm_add_char_device() has been called.

No, it should be sufficient that chip->dev be initialized (which it is
in tpm_chip_alloc()).  What's the error you're getting?

It does look like the intention was to have non-devm with
tpm_chip_alloc() and devm with tpmm_chip_alloc(), but devm_kzalloc
should just work regardless because it's tied to the device model.

James

Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 commands

[Jason Gunthorpe]

On Wed, Jan 04, 2017 at 01:04:59PM -0500, Stefan Berger wrote:

>    > @@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe);
>    >   */
>    >  int tpm2_auto_startup(struct tpm_chip *chip)
>    >  {
>    > +   u32 nr_commands;
>    >     int rc;
>    > +   int i;
>    >
>    >     rc = tpm_get_timeouts(chip);
>    >     if (rc)
>    > @@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>    >        }
>    >     }
>    >
>    > +   rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands,
>    NULL);
>    > +   if (rc)
>    > +      return rc;
>    > +
>    > +   chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands,
>    > +                 GFP_KERNEL);
>    For some reason this devm_kzalloc bombs for the vtpm proxy driver. The
>    only reason I could come up with is that it's being called before
>    tpm_add_char_device() has been called.

It would also fail if nr_commands is wrong, and this should be one of
the array safe allocation functions since nr_command is data from the
TPM...

Jason

[PATCH RFC 3/4] tpm: export tpm2_flush_context_cmd

[Jarkko Sakkinen]

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm.h      |  2 ++
 drivers/char/tpm/tpm2-cmd.c | 47 ++++++++++++++++++++++-----------------------
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index ed21c2c..fb02b57 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -533,6 +533,8 @@ static inline void tpm_add_ppi(struct tpm_chip *chip)
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash);
 int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max);
+void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
+			    unsigned int flags);
 int tpm2_seal_trusted(struct tpm_chip *chip,
 		      struct trusted_key_payload *payload,
 		      struct trusted_key_options *options);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index fa928c7..311dc8e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -401,6 +401,29 @@ static const struct tpm_input_header tpm2_get_tpm_pt_header = {
 };
 
 /**
+ * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ *
+ * Return: same as with tpm_transmit_cmd
+ */
+void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
+			    unsigned int flags)
+{
+	int rc;
+
+	tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
+	tpm_buf_append_u32(&chip->tr_buf, handle);
+
+	rc = tpm_transmit_cmd(chip, chip->tr_buf.data, TPM_BUFSIZE, flags,
+			      "flushing context");
+	if (rc)
+		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
+			 rc);
+}
+
+/**
  * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
  *
  * @buf: an allocated tpm_buf instance
@@ -589,30 +612,6 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 }
 
 /**
- * tpm2_flush_context_cmd() - execute a TPM2_FlushContext command
- *
- * @chip: TPM chip to use
- * @handle: the key data in clear and encrypted form
- * @flags: tpm transmit flags
- *
- * Return: Same as with tpm_transmit_cmd.
- */
-static void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
-				   unsigned int flags)
-{
-	int rc;
-
-	tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
-	tpm_buf_append_u32(&chip->tr_buf, handle);
-
-	rc = tpm_transmit_cmd(chip, &chip->tr_buf, TPM_BUFSIZE, flags,
-			      "flushing context");
-	if (rc)
-		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
-			 rc);
-}
-
-/**
  * tpm2_unseal_cmd() - execute a TPM2_Unload command
  *
  * @chip: TPM chip to use
-- 
2.9.3

[PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jarkko Sakkinen]

Added a ioctl for creating a TPM space. The space is isolated from the
other users of the TPM. Only a process holding the file with the handle
can access the objects and only objects that are created through that
file handle can be accessed.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/Makefile        |   2 +-
 drivers/char/tpm/tpm-chip.c      |   7 +
 drivers/char/tpm/tpm-dev.c       |  80 ++++++++++-
 drivers/char/tpm/tpm-interface.c |  61 +++++----
 drivers/char/tpm/tpm-sysfs.c     |   2 +-
 drivers/char/tpm/tpm.h           |  22 ++-
 drivers/char/tpm/tpm2-cmd.c      |  30 ++--
 drivers/char/tpm/tpm2-space.c    | 288 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/tpm.h         |  23 ++++
 9 files changed, 470 insertions(+), 45 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2-space.c
 create mode 100644 include/uapi/linux/tpm.h

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a05b1eb..251d0ed 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -3,7 +3,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_eventlog.o
+	 tpm_eventlog.o tpm2-space.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
 tpm-$(CONFIG_OF) += tpm_of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 41e518e..468af50 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -129,6 +129,7 @@ static void tpm_dev_release(struct device *dev)
 
 	kfree(chip->log.bios_event_log);
 	kfree(chip->tr_buf.data);
+	kfree(chip->work_space.context_buf);
 	kfree(chip);
 }
 
@@ -197,6 +198,12 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	}
 	chip->tr_buf.size = TPM_BUFSIZE;
 
+	chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!chip->work_space.context_buf) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
 	return chip;
 
 out:
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 912ad30..139638b 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -19,6 +19,7 @@
  */
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <uapi/linux/tpm.h>
 #include "tpm.h"
 
 struct file_priv {
@@ -32,6 +33,8 @@ struct file_priv {
 	struct work_struct work;
 
 	u8 data_buffer[TPM_BUFSIZE];
+	struct tpm_space space;
+	bool has_space;
 };
 
 static void user_reader_timeout(unsigned long ptr)
@@ -115,6 +118,7 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
 			 size_t size, loff_t *off)
 {
 	struct file_priv *priv = file->private_data;
+	struct tpm_space *space = NULL;
 	size_t in_size = size;
 	ssize_t out_size;
 
@@ -130,6 +134,9 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
 
 	mutex_lock(&priv->buffer_mutex);
 
+	if (priv->has_space)
+		space = &priv->space;
+
 	if (copy_from_user
 	    (priv->data_buffer, (void __user *) buf, in_size)) {
 		mutex_unlock(&priv->buffer_mutex);
@@ -144,7 +151,7 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
 		mutex_unlock(&priv->buffer_mutex);
 		return -EPIPE;
 	}
-	out_size = tpm_transmit(priv->chip, priv->data_buffer,
+	out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
 				sizeof(priv->data_buffer), 0);
 
 	tpm_put_ops(priv->chip);
@@ -162,6 +169,65 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
 	return in_size;
 }
 
+/**
+ * tpm_ioc_new_space - handler for %SGX_IOC_NEW_SPACE ioctl
+ *
+ * Creates a new TPM space that can hold a set of transient objects. The space
+ * is isolated with virtual handles that are mapped into physical handles by the
+ * driver.
+ */
+static long tpm_ioc_new_space(struct file *file, unsigned int ioctl,
+			      unsigned long arg)
+{
+	struct file_priv *priv = file->private_data;
+	struct tpm_chip *chip = priv->chip;
+	int rc = 0;
+
+	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+		return -EOPNOTSUPP;
+
+	mutex_lock(&priv->buffer_mutex);
+
+	if (priv->has_space) {
+		rc = -EBUSY;
+		goto out;
+	}
+
+	priv->space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!priv->space.context_buf) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	/* The TPM device can be opened again as this file has been moved to a
+	 * TPM handle space.
+	 */
+	priv->has_space = true;
+	clear_bit(0, &chip->is_open);
+out:
+	mutex_unlock(&priv->buffer_mutex);
+	return rc;
+}
+
+static long tpm_ioctl(struct file *file, unsigned int ioctl,
+		      unsigned long arg)
+{
+	switch (ioctl) {
+	case TPM_IOC_NEW_SPACE:
+		return tpm_ioc_new_space(file, ioctl, arg);
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
+#ifdef CONFIG_COMPAT
+static long tpm_compat_ioctl(struct file *file, unsigned int ioctl,
+			     unsigned long arg)
+{
+	return tpm_ioctl(file, ioctl, arg);
+}
+#endif
+
 /*
  * Called on file close
  */
@@ -169,6 +235,14 @@ static int tpm_release(struct inode *inode, struct file *file)
 {
 	struct file_priv *priv = file->private_data;
 
+	if (tpm_try_get_ops(priv->chip)) {
+		mutex_unlock(&priv->buffer_mutex);
+		return -EPIPE;
+	}
+	if (priv->has_space)
+		kfree(priv->space.context_buf);
+	tpm_put_ops(priv->chip);
+
 	del_singleshot_timer_sync(&priv->user_read_timer);
 	flush_work(&priv->work);
 	file->private_data = NULL;
@@ -184,6 +258,10 @@ const struct file_operations tpm_fops = {
 	.open = tpm_open,
 	.read = tpm_read,
 	.write = tpm_write,
+	.unlocked_ioctl = tpm_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = tpm_compat_ioctl,
+#endif
 	.release = tpm_release,
 };
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 0794a5d3..a1ae57e 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -370,10 +370,11 @@ static bool tpm_validate_command(struct tpm_chip *chip, const u8 *cmd,
  *     0 when the operation is successful.
  *     A negative number for system errors (errno).
  */
-ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
-		     unsigned int flags)
+ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
+		     u8 *buf, size_t bufsiz, unsigned int flags)
 {
-	ssize_t rc;
+	int rc;
+	ssize_t len = 0;
 	u32 count, ordinal;
 	unsigned long stop;
 
@@ -399,10 +400,14 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 	if (chip->dev.parent)
 		pm_runtime_get_sync(chip->dev.parent);
 
+	rc = tpm2_prepare_space(chip, space, ordinal, buf, bufsiz);
+	if (rc)
+		goto out;
+
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
 		dev_err(&chip->dev,
-			"tpm_transmit: tpm_send: error %zd\n", rc);
+			"tpm_transmit: tpm_send: error %d\n", rc);
 		goto out;
 	}
 
@@ -435,17 +440,23 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 	goto out;
 
 out_recv:
-	rc = chip->ops->recv(chip, (u8 *) buf, bufsiz);
-	if (rc < 0)
+	len = chip->ops->recv(chip, (u8 *) buf, bufsiz);
+	if (len < 0) {
 		dev_err(&chip->dev,
-			"tpm_transmit: tpm_recv: error %zd\n", rc);
+			"tpm_transmit: tpm_recv: error %d\n", rc);
+		rc = len;
+		goto out;
+	}
+
+	rc = tpm2_commit_space(chip, space, ordinal, buf, bufsiz);
+
 out:
 	if (chip->dev.parent)
 		pm_runtime_put_sync(chip->dev.parent);
 
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_unlock(&chip->tpm_mutex);
-	return rc;
+	return rc ? rc : len;
 }
 
 /**
@@ -463,13 +474,14 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
  *     A negative number for system errors (errno).
  *     A positive number for a TPM error.
  */
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd,
-			 int len, unsigned int flags, const char *desc)
+ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
+			 void *cmd, int len, unsigned int flags,
+			 const char *desc)
 {
 	const struct tpm_output_header *header;
 	int err;
 
-	len = tpm_transmit(chip, (const u8 *)cmd, len, flags);
+	len = tpm_transmit(chip, space, cmd, len, flags);
 	if (len <  0)
 		return len;
 	else if (len < TPM_HEADER_SIZE)
@@ -521,7 +533,7 @@ ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
 		tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
 		tpm_cmd.params.getcap_in.subcap = cpu_to_be32(subcap_id);
 	}
-	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, 0,
 			      desc);
 	if (!rc)
 		*cap = tpm_cmd.params.getcap_out.cap;
@@ -545,8 +557,9 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
 	start_cmd.header.in = tpm_startup_header;
 
 	start_cmd.params.startup_in.startup_type = startup_type;
-	return tpm_transmit_cmd(chip, &start_cmd, TPM_INTERNAL_RESULT_SIZE, 0,
-				"attempting to start the TPM");
+	return tpm_transmit_cmd(chip, NULL, &start_cmd,
+				TPM_INTERNAL_RESULT_SIZE,
+				0, "attempting to start the TPM");
 }
 
 int tpm_get_timeouts(struct tpm_chip *chip)
@@ -684,8 +697,8 @@ static int tpm_continue_selftest(struct tpm_chip *chip)
 	struct tpm_cmd_t cmd;
 
 	cmd.header.in = continue_selftest_header;
-	rc = tpm_transmit_cmd(chip, &cmd, CONTINUE_SELFTEST_RESULT_SIZE, 0,
-			      "continue selftest");
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, CONTINUE_SELFTEST_RESULT_SIZE,
+			      0, "continue selftest");
 	return rc;
 }
 
@@ -704,7 +717,7 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 
 	cmd.header.in = pcrread_header;
 	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
-	rc = tpm_transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, READ_PCR_RESULT_SIZE, 0,
 			      "attempting to read a pcr value");
 
 	if (rc == 0)
@@ -802,7 +815,7 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
 	cmd.header.in = pcrextend_header;
 	cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
 	memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE);
-	rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, EXTEND_PCR_RESULT_SIZE, 0,
 			      "attempting extend a PCR value");
 
 	tpm_put_ops(chip);
@@ -906,7 +919,7 @@ int tpm_send(u32 chip_num, void *cmd, size_t buflen)
 	if (chip == NULL)
 		return -ENODEV;
 
-	rc = tpm_transmit_cmd(chip, cmd, buflen, 0, "attempting tpm_cmd");
+	rc = tpm_transmit_cmd(chip, NULL, cmd, buflen, 0, "attempting tpm_cmd");
 
 	tpm_put_ops(chip);
 	return rc;
@@ -1008,15 +1021,15 @@ int tpm_pm_suspend(struct device *dev)
 		cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(tpm_suspend_pcr);
 		memcpy(cmd.params.pcrextend_in.hash, dummy_hash,
 		       TPM_DIGEST_SIZE);
-		rc = tpm_transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE, 0,
-				      "extending dummy pcr before suspend");
+		rc = tpm_transmit_cmd(chip, NULL, &cmd, EXTEND_PCR_RESULT_SIZE,
+				      0, "extending dummy pcr before suspend");
 	}
 
 	/* now do the actual savestate */
 	for (try = 0; try < TPM_RETRY; try++) {
 		cmd.header.in = savestate_header;
-		rc = tpm_transmit_cmd(chip, &cmd, SAVESTATE_RESULT_SIZE, 0,
-				      NULL);
+		rc = tpm_transmit_cmd(chip, NULL, &cmd, SAVESTATE_RESULT_SIZE,
+				      0, NULL);
 
 		/*
 		 * If the TPM indicates that it is too busy to respond to
@@ -1099,7 +1112,7 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
 		tpm_cmd.header.in = tpm_getrandom_header;
 		tpm_cmd.params.getrandom_in.num_bytes = cpu_to_be32(num_bytes);
 
-		err = tpm_transmit_cmd(chip, &tpm_cmd,
+		err = tpm_transmit_cmd(chip, NULL, &tpm_cmd,
 				       TPM_GETRANDOM_RESULT_SIZE + num_bytes,
 				       0, "attempting get random");
 		if (err)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 848ad65..dd31a00 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -39,7 +39,7 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	tpm_cmd.header.in = tpm_readpubek_header;
-	err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, 0,
+	err = tpm_transmit_cmd(chip, NULL, &tpm_cmd, READ_PUBEK_RESULT_SIZE, 0,
 			       "attempting to read the PUBEK");
 	if (err)
 		goto out;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index fb02b57..c6171e5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -91,6 +91,7 @@ enum tpm2_structures {
 
 enum tpm2_return_codes {
 	TPM2_RC_HASH		= 0x0083, /* RC_FMT1 */
+	TPM2_RC_HANDLE		= 0x008B,
 	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
 	TPM2_RC_DISABLED	= 0x0120,
 	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
@@ -114,6 +115,8 @@ enum tpm2_command_codes {
 	TPM2_CC_CREATE		= 0x0153,
 	TPM2_CC_LOAD		= 0x0157,
 	TPM2_CC_UNSEAL		= 0x015E,
+	TPM2_CC_CONTEXT_LOAD	= 0x0161,
+	TPM2_CC_CONTEXT_SAVE	= 0x0162,
 	TPM2_CC_FLUSH_CONTEXT	= 0x0165,
 	TPM2_CC_GET_CAPABILITY	= 0x017A,
 	TPM2_CC_GET_RANDOM	= 0x017B,
@@ -151,6 +154,11 @@ enum tpm2_cc_attrs {
 
 #define TPM_PPI_VERSION_LEN		3
 
+struct tpm_space {
+	u32 context_tbl[14];
+	u8 *context_buf;
+};
+
 enum tpm_chip_flags {
 	TPM_CHIP_FLAG_TPM2		= BIT(1),
 	TPM_CHIP_FLAG_IRQ		= BIT(2),
@@ -217,6 +225,7 @@ struct tpm_chip {
 #endif /* CONFIG_ACPI */
 
 	struct tpm_buf tr_buf;
+	struct tpm_space work_space;
 	u32 nr_commands;
 	u32 *cc_attrs_tbl;
 };
@@ -492,10 +501,11 @@ enum tpm_transmit_flags {
 	TPM_TRANSMIT_UNLOCKED	= BIT(0),
 };
 
-ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
-		     unsigned int flags);
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd, int len,
-			 unsigned int flags, const char *desc);
+ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
+		     u8 *buf, size_t bufsiz, unsigned int flags);
+ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
+			 void *cmd, int len, unsigned int flags,
+			 const char *desc);
 ssize_t tpm_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
 		   const char *desc);
 int tpm_get_timeouts(struct tpm_chip *);
@@ -549,4 +559,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
 int tpm2_probe(struct tpm_chip *chip);
 bool tpm2_find_cc_attrs(struct tpm_chip *chip, u32 cc, u32 *attrs);
+int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
+		       u32 cc, u8 *buf, size_t bufsiz);
+int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
+		      u32 cc, u8 *buf, size_t bufsiz);
 #endif
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 311dc8e..abaa355 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -280,7 +280,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 	       sizeof(cmd.params.pcrread_in.pcr_select));
 	cmd.params.pcrread_in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0,
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0,
 			      "attempting to read a pcr value");
 	if (rc == 0) {
 		buf = cmd.params.pcrread_out.digest;
@@ -327,7 +327,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 	cmd.params.pcrextend_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
 	memcpy(cmd.params.pcrextend_in.digest, hash, TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0,
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0,
 			      "attempting extend a PCR value");
 
 	return rc;
@@ -373,7 +373,7 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 		cmd.header.in = tpm2_getrandom_header;
 		cmd.params.getrandom_in.size = cpu_to_be16(num_bytes);
 
-		err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0,
+		err = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0,
 				       "attempting get random");
 		if (err)
 			break;
@@ -416,7 +416,7 @@ void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 	tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
 	tpm_buf_append_u32(&chip->tr_buf, handle);
 
-	rc = tpm_transmit_cmd(chip, chip->tr_buf.data, TPM_BUFSIZE, flags,
+	rc = tpm_transmit_cmd(chip, NULL, chip->tr_buf.data, TPM_BUFSIZE, flags,
 			      "flushing context");
 	if (rc)
 		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
@@ -529,7 +529,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	if (chip->tr_buf.flags & TPM_BUF_OVERFLOW)
 		return -E2BIG;
 
-	rc = tpm_transmit_cmd(chip, &chip->tr_buf, TPM_BUFSIZE, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &chip->tr_buf, TPM_BUFSIZE, 0,
 			      "sealing data");
 	if (rc)
 		return rc;
@@ -599,7 +599,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 	if (chip->tr_buf.flags & TPM_BUF_OVERFLOW)
 		return -E2BIG;
 
-	rc = tpm_transmit_cmd(chip, &chip->tr_buf, TPM_BUFSIZE, flags,
+	rc = tpm_transmit_cmd(chip, NULL, &chip->tr_buf, TPM_BUFSIZE, flags,
 			      "loading blob");
 	if (!rc)
 		*blob_handle = be32_to_cpup(
@@ -644,7 +644,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, chip->tr_buf.data, TPM_BUFSIZE, flags,
+	rc = tpm_transmit_cmd(chip, NULL, chip->tr_buf.data, TPM_BUFSIZE, flags,
 			      "unsealing");
 	if (rc > 0)
 		rc = -EPERM;
@@ -712,7 +712,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
 	cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id);
 	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, desc);
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0, desc);
 	if (!rc)
 		*value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);
 
@@ -746,7 +746,7 @@ static int tpm2_startup(struct tpm_chip *chip, u16 startup_type)
 	cmd.header.in = tpm2_startup_header;
 
 	cmd.params.startup_in.startup_type = cpu_to_be16(startup_type);
-	return tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0,
+	return tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0,
 				"attempting to start the TPM");
 }
 
@@ -775,7 +775,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
 	cmd.header.in = tpm2_shutdown_header;
 	cmd.params.startup_in.startup_type = cpu_to_be16(shutdown_type);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, "stopping the TPM");
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0,
+			      "stopping the TPM");
 
 	/* In places where shutdown command is sent there's no much we can do
 	 * except print the error code on a system failure.
@@ -838,7 +839,7 @@ static int tpm2_start_selftest(struct tpm_chip *chip, bool full)
 	cmd.header.in = tpm2_selftest_header;
 	cmd.params.selftest_in.full_test = full;
 
-	rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE, 0,
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, TPM2_SELF_TEST_IN_SIZE, 0,
 			      "continue selftest");
 
 	/* At least some prototype chips seem to give RC_TESTING error
@@ -889,7 +890,7 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
 		cmd.params.pcrread_in.pcr_select[1] = 0x00;
 		cmd.params.pcrread_in.pcr_select[2] = 0x00;
 
-		rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL);
+		rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd), 0, NULL);
 		if (rc < 0)
 			break;
 
@@ -922,7 +923,7 @@ int tpm2_probe(struct tpm_chip *chip)
 	cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100);
 	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),  0, NULL);
+	rc = tpm_transmit_cmd(chip, NULL, &cmd, sizeof(cmd),  0, NULL);
 	if (rc <  0)
 		return rc;
 
@@ -981,7 +982,8 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 	tpm_buf_append_u32(&chip->tr_buf, TPM2_CC_FIRST);
 	tpm_buf_append_u32(&chip->tr_buf, nr_commands);
 
-	rc = tpm_transmit_cmd(chip, chip->tr_buf.data, TPM_BUFSIZE, 0, NULL);
+	rc = tpm_transmit_cmd(chip, NULL, chip->tr_buf.data, TPM_BUFSIZE, 0,
+			      NULL);
 	if (rc < 0)
 		goto out;
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
new file mode 100644
index 0000000..12a84e6
--- /dev/null
+++ b/drivers/char/tpm/tpm2-space.c
@@ -0,0 +1,288 @@
+/*
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Authors:
+ * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
+ *
+ * Maintained by: <tpmdd-devel@lists.sourceforge.net>
+ *
+ * This file contains TPM2 protocol implementations of the commands
+ * used by the kernel internally.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#include <linux/gfp.h>
+#include "tpm.h"
+
+enum tpm2_handle_types {
+	TPM2_HT_HMAC_SESSION	= 0x02000000,
+	TPM2_HT_POLICY_SESSION	= 0x03000000,
+	TPM2_HT_TRANSIENT	= 0x80000000,
+};
+
+static void tpm2_flush_space(struct tpm_chip *chip)
+{
+	struct tpm_space *space = &chip->work_space;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
+		if (space->context_tbl[i] && ~space->context_tbl[i])
+			tpm2_flush_context_cmd(chip, space->context_tbl[i],
+					       TPM_TRANSMIT_UNLOCKED);
+}
+
+struct tpm2_context {
+	__be64 sequence;
+	__be32 saved_handle;
+	__be32 hierarchy;
+	__be16 blob_size;
+} __packed;
+
+static int tpm2_load_space(struct tpm_chip *chip)
+{
+	struct tpm_space *space = &chip->work_space;
+	struct tpm2_context *ctx;
+	int i;
+	int j;
+	int rc;
+	u32 s;
+
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
+		if (!space->context_tbl[i])
+			continue;
+
+		/* sanity check, should never happen */
+		if (~space->context_tbl[i]) {
+			dev_err(&chip->dev, "context table is inconsistent");
+			return -EFAULT;
+		}
+
+		tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS,
+			     TPM2_CC_CONTEXT_LOAD);
+
+		ctx = (struct tpm2_context *)&space->context_buf[j];
+		s = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
+		tpm_buf_append(&chip->tr_buf, &space->context_buf[j], s);
+
+		rc = tpm_transmit_cmd(chip, NULL, chip->tr_buf.data, PAGE_SIZE,
+				      TPM_TRANSMIT_UNLOCKED, NULL);
+		if (rc) {
+			dev_warn(&chip->dev, "%s: loading failed with %d\n",
+				 __func__, rc);
+			rc = -EFAULT;
+			goto out_err;
+		}
+
+		space->context_tbl[i] =
+			be32_to_cpup(
+				(__be32 *)&chip->tr_buf.data[TPM_HEADER_SIZE]);
+
+		j += s;
+	}
+
+	return 0;
+
+out_err:
+	tpm2_flush_space(chip);
+	return rc;
+}
+
+static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t len)
+{
+	struct tpm_space *space = &chip->work_space;
+	unsigned int nr_handles;
+	u32 vhandle;
+	u32 phandle;
+	u32 attrs;
+	int i;
+	int j;
+	int rc;
+
+	if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
+		rc = -EINVAL;
+		goto out_err;
+	}
+
+	nr_handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
+
+	for (i = 0; i < nr_handles; i++) {
+		vhandle = be32_to_cpup((__be32 *)&cmd[TPM_HEADER_SIZE + 4 * i]);
+		if ((vhandle & 0xFF000000) != TPM2_HT_TRANSIENT)
+			continue;
+
+		j = 0xFFFFFF - (vhandle & 0xFFFFFF);
+		if (j > ARRAY_SIZE(space->context_tbl) ||
+		    !space->context_tbl[j]) {
+			rc = -EINVAL;
+			goto out_err;
+		}
+
+		phandle = space->context_tbl[j];
+		*((__be32 *)&cmd[TPM_HEADER_SIZE + 4 * i]) =
+			cpu_to_be32(phandle);
+	}
+
+	return 0;
+
+out_err:
+	tpm2_flush_space(chip);
+	return rc;
+}
+
+int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
+		       u32 cc, u8 *buf, size_t bufsiz)
+{
+	int rc;
+
+	if (!space)
+		return 0;
+
+	memcpy(&chip->work_space.context_tbl, &space->context_tbl,
+	       sizeof(space->context_tbl));
+	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
+
+	rc = tpm2_load_space(chip);
+	if (rc)
+		return rc;
+
+	rc = tpm2_map_command(chip, cc, buf, bufsiz);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
+{
+	struct tpm_space *space = &chip->work_space;
+	u32 phandle;
+	u32 vhandle;
+	u32 attrs;
+	int i;
+	int rc;
+
+	if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
+		/* should never happen */
+		dev_err(&chip->dev, "TPM returned a different CC: 0x%04x\n",
+			cc);
+		rc = -EFAULT;
+		goto out_err;
+	}
+
+	if (!((attrs >> TPM2_CC_ATTR_RHANDLE) & 1))
+		return 0;
+
+	phandle = be32_to_cpup((__be32 *)&rsp[TPM_HEADER_SIZE]);
+	if ((phandle & 0xFF000000) != TPM2_HT_TRANSIENT)
+		return 0;
+
+	/* Garbage collect a dead context. */
+	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
+		if (space->context_tbl[i] == phandle) {
+			space->context_tbl[i] = 0;
+			break;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
+		if (!space->context_tbl[i])
+			break;
+
+	if (i == ARRAY_SIZE(space->context_tbl)) {
+		dev_warn(&chip->dev, "%s: out of context slots\n", __func__);
+		tpm2_flush_context_cmd(chip, phandle, TPM_TRANSMIT_UNLOCKED);
+		rc = -ENOMEM;
+		goto out_err;
+	}
+
+	space->context_tbl[i] = phandle;
+	vhandle = TPM2_HT_TRANSIENT | (0xFFFFFF - i);
+	*(__be32 *)&rsp[TPM_HEADER_SIZE] = cpu_to_be32(vhandle);
+
+	return 0;
+
+out_err:
+	tpm2_flush_space(chip);
+	return rc;
+}
+
+static int tpm2_save_space(struct tpm_chip *chip)
+{
+	struct tpm_space *space = &chip->work_space;
+	int i;
+	int j;
+	int rc;
+	u32 s;
+
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
+		if (!(space->context_tbl[i] && ~space->context_tbl[i]))
+			continue;
+
+		tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS,
+			     TPM2_CC_CONTEXT_SAVE);
+		tpm_buf_append_u32(&chip->tr_buf, space->context_tbl[i]);
+
+		rc = tpm_transmit_cmd(chip, NULL, chip->tr_buf.data, PAGE_SIZE,
+				      TPM_TRANSMIT_UNLOCKED, NULL);
+		if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
+			space->context_tbl[i] = 0;
+			continue;
+		} else if (rc) {
+			dev_warn(&chip->dev, "%s: saving failed with %d\n",
+				 __func__, rc);
+			rc = -EFAULT;
+			goto out_err;
+		}
+
+		s = tpm_buf_length(&chip->tr_buf) - TPM_HEADER_SIZE;
+		if ((j + s) > PAGE_SIZE) {
+			dev_warn(&chip->dev, "%s: out of backing storage\n",
+				 __func__);
+			rc = -ENOMEM;
+			goto out_err;
+		}
+
+		memcpy(&space->context_buf[j],
+		       &chip->tr_buf.data[TPM_HEADER_SIZE], s);
+
+		tpm2_flush_context_cmd(chip, space->context_tbl[i],
+				       TPM_TRANSMIT_UNLOCKED);
+
+		space->context_tbl[i] = ~0;
+
+		j += s;
+	}
+
+	return 0;
+
+out_err:
+	tpm2_flush_space(chip);
+	return rc;
+}
+
+int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
+		      u32 cc, u8 *buf, size_t bufsiz)
+{
+	int rc;
+
+	if (!space)
+		return 0;
+
+	rc = tpm2_map_response(chip, cc, buf, bufsiz);
+	if (rc)
+		return rc;
+
+	rc = tpm2_save_space(chip);
+	if (rc)
+		return rc;
+
+	memcpy(&space->context_tbl, &chip->work_space.context_tbl,
+	       sizeof(space->context_tbl));
+	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
+
+	return 0;
+}
diff --git a/include/uapi/linux/tpm.h b/include/uapi/linux/tpm.h
new file mode 100644
index 0000000..1df5b61
--- /dev/null
+++ b/include/uapi/linux/tpm.h
@@ -0,0 +1,23 @@
+/*
+ * API and definitions for the TPM device driver
+ * Copyright (C) 2016 Intel Corporation
+ *
+ * Authors:
+ * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#ifndef _UAPI_TPM_H
+#define _UAPI_TPM_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define TPM_IOC_MAGIC 0xa2
+#define TPM_IOC_NEW_SPACE _IO(TPM_IOC_MAGIC, 0x00)
+
+#endif /* _UAPI_TPM_H */
-- 
2.9.3

Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jason Gunthorpe]

On Mon, Jan 02, 2017 at 03:22:10PM +0200, Jarkko Sakkinen wrote:
> Added a ioctl for creating a TPM space. The space is isolated from the
> other users of the TPM. Only a process holding the file with the handle
> can access the objects and only objects that are created through that
> file handle can be accessed.

I don't understand this comment. /dev/tpmX is forced to be
single-process-open, so how can there ever be more than 1 FD for it?

Since the space is tied to that single fd these patches just create a
way for the single user-space process to auto-cleanup if it crashes?

Is that the entire intent of this design? I guess it is OK as a
stepping point..

> -ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
> -		     unsigned int flags)
> +ssize_t tpm_transmit(struct tpm_chip *chip, struct tpm_space *space,
> +		     u8 *buf, size_t bufsiz, unsigned int flags)

Please split this patch so that 'struct tpm_space' introduction is in
its own patch and the actual UAPI change is in a much smaller
patch. It is very hard to see the uapi stuff in all of this churn.

Jason

Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jarkko Sakkinen]

On Mon, Jan 02, 2017 at 02:09:53PM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 02, 2017 at 03:22:10PM +0200, Jarkko Sakkinen wrote:
> > Added a ioctl for creating a TPM space. The space is isolated from the
> > other users of the TPM. Only a process holding the file with the handle
> > can access the objects and only objects that are created through that
> > file handle can be accessed.
> 
> I don't understand this comment. /dev/tpmX is forced to be
> single-process-open, so how can there ever be more than 1 FD for it?
> 
> Since the space is tied to that single fd these patches just create a
> way for the single user-space process to auto-cleanup if it crashes?
> 
> Is that the entire intent of this design? I guess it is OK as a
> stepping point..

is_open is cleared in tpm_ioc_new_space.

/Jarkko

Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jason Gunthorpe]

On Tue, Jan 03, 2017 at 02:37:30AM +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 02, 2017 at 02:09:53PM -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 02, 2017 at 03:22:10PM +0200, Jarkko Sakkinen wrote:
> > > Added a ioctl for creating a TPM space. The space is isolated from the
> > > other users of the TPM. Only a process holding the file with the handle
> > > can access the objects and only objects that are created through that
> > > file handle can be accessed.
> > 
> > I don't understand this comment. /dev/tpmX is forced to be
> > single-process-open, so how can there ever be more than 1 FD for it?
> > 
> > Since the space is tied to that single fd these patches just create a
> > way for the single user-space process to auto-cleanup if it crashes?
> > 
> > Is that the entire intent of this design? I guess it is OK as a
> > stepping point..
> 
> is_open is cleared in tpm_ioc_new_space.

That is no good, it is racy if the intention is to use multiple
clients, and any single client that doesn't support the new API blocks
all access.

Jason

Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 11:46:27AM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 03, 2017 at 02:37:30AM +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 02:09:53PM -0700, Jason Gunthorpe wrote:
> > > On Mon, Jan 02, 2017 at 03:22:10PM +0200, Jarkko Sakkinen wrote:
> > > > Added a ioctl for creating a TPM space. The space is isolated from the
> > > > other users of the TPM. Only a process holding the file with the handle
> > > > can access the objects and only objects that are created through that
> > > > file handle can be accessed.
> > > 
> > > I don't understand this comment. /dev/tpmX is forced to be
> > > single-process-open, so how can there ever be more than 1 FD for it?
> > > 
> > > Since the space is tied to that single fd these patches just create a
> > > way for the single user-space process to auto-cleanup if it crashes?
> > > 
> > > Is that the entire intent of this design? I guess it is OK as a
> > > stepping point..
> > 
> > is_open is cleared in tpm_ioc_new_space.
> 
> That is no good, it is racy if the intention is to use multiple
> clients, and any single client that doesn't support the new API blocks
> all access.

Luckily this will be implicitly fixed with a separate device file 
in the non-RFC patch set :-)

> Jason

/Jarkko

Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jason Gunthorpe]

On Tue, Jan 03, 2017 at 02:37:30AM +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 02, 2017 at 02:09:53PM -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 02, 2017 at 03:22:10PM +0200, Jarkko Sakkinen wrote:
> > > Added a ioctl for creating a TPM space. The space is isolated from the
> > > other users of the TPM. Only a process holding the file with the handle
> > > can access the objects and only objects that are created through that
> > > file handle can be accessed.
> > 
> > I don't understand this comment. /dev/tpmX is forced to be
> > single-process-open, so how can there ever be more than 1 FD for it?
> > 
> > Since the space is tied to that single fd these patches just create a
> > way for the single user-space process to auto-cleanup if it crashes?
> > 
> > Is that the entire intent of this design? I guess it is OK as a
> > stepping point..
> 
> is_open is cleared in tpm_ioc_new_space.

There is also a bug with the uncondtional clear of is_open in
tpm_release - this cannot happen if the ioctl is done - but I think
this approach of using an ioctl is not a good idea.

I have pondered using an open flag in the past - what about using
something like O_EXCL to indicate that the fd is to be used in
resource sharing mode? Not sure if that would be considered abuse of
the open flags or not.

Jason

Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 12:16:34PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 03, 2017 at 02:37:30AM +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 02:09:53PM -0700, Jason Gunthorpe wrote:
> > > On Mon, Jan 02, 2017 at 03:22:10PM +0200, Jarkko Sakkinen wrote:
> > > > Added a ioctl for creating a TPM space. The space is isolated from the
> > > > other users of the TPM. Only a process holding the file with the handle
> > > > can access the objects and only objects that are created through that
> > > > file handle can be accessed.
> > > 
> > > I don't understand this comment. /dev/tpmX is forced to be
> > > single-process-open, so how can there ever be more than 1 FD for it?
> > > 
> > > Since the space is tied to that single fd these patches just create a
> > > way for the single user-space process to auto-cleanup if it crashes?
> > > 
> > > Is that the entire intent of this design? I guess it is OK as a
> > > stepping point..
> > 
> > is_open is cleared in tpm_ioc_new_space.
> 
> There is also a bug with the uncondtional clear of is_open in
> tpm_release - this cannot happen if the ioctl is done - but I think
> this approach of using an ioctl is not a good idea.
> 
> I have pondered using an open flag in the past - what about using
> something like O_EXCL to indicate that the fd is to be used in
> resource sharing mode? Not sure if that would be considered abuse of
> the open flags or not.

I've now leaned toward James' idea of having a separate /dev/tpms0.

> Jason

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0

[Jarkko Sakkinen]

On Wed, Jan 04, 2017 at 12:50:21PM -0500, Stefan Berger wrote:
>    Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 01/02/2017
>    08:22:10 AM:
> 
>    >
>    > Added a ioctl for creating a TPM space. The space is isolated from the
>    > other users of the TPM. Only a process holding the file with the handle
>    > can access the objects and only objects that are created through that
>    > file handle can be accessed.
>    >
>    > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>    > ---
> 
>    > diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
>    > index 912ad30..139638b 100644
>    > --- a/drivers/char/tpm/tpm-dev.c
>    > +++ b/drivers/char/tpm/tpm-dev.c
>    > @@ -19,6 +19,7 @@
>    >   */
>    >  #include <linux/slab.h>
>    >  #include <linux/uaccess.h>
>    > +#include <uapi/linux/tpm.h>
>    >  #include "tpm.h"
>    >  
>    >  struct file_priv {
>    > @@ -32,6 +33,8 @@ struct file_priv {
>    >     struct work_struct work;
>    >  
>    >     u8 data_buffer[TPM_BUFSIZE];
>    > +   struct tpm_space space;
>    > +   bool has_space;
>    >  };
>    >  
>    >  static void user_reader_timeout(unsigned long ptr)
>    > @@ -115,6 +118,7 @@ static ssize_t tpm_write(struct file *file,
>    > const char __user *buf,
>    >            size_t size, loff_t *off)
>    >  {
>    >     struct file_priv *priv = file->private_data;
>    > +   struct tpm_space *space = NULL;
>    >     size_t in_size = size;
>    >     ssize_t out_size;
>    >  
>    > @@ -130,6 +134,9 @@ static ssize_t tpm_write(struct file *file,
>    > const char __user *buf,
>    >  
>    >     mutex_lock(&priv->buffer_mutex);
>    >  
>    > +   if (priv->has_space)
>    > +      space = &priv->space;
>    > +
>    >     if (copy_from_user
>    >         (priv->data_buffer, (void __user *) buf, in_size)) {
>    >        mutex_unlock(&priv->buffer_mutex);
>    > @@ -144,7 +151,7 @@ static ssize_t tpm_write(struct file *file,
>    > const char __user *buf,
>    >        mutex_unlock(&priv->buffer_mutex);
>    >        return -EPIPE;
>    >     }
>    > -   out_size = tpm_transmit(priv->chip, priv->data_buffer,
>    > +   out_size = tpm_transmit(priv->chip, space, priv->data_buffer,
>    >              sizeof(priv->data_buffer), 0);
>    >  
>    >     tpm_put_ops(priv->chip);
>    > @@ -162,6 +169,65 @@ static ssize_t tpm_write(struct file *file,
>    > const char __user *buf,
>    >     return in_size;
>    >  }
>    >  
>    > +/**
>    > + * tpm_ioc_new_space - handler for %SGX_IOC_NEW_SPACE ioctl
>    > + *
>    > + * Creates a new TPM space that can hold a set of transient
>    > objects. The space
>    > + * is isolated with virtual handles that are mapped into physical
>    > handles by the
>    > + * driver.
>    > + */
>    > +static long tpm_ioc_new_space(struct file *file, unsigned int ioctl,
>    > +               unsigned long arg)
>    > +{
>    > +   struct file_priv *priv = file->private_data;
>    > +   struct tpm_chip *chip = priv->chip;
>    > +   int rc = 0;
>    > +
>    > +   if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
>    > +      return -EOPNOTSUPP;
>    > +
>    > +   mutex_lock(&priv->buffer_mutex);
>    > +
>    > +   if (priv->has_space) {
>    > +      rc = -EBUSY;
>    > +      goto out;
>    > +   }
>    > +
>    > +   priv->space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>    > +   if (!priv->space.context_buf) {
>    > +      rc = -ENOMEM;
>    > +      goto out;
>    > +   }
>    > +
>    > +   /* The TPM device can be opened again as this file has been moved to
>    a
>    > +    * TPM handle space.
>    > +    */
>    > +   priv->has_space = true;
>    > +   clear_bit(0, &chip->is_open);
>    > +out:
>    > +   mutex_unlock(&priv->buffer_mutex);
>    > +   return rc;
>    > +}
>    > +
>    > +static long tpm_ioctl(struct file *file, unsigned int ioctl,
>    > +            unsigned long arg)
>    > +{
>    > +   switch (ioctl) {
>    > +   case TPM_IOC_NEW_SPACE:
>    > +      return tpm_ioc_new_space(file, ioctl, arg);
>    > +   default:
>    > +      return -ENOIOCTLCMD;
>    > +   }
>    > +}
>    > +
>    > +#ifdef CONFIG_COMPAT
>    > +static long tpm_compat_ioctl(struct file *file, unsigned int ioctl,
>    > +              unsigned long arg)
>    > +{
>    > +   return tpm_ioctl(file, ioctl, arg);
>    > +}
>    > +#endif
>    > +
>    >  /*
>    >   * Called on file close
>    >   */
>    > @@ -169,6 +235,14 @@ static int tpm_release(struct inode *inode,
>    > struct file *file)
>    >  {
>    >     struct file_priv *priv = file->private_data;
>    >  
>    > +   if (tpm_try_get_ops(priv->chip)) {
>    > +      mutex_unlock(&priv->buffer_mutex);
>    > +      return -EPIPE;
>    > +   }
> 
>    That mutex_unlock looks wrong.

Thanks.

This will be anyway gone with own device file.

>       Stefan

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> This patch set adds support for TPM spaces that provide a context
> for isolating and swapping transient objects. This patch set does
> not yet include support for isolating policy and HMAC sessions but
> it is trivial to add once the basic approach is settled (and that's
> why I created an RFC patch set).

The approach looks fine to me.  The only basic query I have is about
the default: shouldn't it be with resource manager on rather than off? 
 I can't really think of a use case that wants the RM off (even if
you're running your own, having another doesn't hurt anything, and it's
still required to share with in-kernel uses).

> There's a test script for trying out TPM spaces in
> 
>   git://git.infradead.org/users/jjs/tpm2-scripts.git
> 
> A simple smoke test can be run by
> 
>   sudo python -m unittest -v tpm2_smoke.SpaceTest   

I've also added an enabling patch to the tss

https://build.opensuse.org/package/view_file/home:jejb1:Tumbleweed/tss2/0002-tssProperties-add-TPM_USE_RESOURCE_MANAGER.patch?expand=1

And with that, I've TPM 2 enabled both gnome-keyring and openssl:

https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/gnome-keyring
https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/openssl_tpm_engine

I'm running them in production on my day to day laptop and so far
everything's working nicely (better than 1.2, in fact, since tcsd
periodically crashes necessitating a restart of everything).

So you can definitely add my Tested-By.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > This patch set adds support for TPM spaces that provide a context
> > for isolating and swapping transient objects. This patch set does
> > not yet include support for isolating policy and HMAC sessions but
> > it is trivial to add once the basic approach is settled (and that's
> > why I created an RFC patch set).
> 
> The approach looks fine to me.  The only basic query I have is about
> the default: shouldn't it be with resource manager on rather than off? 
>  I can't really think of a use case that wants the RM off (even if
> you're running your own, having another doesn't hurt anything, and it's
> still required to share with in-kernel uses).

This is a valid question and here's a longish explanation.

In TPM2_GetCapability and maybe couple of other commands you can get
handles in the response body. I do not want to have special cases in the
kernel for response bodies because there is no a generic way to do the
substitution. What's worse, new commands in the standard future
revisions could have such commands requiring special cases. In addition,
vendor specific commans could have handles in the response bodies.

It's better to leverage that to the user space. I would do only simple
and fail-safe stuff in the kernel.

Turning RM on by default would raise a backwards compatibility issue.

> 
> > There's a test script for trying out TPM spaces in
> > 
> >   git://git.infradead.org/users/jjs/tpm2-scripts.git
> > 
> > A simple smoke test can be run by
> > 
> >   sudo python -m unittest -v tpm2_smoke.SpaceTest   
> 
> I've also added an enabling patch to the tss
> 
> https://build.opensuse.org/package/view_file/home:jejb1:Tumbleweed/tss2/0002-tssProperties-add-TPM_USE_RESOURCE_MANAGER.patch?expand=1
> 
> And with that, I've TPM 2 enabled both gnome-keyring and openssl:
> 
> https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/gnome-keyring
> https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/openssl_tpm_engine
> 
> I'm running them in production on my day to day laptop and so far
> everything's working nicely (better than 1.2, in fact, since tcsd
> periodically crashes necessitating a restart of everything).

Great, thanks for doing this!

> So you can definitely add my Tested-By.

Thank you.

> James

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > This patch set adds support for TPM spaces that provide a context
> > > for isolating and swapping transient objects. This patch set does
> > > not yet include support for isolating policy and HMAC sessions 
> > > but it is trivial to add once the basic approach is settled (and
> > > that's why I created an RFC patch set).
> > 
> > The approach looks fine to me.  The only basic query I have is 
> > about the default: shouldn't it be with resource manager on rather 
> > than off?  I can't really think of a use case that wants the RM off 
> > (even if you're running your own, having another doesn't hurt 
> > anything, and it's still required to share with in-kernel uses).
> 
> This is a valid question and here's a longish explanation.
> 
> In TPM2_GetCapability and maybe couple of other commands you can get
> handles in the response body. I do not want to have special cases in 
> the kernel for response bodies because there is no a generic way to 
> do the substitution. What's worse, new commands in the standard 
> future revisions could have such commands requiring special cases. In
> addition, vendor specific commans could have handles in the response
> bodies.

OK, in general I buy this ... what you're effectively saying is that we
need a non-RM interface for certain management type commands.

However, let me expand a bit on why I'm fretting about the non-RM use
case.  Right at the moment, we have a single TPM device which you use
for access to the kernel TPM.  The current tss2 just makes direct use
of this, meaning it has to have 0666 permissions.  This means that any
local user can simply DoS the TPM by running us out of transient
resources if they don't activate the RM.  If they get a connection
always via the RM, this isn't a worry.  Perhaps the best way of fixing
this is to expose two separate device nodes: one raw to the TPM which
we could keep at 0600 and one with an always RM connection which we can
set to 0666.  That would mean that access to the non-RM connection is
either root only or governed by a system set ACL.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > This patch set adds support for TPM spaces that provide a 
> > > > context for isolating and swapping transient objects. This 
> > > > patch set does not yet include support for isolating policy and 
> > > > HMAC sessions but it is trivial to add once the basic approach 
> > > > is settled (and that's why I created an RFC patch set).
> > > 
> > > The approach looks fine to me.  The only basic query I have is 
> > > about the default: shouldn't it be with resource manager on 
> > > rather than off?  I can't really think of a use case that wants 
> > > the RM off (even if you're running your own, having another 
> > > doesn't hurt anything, and it's still required to share with in
> > > -kernel uses).
> > 
> > This is a valid question and here's a longish explanation.
> > 
> > In TPM2_GetCapability and maybe couple of other commands you can 
> > get handles in the response body. I do not want to have special 
> > cases in the kernel for response bodies because there is no a 
> > generic way to do the substitution. What's worse, new commands in 
> > the standard future revisions could have such commands requiring 
> > special cases. In addition, vendor specific commans could have 
> > handles in the response bodies.
> 
> OK, in general I buy this ... what you're effectively saying is that 
> we need a non-RM interface for certain management type commands.
> 
> However, let me expand a bit on why I'm fretting about the non-RM use
> case.  Right at the moment, we have a single TPM device which you use
> for access to the kernel TPM.  The current tss2 just makes direct use
> of this, meaning it has to have 0666 permissions.  This means that 
> any local user can simply DoS the TPM by running us out of transient
> resources if they don't activate the RM.  If they get a connection
> always via the RM, this isn't a worry.  Perhaps the best way of 
> fixing this is to expose two separate device nodes: one raw to the 
> TPM which we could keep at 0600 and one with an always RM connection 
> which we can set to 0666.  That would mean that access to the non-RM 
> connection is either root only or governed by a system set ACL.

OK, so I put a patch together that does this (see below). It all works
nicely (with a udev script that sets the resource manager device to
0666):

jejb@jarvis:~> ls -l /dev/tpm*
crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm

I've modified the tss to connect to /dev/tpm0rm by default and it all
seems to work.

The patch applies on top of your tabrm branch, by the way.

James

---

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ac4c05f..25b8d30 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,6 +33,7 @@ DEFINE_IDR(dev_nums_idr);
 static DEFINE_MUTEX(idr_lock);
 
 struct class *tpm_class;
+struct class *tpm_rm_class;
 dev_t tpm_devt;
 
 /**
@@ -169,27 +170,39 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	chip->dev_num = rc;
 
 	device_initialize(&chip->dev);
+	device_initialize(&chip->devrm);
 
 	chip->dev.class = tpm_class;
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = pdev;
 	chip->dev.groups = chip->groups;
 
+	chip->devrm.parent = pdev;
+	chip->devrm.class = tpm_rm_class;
+
 	if (chip->dev_num == 0)
 		chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
 	else
 		chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
 
+	chip->devrm.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);
+
 	rc = dev_set_name(&chip->dev, "tpm%d", chip->dev_num);
 	if (rc)
 		goto out;
+	rc = dev_set_name(&chip->devrm, "tpm%drm", chip->dev_num);
+	if (rc)
+		goto out;
 
 	if (!pdev)
 		chip->flags |= TPM_CHIP_FLAG_VIRTUAL;
 
 	cdev_init(&chip->cdev, &tpm_fops);
+	cdev_init(&chip->cdevrm, &tpm_rm_fops);
 	chip->cdev.owner = THIS_MODULE;
+	chip->cdevrm.owner = THIS_MODULE;
 	chip->cdev.kobj.parent = &chip->dev.kobj;
+	chip->cdevrm.kobj.parent = &chip->devrm.kobj;
 
 	chip->tr_buf.data = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
 	if (!chip->tr_buf.data) {
@@ -208,6 +221,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 
 out:
 	put_device(&chip->dev);
+	put_device(&chip->devrm);
 	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_GPL(tpm_chip_alloc);
@@ -252,7 +266,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 			dev_name(&chip->dev), MAJOR(chip->dev.devt),
 			MINOR(chip->dev.devt), rc);
 
-		return rc;
+		goto err_1;
 	}
 
 	rc = device_add(&chip->dev);
@@ -262,16 +276,44 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 			dev_name(&chip->dev), MAJOR(chip->dev.devt),
 			MINOR(chip->dev.devt), rc);
 
-		cdev_del(&chip->cdev);
-		return rc;
+		goto err_2;
+	}
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = cdev_add(&chip->cdevrm, chip->devrm.devt, 1);
+	if (rc) {
+		dev_err(&chip->dev,
+			"unable to cdev_add() %s, major %d, minor %d, err=%d\n",
+			dev_name(&chip->devrm), MAJOR(chip->devrm.devt),
+			MINOR(chip->devrm.devt), rc);
+
+		goto err_3;
 	}
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = device_add(&chip->devrm);
+	if (rc) {
+		dev_err(&chip->dev,
+			"unable to device_register() %s, major %d, minor %d, err=%d\n",
+			dev_name(&chip->devrm), MAJOR(chip->devrm.devt),
+			MINOR(chip->devrm.devt), rc);
+
+		goto err_4;
+	}
 	/* Make the chip available. */
 	mutex_lock(&idr_lock);
 	idr_replace(&dev_nums_idr, chip, chip->dev_num);
 	mutex_unlock(&idr_lock);
 
 	return rc;
+ err_4:
+	cdev_del(&chip->cdevrm);
+ err_3:
+	device_del(&chip->dev);
+ err_2:
+	cdev_del(&chip->cdev);
+ err_1:
+	return rc;
 }
 
 static void tpm_del_char_device(struct tpm_chip *chip)
@@ -279,6 +321,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 	cdev_del(&chip->cdev);
 	device_del(&chip->dev);
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		cdev_del(&chip->cdevrm);
+		device_del(&chip->devrm);
+	}
+
 	/* Make the chip unavailable. */
 	mutex_lock(&idr_lock);
 	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 139638b..bed29f9 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -54,23 +54,28 @@ static void timeout_work(struct work_struct *work)
 	mutex_unlock(&priv->buffer_mutex);
 }
 
-static int tpm_open(struct inode *inode, struct file *file)
+static int tpm_open_internal(struct inode *inode, struct file *file, bool is_rm)
 {
-	struct tpm_chip *chip =
-		container_of(inode->i_cdev, struct tpm_chip, cdev);
+	struct tpm_chip *chip;
 	struct file_priv *priv;
 
+	if (is_rm)
+		chip = container_of(inode->i_cdev, struct tpm_chip, cdevrm);
+	else
+		chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
+
 	/* It's assured that the chip will be opened just once,
 	 * by the check of is_open variable, which is protected
 	 * by driver_lock. */
-	if (test_and_set_bit(0, &chip->is_open)) {
+	if (!is_rm && test_and_set_bit(0, &chip->is_open)) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL) {
-		clear_bit(0, &chip->is_open);
+		if (!is_rm)
+			clear_bit(0, &chip->is_open);
 		return -ENOMEM;
 	}
 
@@ -82,9 +87,27 @@ static int tpm_open(struct inode *inode, struct file *file)
 	INIT_WORK(&priv->work, timeout_work);
 
 	file->private_data = priv;
+
+	if (is_rm) {
+		priv->space.context_buf =  kzalloc(PAGE_SIZE, GFP_KERNEL);
+		if (!priv->space.context_buf)
+			return -ENOMEM;
+		priv->has_space = true;
+	}
+
 	return 0;
 }
 
+static int tpm_open(struct inode *inode, struct file *file)
+{
+	return tpm_open_internal(inode, file, false);
+}
+
+static int tpm_rm_open(struct inode *inode, struct file *file)
+{
+	return tpm_open_internal(inode, file, true);
+}
+
 static ssize_t tpm_read(struct file *file, char __user *buf,
 			size_t size, loff_t *off)
 {
@@ -169,65 +192,6 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
 	return in_size;
 }
 
-/**
- * tpm_ioc_new_space - handler for %SGX_IOC_NEW_SPACE ioctl
- *
- * Creates a new TPM space that can hold a set of transient objects. The space
- * is isolated with virtual handles that are mapped into physical handles by the
- * driver.
- */
-static long tpm_ioc_new_space(struct file *file, unsigned int ioctl,
-			      unsigned long arg)
-{
-	struct file_priv *priv = file->private_data;
-	struct tpm_chip *chip = priv->chip;
-	int rc = 0;
-
-	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
-		return -EOPNOTSUPP;
-
-	mutex_lock(&priv->buffer_mutex);
-
-	if (priv->has_space) {
-		rc = -EBUSY;
-		goto out;
-	}
-
-	priv->space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!priv->space.context_buf) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
-	/* The TPM device can be opened again as this file has been moved to a
-	 * TPM handle space.
-	 */
-	priv->has_space = true;
-	clear_bit(0, &chip->is_open);
-out:
-	mutex_unlock(&priv->buffer_mutex);
-	return rc;
-}
-
-static long tpm_ioctl(struct file *file, unsigned int ioctl,
-		      unsigned long arg)
-{
-	switch (ioctl) {
-	case TPM_IOC_NEW_SPACE:
-		return tpm_ioc_new_space(file, ioctl, arg);
-	default:
-		return -ENOIOCTLCMD;
-	}
-}
-
-#ifdef CONFIG_COMPAT
-static long tpm_compat_ioctl(struct file *file, unsigned int ioctl,
-			     unsigned long arg)
-{
-	return tpm_ioctl(file, ioctl, arg);
-}
-#endif
-
 /*
  * Called on file close
  */
@@ -247,7 +211,8 @@ static int tpm_release(struct inode *inode, struct file *file)
 	flush_work(&priv->work);
 	file->private_data = NULL;
 	atomic_set(&priv->data_pending, 0);
-	clear_bit(0, &priv->chip->is_open);
+	if (!priv->has_space)
+		clear_bit(0, &priv->chip->is_open);
 	kfree(priv);
 	return 0;
 }
@@ -258,10 +223,15 @@ const struct file_operations tpm_fops = {
 	.open = tpm_open,
 	.read = tpm_read,
 	.write = tpm_write,
-	.unlocked_ioctl = tpm_ioctl,
-#ifdef CONFIG_COMPAT
-	.compat_ioctl = tpm_compat_ioctl,
-#endif
+	.release = tpm_release,
+};
+
+const struct file_operations tpm_rm_fops = {
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.open = tpm_rm_open,
+	.read = tpm_read,
+	.write = tpm_write,
 	.release = tpm_release,
 };
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index a1ae57e..c1829de 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1194,9 +1194,17 @@ static int __init tpm_init(void)
 		return PTR_ERR(tpm_class);
 	}
 
-	rc = alloc_chrdev_region(&tpm_devt, 0, TPM_NUM_DEVICES, "tpm");
+	tpm_rm_class = class_create(THIS_MODULE, "tpmrm");
+	if (IS_ERR(tpm_rm_class)) {
+		pr_err("couldn't create tpmrm class\n");
+		class_destroy(tpm_class);
+		return PTR_ERR(tpm_rm_class);
+	}
+
+	rc = alloc_chrdev_region(&tpm_devt, 0, 2*TPM_NUM_DEVICES, "tpm");
 	if (rc < 0) {
 		pr_err("tpm: failed to allocate char dev region\n");
+		class_destroy(tpm_rm_class);
 		class_destroy(tpm_class);
 		return rc;
 	}
@@ -1208,7 +1216,8 @@ static void __exit tpm_exit(void)
 {
 	idr_destroy(&dev_nums_idr);
 	class_destroy(tpm_class);
-	unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES);
+	class_destroy(tpm_rm_class);
+	unregister_chrdev_region(tpm_devt, 2*TPM_NUM_DEVICES);
 }
 
 subsys_initcall(tpm_init);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c6171e5..890fb6b 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,8 +186,8 @@ struct tpm_buf {
 };
 
 struct tpm_chip {
-	struct device dev;
-	struct cdev cdev;
+	struct device dev, devrm;
+	struct cdev cdev, cdevrm;
 
 	/* A driver callback under ops cannot be run unless ops_sem is held
 	 * (sometimes implicitly, eg for the sysfs code). ops becomes null
@@ -493,8 +493,10 @@ static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 }
 
 extern struct class *tpm_class;
+extern struct class *tpm_rm_class;
 extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
+extern const struct file_operations tpm_rm_fops;
 extern struct idr dev_nums_idr;
 
 enum tpm_transmit_flags {

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > This patch set adds support for TPM spaces that provide a 
> > > > > context for isolating and swapping transient objects. This 
> > > > > patch set does not yet include support for isolating policy and 
> > > > > HMAC sessions but it is trivial to add once the basic approach 
> > > > > is settled (and that's why I created an RFC patch set).
> > > > 
> > > > The approach looks fine to me.  The only basic query I have is 
> > > > about the default: shouldn't it be with resource manager on 
> > > > rather than off?  I can't really think of a use case that wants 
> > > > the RM off (even if you're running your own, having another 
> > > > doesn't hurt anything, and it's still required to share with in
> > > > -kernel uses).
> > > 
> > > This is a valid question and here's a longish explanation.
> > > 
> > > In TPM2_GetCapability and maybe couple of other commands you can 
> > > get handles in the response body. I do not want to have special 
> > > cases in the kernel for response bodies because there is no a 
> > > generic way to do the substitution. What's worse, new commands in 
> > > the standard future revisions could have such commands requiring 
> > > special cases. In addition, vendor specific commans could have 
> > > handles in the response bodies.
> > 
> > OK, in general I buy this ... what you're effectively saying is that 
> > we need a non-RM interface for certain management type commands.
> > 
> > However, let me expand a bit on why I'm fretting about the non-RM use
> > case.  Right at the moment, we have a single TPM device which you use
> > for access to the kernel TPM.  The current tss2 just makes direct use
> > of this, meaning it has to have 0666 permissions.  This means that 
> > any local user can simply DoS the TPM by running us out of transient
> > resources if they don't activate the RM.  If they get a connection
> > always via the RM, this isn't a worry.  Perhaps the best way of 
> > fixing this is to expose two separate device nodes: one raw to the 
> > TPM which we could keep at 0600 and one with an always RM connection 
> > which we can set to 0666.  That would mean that access to the non-RM 
> > connection is either root only or governed by a system set ACL.
> 
> OK, so I put a patch together that does this (see below). It all works
> nicely (with a udev script that sets the resource manager device to
> 0666):

This is not yet a comment about this suggestion but I guess one thing
is clear: the stuff in tpm2-space.c and tpm-interface.c changes are the
thing that we can mostly agree on and the area of argumentation is the
user space interface to it?

Just thinking how to split up the non-RFC patch set. This was also what
Jason suggested if I understood his remark correctly.

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley
> > > > wrote:
> > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > This patch set adds support for TPM spaces that provide a 
> > > > > > context for isolating and swapping transient objects. This 
> > > > > > patch set does not yet include support for isolating policy 
> > > > > > and HMAC sessions but it is trivial to add once the basic
> > > > > > approach is settled (and that's why I created an RFC patch
> > > > > > set).
> > > > > 
> > > > > The approach looks fine to me.  The only basic query I have 
> > > > > is about the default: shouldn't it be with resource manager 
> > > > > on rather than off?  I can't really think of a use case that
> > > > > wants the RM off (even if you're running your own, having 
> > > > > another doesn't hurt anything, and it's still required to 
> > > > > share with in-kernel uses).
> > > > 
> > > > This is a valid question and here's a longish explanation.
> > > > 
> > > > In TPM2_GetCapability and maybe couple of other commands you 
> > > > can get handles in the response body. I do not want to have 
> > > > special cases in the kernel for response bodies because there 
> > > > is no a generic way to do the substitution. What's worse, new 
> > > > commands in the standard future revisions could have such 
> > > > commands requiring special cases. In addition, vendor specific 
> > > > commans could have handles in the response bodies.
> > > 
> > > OK, in general I buy this ... what you're effectively saying is 
> > > that we need a non-RM interface for certain management type
> > > commands.
> > > 
> > > However, let me expand a bit on why I'm fretting about the non-RM 
> > > use case.  Right at the moment, we have a single TPM device which 
> > > you use for access to the kernel TPM.  The current tss2 just 
> > > makes direct use of this, meaning it has to have 0666 
> > > permissions.  This means that any local user can simply DoS the 
> > > TPM by running us out of transient resources if they don't 
> > > activate the RM.  If they get a connection always via the RM, 
> > > this isn't a worry.  Perhaps the best way of fixing this is to 
> > > expose two separate device nodes: one raw to the TPM which we 
> > > could keep at 0600 and one with an always RM connection
> > > which we can set to 0666.  That would mean that access to the non
> > > -RM connection is either root only or governed by a system set
> > > ACL.
> > 
> > OK, so I put a patch together that does this (see below). It all 
> > works nicely (with a udev script that sets the resource manager 
> > device to 0666):
> 
> This is not yet a comment about this suggestion but I guess one thing
> is clear: the stuff in tpm2-space.c and tpm-interface.c changes are 
> the thing that we can mostly agree on and the area of argumentation 
> is the user space interface to it?

Agreed.  As I've already said, the space and interface code is working
well for me in production on my laptop.

> Just thinking how to split up the non-RFC patch set. This was also 
> what Jason suggested if I understood his remark correctly.

SUre ... let's get agreement on how we move forward first.  How the
patch is activated by the user has to be sorted out as well before it
can go in, but it doesn't have to be the first thing we do.  I'm happy
to continue playing with the interfaces to see what works and what
doesn't.  My main current feedback is that I think separate devices
works way better than an ioctl becuase the separate devices approach
allows differing system policies for who accesses the RM backed TPM vs
who accesses the raw one.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 08:14:55AM -0800, James Bottomley wrote:
> On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley
> > > > > wrote:
> > > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > > This patch set adds support for TPM spaces that provide a 
> > > > > > > context for isolating and swapping transient objects. This 
> > > > > > > patch set does not yet include support for isolating policy 
> > > > > > > and HMAC sessions but it is trivial to add once the basic
> > > > > > > approach is settled (and that's why I created an RFC patch
> > > > > > > set).
> > > > > > 
> > > > > > The approach looks fine to me.  The only basic query I have 
> > > > > > is about the default: shouldn't it be with resource manager 
> > > > > > on rather than off?  I can't really think of a use case that
> > > > > > wants the RM off (even if you're running your own, having 
> > > > > > another doesn't hurt anything, and it's still required to 
> > > > > > share with in-kernel uses).
> > > > > 
> > > > > This is a valid question and here's a longish explanation.
> > > > > 
> > > > > In TPM2_GetCapability and maybe couple of other commands you 
> > > > > can get handles in the response body. I do not want to have 
> > > > > special cases in the kernel for response bodies because there 
> > > > > is no a generic way to do the substitution. What's worse, new 
> > > > > commands in the standard future revisions could have such 
> > > > > commands requiring special cases. In addition, vendor specific 
> > > > > commans could have handles in the response bodies.
> > > > 
> > > > OK, in general I buy this ... what you're effectively saying is 
> > > > that we need a non-RM interface for certain management type
> > > > commands.
> > > > 
> > > > However, let me expand a bit on why I'm fretting about the non-RM 
> > > > use case.  Right at the moment, we have a single TPM device which 
> > > > you use for access to the kernel TPM.  The current tss2 just 
> > > > makes direct use of this, meaning it has to have 0666 
> > > > permissions.  This means that any local user can simply DoS the 
> > > > TPM by running us out of transient resources if they don't 
> > > > activate the RM.  If they get a connection always via the RM, 
> > > > this isn't a worry.  Perhaps the best way of fixing this is to 
> > > > expose two separate device nodes: one raw to the TPM which we 
> > > > could keep at 0600 and one with an always RM connection
> > > > which we can set to 0666.  That would mean that access to the non
> > > > -RM connection is either root only or governed by a system set
> > > > ACL.
> > > 
> > > OK, so I put a patch together that does this (see below). It all 
> > > works nicely (with a udev script that sets the resource manager 
> > > device to 0666):
> > 
> > This is not yet a comment about this suggestion but I guess one thing
> > is clear: the stuff in tpm2-space.c and tpm-interface.c changes are 
> > the thing that we can mostly agree on and the area of argumentation 
> > is the user space interface to it?
> 
> Agreed.  As I've already said, the space and interface code is working
> well for me in production on my laptop.
> 
> > Just thinking how to split up the non-RFC patch set. This was also 
> > what Jason suggested if I understood his remark correctly.
> 
> SUre ... let's get agreement on how we move forward first.  How the
> patch is activated by the user has to be sorted out as well before it
> can go in, but it doesn't have to be the first thing we do.  I'm happy
> to continue playing with the interfaces to see what works and what
> doesn't.  My main current feedback is that I think separate devices
> works way better than an ioctl becuase the separate devices approach
> allows differing system policies for who accesses the RM backed TPM vs
> who accesses the raw one.

I think I see your point. I would rather name the device as tpms0 but
otherwise I think we could do it in the way you suggest...

> James

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 08:36:02PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 03, 2017 at 08:14:55AM -0800, James Bottomley wrote:
> > On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley
> > > > > > wrote:
> > > > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > > > This patch set adds support for TPM spaces that provide a 
> > > > > > > > context for isolating and swapping transient objects. This 
> > > > > > > > patch set does not yet include support for isolating policy 
> > > > > > > > and HMAC sessions but it is trivial to add once the basic
> > > > > > > > approach is settled (and that's why I created an RFC patch
> > > > > > > > set).
> > > > > > > 
> > > > > > > The approach looks fine to me.  The only basic query I have 
> > > > > > > is about the default: shouldn't it be with resource manager 
> > > > > > > on rather than off?  I can't really think of a use case that
> > > > > > > wants the RM off (even if you're running your own, having 
> > > > > > > another doesn't hurt anything, and it's still required to 
> > > > > > > share with in-kernel uses).
> > > > > > 
> > > > > > This is a valid question and here's a longish explanation.
> > > > > > 
> > > > > > In TPM2_GetCapability and maybe couple of other commands you 
> > > > > > can get handles in the response body. I do not want to have 
> > > > > > special cases in the kernel for response bodies because there 
> > > > > > is no a generic way to do the substitution. What's worse, new 
> > > > > > commands in the standard future revisions could have such 
> > > > > > commands requiring special cases. In addition, vendor specific 
> > > > > > commans could have handles in the response bodies.
> > > > > 
> > > > > OK, in general I buy this ... what you're effectively saying is 
> > > > > that we need a non-RM interface for certain management type
> > > > > commands.
> > > > > 
> > > > > However, let me expand a bit on why I'm fretting about the non-RM 
> > > > > use case.  Right at the moment, we have a single TPM device which 
> > > > > you use for access to the kernel TPM.  The current tss2 just 
> > > > > makes direct use of this, meaning it has to have 0666 
> > > > > permissions.  This means that any local user can simply DoS the 
> > > > > TPM by running us out of transient resources if they don't 
> > > > > activate the RM.  If they get a connection always via the RM, 
> > > > > this isn't a worry.  Perhaps the best way of fixing this is to 
> > > > > expose two separate device nodes: one raw to the TPM which we 
> > > > > could keep at 0600 and one with an always RM connection
> > > > > which we can set to 0666.  That would mean that access to the non
> > > > > -RM connection is either root only or governed by a system set
> > > > > ACL.
> > > > 
> > > > OK, so I put a patch together that does this (see below). It all 
> > > > works nicely (with a udev script that sets the resource manager 
> > > > device to 0666):
> > > 
> > > This is not yet a comment about this suggestion but I guess one thing
> > > is clear: the stuff in tpm2-space.c and tpm-interface.c changes are 
> > > the thing that we can mostly agree on and the area of argumentation 
> > > is the user space interface to it?
> > 
> > Agreed.  As I've already said, the space and interface code is working
> > well for me in production on my laptop.
> > 
> > > Just thinking how to split up the non-RFC patch set. This was also 
> > > what Jason suggested if I understood his remark correctly.
> > 
> > SUre ... let's get agreement on how we move forward first.  How the
> > patch is activated by the user has to be sorted out as well before it
> > can go in, but it doesn't have to be the first thing we do.  I'm happy
> > to continue playing with the interfaces to see what works and what
> > doesn't.  My main current feedback is that I think separate devices
> > works way better than an ioctl becuase the separate devices approach
> > allows differing system policies for who accesses the RM backed TPM vs
> > who accesses the raw one.
> 
> I think I see your point. I would rather name the device as tpms0 but
> otherwise I think we could do it in the way you suggest...

I think one more stronger argument for tpms0 is that it keeps tpm0
intact. Those who don't care about tpms0 don't have to worry about it
causing regressions. Also it makes it cleaner to put the whole feature
under a compilation flag, which would make to me because that gives
distributions a choice to not enable in-kernel RM when it first hits the
mainline.

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Tue, 2017-01-03 at 21:14 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 03, 2017 at 08:36:02PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Jan 03, 2017 at 08:14:55AM -0800, James Bottomley wrote:
> > > On Tue, 2017-01-03 at 15:41 +0200, Jarkko Sakkinen wrote:
[...]
> > > > Just thinking how to split up the non-RFC patch set. This was 
> > > > also what Jason suggested if I understood his remark correctly.
> > > 
> > > SUre ... let's get agreement on how we move forward first.  How 
> > > the patch is activated by the user has to be sorted out as well
> > > before it can go in, but it doesn't have to be the first thing we 
> > > do.  I'm happy to continue playing with the interfaces to see 
> > > what works and what doesn't.  My main current feedback is that I 
> > > think separate devices works way better than an ioctl becuase the 
> > > separate devices approach allows differing system policies for 
> > > who accesses the RM backed TPM vs who accesses the raw one.
> > 
> > I think I see your point. I would rather name the device as tpms0 
> > but otherwise I think we could do it in the way you suggest...

I'm not at all wedded to the name tpm0rm, so tpms0 is fine by me.

> I think one more stronger argument for tpms0 is that it keeps tpm0
> intact. Those who don't care about tpms0 don't have to worry about it
> causing regressions. Also it makes it cleaner to put the whole 
> feature under a compilation flag, which would make to me because that 
> gives distributions a choice to not enable in-kernel RM when it first 
> hits the mainline.

I wouldn't go that far: one of the evils we cause for distros is too
many compile options.  In this case, I can't think of a good reason to
have an option to disable this now the feature is segregated on to a
separate device.  If we get a regression, only users of the new device
will notice.  If it's a compile option, this is the same if the distro
enables it and if it's disabled, no user can test out the feature (and
distros eventually get complaints about it not working).

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:

> OK, so I put a patch together that does this (see below). It all works
> nicely (with a udev script that sets the resource manager device to
> 0666):
> 
> jejb@jarvis:~> ls -l /dev/tpm*
> crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> 
> I've modified the tss to connect to /dev/tpm0rm by default and it all
> seems to work.
> 
> The patch applies on top of your tabrm branch, by the way.

If we are making a new /dev/ node we should think more carefully about
the design.

- Do we need a cdev node for every chip? What about just '/dev/tpm' and
  we encode the chip number in the message. Since the exclusive
  locking is gone this is very doable.
- Should we get rid of the read/write protocol and use ioctl instead?
  As I understand it ioctl is more usable with seccomp and related
  schemes? I could see passing a TPM FD into a sandbox and wanting the
  sandbox only able to do do decrypt/encrypt operations, for instance.
- Something to identify tpm chips and help match key data with the
  proper chip.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 02:54:45PM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> 
> > OK, so I put a patch together that does this (see below). It all works
> > nicely (with a udev script that sets the resource manager device to
> > 0666):
> > 
> > jejb@jarvis:~> ls -l /dev/tpm*
> > crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> > crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> > 
> > I've modified the tss to connect to /dev/tpm0rm by default and it all
> > seems to work.
> > 
> > The patch applies on top of your tabrm branch, by the way.
> 
> If we are making a new /dev/ node we should think more carefully about
> the design.
> 
> - Do we need a cdev node for every chip? What about just '/dev/tpm' and
>   we encode the chip number in the message. Since the exclusive
>   locking is gone this is very doable.

What about backwards compatiblity? Or would this be just for /dev/tpms?
We can consider this.

> - Should we get rid of the read/write protocol and use ioctl instead?
>   As I understand it ioctl is more usable with seccomp and related
>   schemes? I could see passing a TPM FD into a sandbox and wanting the
>   sandbox only able to do do decrypt/encrypt operations, for instance.

Are you suggesting that command/response transaction would be handled
by ioctl instead of read/write pair. Would make sense looking at how
read/write is managed now (it is more or less of a hack because it
actually is a transction).

> - Something to identify tpm chips and help match key data with the
>   proper chip.

Hey, here's what I propose. I take some of the ideas (not all there
have been so many) and bake a v2 of the RFC. Lets see where we are
at then. I won't add any reviewed/tested-by's before we are in the
same line with "big ideas" nor do I create a non-RFC patch set.

> Jason

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Wed, Jan 04, 2017 at 02:58:10PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 03, 2017 at 02:54:45PM -0700, Jason Gunthorpe wrote:
> > On Mon, Jan 02, 2017 at 09:26:58PM -0800, James Bottomley wrote:
> > 
> > > OK, so I put a patch together that does this (see below). It all works
> > > nicely (with a udev script that sets the resource manager device to
> > > 0666):
> > > 
> > > jejb@jarvis:~> ls -l /dev/tpm*
> > > crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> > > crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> > > 
> > > I've modified the tss to connect to /dev/tpm0rm by default and it all
> > > seems to work.
> > > 
> > > The patch applies on top of your tabrm branch, by the way.
> > 
> > If we are making a new /dev/ node we should think more carefully about
> > the design.
> > 
> > - Do we need a cdev node for every chip? What about just '/dev/tpm' and
> >   we encode the chip number in the message. Since the exclusive
> >   locking is gone this is very doable.
> 
> What about backwards compatiblity? Or would this be just for /dev/tpms?
> We can consider this.

Yes, just for the new char dev.

> > - Should we get rid of the read/write protocol and use ioctl instead?
> >   As I understand it ioctl is more usable with seccomp and related
> >   schemes? I could see passing a TPM FD into a sandbox and wanting the
> >   sandbox only able to do do decrypt/encrypt operations, for instance.
> 
> Are you suggesting that command/response transaction would be handled
> by ioctl instead of read/write pair. Would make sense looking at how
> read/write is managed now (it is more or less of a hack because it
> actually is a transction).

Yes.

> > - Something to identify tpm chips and help match key data with the
> >   proper chip.
> 
> Hey, here's what I propose. I take some of the ideas (not all there
> have been so many) and bake a v2 of the RFC. Lets see where we are
> at then. I won't add any reviewed/tested-by's before we are in the
> same line with "big ideas" nor do I create a non-RFC patch set.

Usually with something like this there will be lots of dicussion
around the uapi portion - that is the portion we have to reatin
backwards compatability with forever - so there is a natural need to
make sure it is sane.

This is why I'm so cautious to limit what is possible because it is
easier to add new stuff then take stuff away.

So design your patch set to keep the uapi stuff as distinct and
well-described as possible - the other parts are much easier to review
and agree on.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Andy Lutomirski]

On 01/02/2017 09:26 PM, James Bottomley wrote:
> On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
>> On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
>>> On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
>>>> On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
>>>>> This patch set adds support for TPM spaces that provide a
>>>>> context for isolating and swapping transient objects. This
>>>>> patch set does not yet include support for isolating policy and
>>>>> HMAC sessions but it is trivial to add once the basic approach
>>>>> is settled (and that's why I created an RFC patch set).
>>>>
>>>> The approach looks fine to me.  The only basic query I have is
>>>> about the default: shouldn't it be with resource manager on
>>>> rather than off?  I can't really think of a use case that wants
>>>> the RM off (even if you're running your own, having another
>>>> doesn't hurt anything, and it's still required to share with in
>>>> -kernel uses).
>>>
>>> This is a valid question and here's a longish explanation.
>>>
>>> In TPM2_GetCapability and maybe couple of other commands you can
>>> get handles in the response body. I do not want to have special
>>> cases in the kernel for response bodies because there is no a
>>> generic way to do the substitution. What's worse, new commands in
>>> the standard future revisions could have such commands requiring
>>> special cases. In addition, vendor specific commans could have
>>> handles in the response bodies.
>>
>> OK, in general I buy this ... what you're effectively saying is that
>> we need a non-RM interface for certain management type commands.
>>
>> However, let me expand a bit on why I'm fretting about the non-RM use
>> case.  Right at the moment, we have a single TPM device which you use
>> for access to the kernel TPM.  The current tss2 just makes direct use
>> of this, meaning it has to have 0666 permissions.  This means that
>> any local user can simply DoS the TPM by running us out of transient
>> resources if they don't activate the RM.  If they get a connection
>> always via the RM, this isn't a worry.  Perhaps the best way of
>> fixing this is to expose two separate device nodes: one raw to the
>> TPM which we could keep at 0600 and one with an always RM connection
>> which we can set to 0666.  That would mean that access to the non-RM
>> connection is either root only or governed by a system set ACL.
>
> OK, so I put a patch together that does this (see below). It all works
> nicely (with a udev script that sets the resource manager device to
> 0666):
>
> jejb@jarvis:~> ls -l /dev/tpm*
> crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
>
> I've modified the tss to connect to /dev/tpm0rm by default and it all
> seems to work.
>
> The patch applies on top of your tabrm branch, by the way.

Conceptually I like this a *lot* better.  I believe that this 
effectively solves my major gripe with the TPM 1.2 ecosystem.

However, can this be taken just a little farther?  IMO the tpm0rm (or 
tpms0 or whatever) node should also restrict commands that can be sent 
(perhaps by in-kernel whitelist?) to those that shouldn't be restricted 
to the owner (by which I probably mean the Owner, the Platform, etc)? 
For example, someone with tpm0rm open should not be able to change key 
hierarchy passwords, write to NV memory, clear hierarchies, etc.

Hmm.  Maybe there should be a way to allocate NV slots to users. 
/dev/tpm/nv0?  I don't really like that idea, though.

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 09:47:21PM -0800, Andy Lutomirski wrote:
> On 01/02/2017 09:26 PM, James Bottomley wrote:
> > On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > This patch set adds support for TPM spaces that provide a
> > > > > > context for isolating and swapping transient objects. This
> > > > > > patch set does not yet include support for isolating policy and
> > > > > > HMAC sessions but it is trivial to add once the basic approach
> > > > > > is settled (and that's why I created an RFC patch set).
> > > > > 
> > > > > The approach looks fine to me.  The only basic query I have is
> > > > > about the default: shouldn't it be with resource manager on
> > > > > rather than off?  I can't really think of a use case that wants
> > > > > the RM off (even if you're running your own, having another
> > > > > doesn't hurt anything, and it's still required to share with in
> > > > > -kernel uses).
> > > > 
> > > > This is a valid question and here's a longish explanation.
> > > > 
> > > > In TPM2_GetCapability and maybe couple of other commands you can
> > > > get handles in the response body. I do not want to have special
> > > > cases in the kernel for response bodies because there is no a
> > > > generic way to do the substitution. What's worse, new commands in
> > > > the standard future revisions could have such commands requiring
> > > > special cases. In addition, vendor specific commans could have
> > > > handles in the response bodies.
> > > 
> > > OK, in general I buy this ... what you're effectively saying is that
> > > we need a non-RM interface for certain management type commands.
> > > 
> > > However, let me expand a bit on why I'm fretting about the non-RM use
> > > case.  Right at the moment, we have a single TPM device which you use
> > > for access to the kernel TPM.  The current tss2 just makes direct use
> > > of this, meaning it has to have 0666 permissions.  This means that
> > > any local user can simply DoS the TPM by running us out of transient
> > > resources if they don't activate the RM.  If they get a connection
> > > always via the RM, this isn't a worry.  Perhaps the best way of
> > > fixing this is to expose two separate device nodes: one raw to the
> > > TPM which we could keep at 0600 and one with an always RM connection
> > > which we can set to 0666.  That would mean that access to the non-RM
> > > connection is either root only or governed by a system set ACL.
> > 
> > OK, so I put a patch together that does this (see below). It all works
> > nicely (with a udev script that sets the resource manager device to
> > 0666):
> > 
> > jejb@jarvis:~> ls -l /dev/tpm*
> > crw------- 1 root root  10,   224 Jan  2 20:54 /dev/tpm0
> > crw-rw-rw- 1 root root 246, 65536 Jan  2 20:54 /dev/tpm0rm
> > 
> > I've modified the tss to connect to /dev/tpm0rm by default and it all
> > seems to work.
> > 
> > The patch applies on top of your tabrm branch, by the way.
> 
> Conceptually I like this a *lot* better.  I believe that this effectively
> solves my major gripe with the TPM 1.2 ecosystem.
> 
> However, can this be taken just a little farther?  IMO the tpm0rm (or tpms0
> or whatever) node should also restrict commands that can be sent (perhaps by
> in-kernel whitelist?) to those that shouldn't be restricted to the owner (by
> which I probably mean the Owner, the Platform, etc)? For example, someone
> with tpm0rm open should not be able to change key hierarchy passwords, write
> to NV memory, clear hierarchies, etc.

Yes. This was already discussed in Linux Plumbers. It is trivial to have
that. I just left it out from this RFC patch set to get something not
too complicated out quickly. Whitelist is coming to the non-RFC version.

> Hmm.  Maybe there should be a way to allocate NV slots to users.
> /dev/tpm/nv0?  I don't really like that idea, though.

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Mon, Jan 02, 2017 at 01:40:48PM -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > This patch set adds support for TPM spaces that provide a context
> > > > for isolating and swapping transient objects. This patch set does
> > > > not yet include support for isolating policy and HMAC sessions 
> > > > but it is trivial to add once the basic approach is settled (and
> > > > that's why I created an RFC patch set).
> > > 
> > > The approach looks fine to me.  The only basic query I have is 
> > > about the default: shouldn't it be with resource manager on rather 
> > > than off?  I can't really think of a use case that wants the RM off 
> > > (even if you're running your own, having another doesn't hurt 
> > > anything, and it's still required to share with in-kernel uses).
> > 
> > This is a valid question and here's a longish explanation.
> > 
> > In TPM2_GetCapability and maybe couple of other commands you can get
> > handles in the response body. I do not want to have special cases in 
> > the kernel for response bodies because there is no a generic way to 
> > do the substitution. What's worse, new commands in the standard 
> > future revisions could have such commands requiring special cases. In
> > addition, vendor specific commans could have handles in the response
> > bodies.
> 
> OK, in general I buy this ... what you're effectively saying is that we
> need a non-RM interface for certain management type commands.

Not only that.

Doing virtualization for commands like GetCapability is just a better
fit for doing in the user space. You could have a thin translation layer
in your TSS library for example to handle these specific messages.

> However, let me expand a bit on why I'm fretting about the non-RM use
> case.  Right at the moment, we have a single TPM device which you use
> for access to the kernel TPM.  The current tss2 just makes direct use
> of this, meaning it has to have 0666 permissions.  This means that any
> local user can simply DoS the TPM by running us out of transient
> resources if they don't activate the RM.  If they get a connection
> always via the RM, this isn't a worry.  Perhaps the best way of fixing
> this is to expose two separate device nodes: one raw to the TPM which
> we could keep at 0600 and one with an always RM connection which we can
> set to 0666.  That would mean that access to the non-RM connection is
> either root only or governed by a system set ACL.

I'm not sure about this. Why you couldn't have a very thin daemon that
prepares the file descriptor and sends it through UDS socket to a
client.  The non-RFC version will also have whitelisting ioctl for
further restricting the file descriptor to only specific TPM commands.

This is also architecture I preseted in my LSS presentation and I think
it makes sense especially when I add the whitelisting to the pack.

> James

I'm more dilated to keep things way they are now. I'll stick to that at
least with the first non-RFC version and hopefully get the tpm2-space.c
part reviewed as I split that stuff to a separate commit.

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Tue, 2017-01-03 at 15:51 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 02, 2017 at 01:40:48PM -0800, James Bottomley wrote:
> > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > This patch set adds support for TPM spaces that provide a 
> > > > > context for isolating and swapping transient objects. This 
> > > > > patch set does not yet include support for isolating policy 
> > > > > and HMAC sessions but it is trivial to add once the basic 
> > > > > approach is settled (and that's why I created an RFC patch
> > > > > set).
> > > > 
> > > > The approach looks fine to me.  The only basic query I have is 
> > > > about the default: shouldn't it be with resource manager on 
> > > > rather than off?  I can't really think of a use case that wants 
> > > > the RM off (even if you're running your own, having another 
> > > > doesn't hurt anything, and it's still required to share with in
> > > > -kernel uses).
> > > 
> > > This is a valid question and here's a longish explanation.
> > > 
> > > In TPM2_GetCapability and maybe couple of other commands you can 
> > > get handles in the response body. I do not want to have special 
> > > cases in the kernel for response bodies because there is no a 
> > > generic way to do the substitution. What's worse, new commands in 
> > > the standard future revisions could have such commands requiring 
> > > special cases. In addition, vendor specific commans could have 
> > > handles in the response bodies.
> > 
> > OK, in general I buy this ... what you're effectively saying is 
> > that we need a non-RM interface for certain management type
> > commands.
> 
> Not only that.
> 
> Doing virtualization for commands like GetCapability is just a better
> fit for doing in the user space. You could have a thin translation 
> layer in your TSS library for example to handle these specific
> messages.

Yes, we could do it that way too.  To be honest I can't see much use
for getting the transient handles and all the other handles you'd be
interested in aren't virtualized.

> > However, let me expand a bit on why I'm fretting about the non-RM 
> > use case.  Right at the moment, we have a single TPM device which 
> > you use for access to the kernel TPM.  The current tss2 just makes 
> > direct use of this, meaning it has to have 0666 permissions.  This 
> > means that any local user can simply DoS the TPM by running us out 
> > of transient resources if they don't activate the RM.  If they get 
> > a connection always via the RM, this isn't a worry.  Perhaps the 
> > best way of fixing this is to expose two separate device nodes: one 
> > raw to the TPM which we could keep at 0600 and one with an always 
> > RM connection which we can set to 0666.  That would mean that 
> > access to the non-RM connection is either root only or governed by
> > a system set ACL.
> 
> I'm not sure about this. Why you couldn't have a very thin daemon 
> that prepares the file descriptor and sends it through UDS socket to 
> a client.

So I'm a bit soured on daemons from the trousers experience: tcsd
crashed regularly and when it did it took all the TPM connections down
irrecoverably.  I'm not saying we can't write a stateless daemon to fix
most of the trousers issues, but I think it's valuable first to ask the
question, "can we manage without a daemon at all?"  I actually think
the answer is "yes", so I'm interested in seeing how far that line of
research gets us.

>   The non-RFC version will also have whitelisting ioctl for
> further restricting the file descriptor to only specific TPM
> commands.
> 
> This is also architecture I preseted in my LSS presentation and I 
> think it makes sense especially when I add the whitelisting to the
> pack.

Do you have a link to the presentation?  The Plumbers etherpad doesn't
contain it.  I've been trying to work out whether a properly set up TPM
actually does need any protections at all.  As far as I can tell, once
you've set all the hierarchy authorities and the lockout one, you're
pretty well protected.

> > James
> 
> I'm more dilated to keep things way they are now. I'll stick to that 
> at least with the first non-RFC version and hopefully get the tpm2
> -space.c part reviewed as I split that stuff to a separate commit.

Sure, we need the patch in an acceptable form first.  I'll keep
worrying about the systems implications, but I can layer playing with
those on top of what you do.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 08:36:10AM -0800, James Bottomley wrote:
> On Tue, 2017-01-03 at 15:51 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 01:40:48PM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > > > This patch set adds support for TPM spaces that provide a 
> > > > > > context for isolating and swapping transient objects. This 
> > > > > > patch set does not yet include support for isolating policy 
> > > > > > and HMAC sessions but it is trivial to add once the basic 
> > > > > > approach is settled (and that's why I created an RFC patch
> > > > > > set).
> > > > > 
> > > > > The approach looks fine to me.  The only basic query I have is 
> > > > > about the default: shouldn't it be with resource manager on 
> > > > > rather than off?  I can't really think of a use case that wants 
> > > > > the RM off (even if you're running your own, having another 
> > > > > doesn't hurt anything, and it's still required to share with in
> > > > > -kernel uses).
> > > > 
> > > > This is a valid question and here's a longish explanation.
> > > > 
> > > > In TPM2_GetCapability and maybe couple of other commands you can 
> > > > get handles in the response body. I do not want to have special 
> > > > cases in the kernel for response bodies because there is no a 
> > > > generic way to do the substitution. What's worse, new commands in 
> > > > the standard future revisions could have such commands requiring 
> > > > special cases. In addition, vendor specific commans could have 
> > > > handles in the response bodies.
> > > 
> > > OK, in general I buy this ... what you're effectively saying is 
> > > that we need a non-RM interface for certain management type
> > > commands.
> > 
> > Not only that.
> > 
> > Doing virtualization for commands like GetCapability is just a better
> > fit for doing in the user space. You could have a thin translation 
> > layer in your TSS library for example to handle these specific
> > messages.
> 
> Yes, we could do it that way too.  To be honest I can't see much use
> for getting the transient handles and all the other handles you'd be
> interested in aren't virtualized.
> 
> > > However, let me expand a bit on why I'm fretting about the non-RM 
> > > use case.  Right at the moment, we have a single TPM device which 
> > > you use for access to the kernel TPM.  The current tss2 just makes 
> > > direct use of this, meaning it has to have 0666 permissions.  This 
> > > means that any local user can simply DoS the TPM by running us out 
> > > of transient resources if they don't activate the RM.  If they get 
> > > a connection always via the RM, this isn't a worry.  Perhaps the 
> > > best way of fixing this is to expose two separate device nodes: one 
> > > raw to the TPM which we could keep at 0600 and one with an always 
> > > RM connection which we can set to 0666.  That would mean that 
> > > access to the non-RM connection is either root only or governed by
> > > a system set ACL.
> > 
> > I'm not sure about this. Why you couldn't have a very thin daemon 
> > that prepares the file descriptor and sends it through UDS socket to 
> > a client.
> 
> So I'm a bit soured on daemons from the trousers experience: tcsd
> crashed regularly and when it did it took all the TPM connections down
> irrecoverably.  I'm not saying we can't write a stateless daemon to fix
> most of the trousers issues, but I think it's valuable first to ask the
> question, "can we manage without a daemon at all?"  I actually think
> the answer is "yes", so I'm interested in seeing how far that line of
> research gets us.

This was not a good argument in the first place because you could also
use daemon with tpms0. We can ignore this.

> >   The non-RFC version will also have whitelisting ioctl for
> > further restricting the file descriptor to only specific TPM
> > commands.
> > 
> > This is also architecture I preseted in my LSS presentation and I 
> > think it makes sense especially when I add the whitelisting to the
> > pack.
> 
> Do you have a link to the presentation?  The Plumbers etherpad doesn't
> contain it.  I've been trying to work out whether a properly set up TPM
> actually does need any protections at all.  As far as I can tell, once
> you've set all the hierarchy authorities and the lockout one, you're
> pretty well protected.

http://events.linuxfoundation.org/sites/events/files/slides/201608-LinuxSecuritySummit-TPM.pdf

> > > James
> > 
> > I'm more dilated to keep things way they are now. I'll stick to that 
> > at least with the first non-RFC version and hopefully get the tpm2
> > -space.c part reviewed as I split that stuff to a separate commit.
> 
> Sure, we need the patch in an acceptable form first.  I'll keep
> worrying about the systems implications, but I can layer playing with
> those on top of what you do.
> 
> James

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Tue, Jan 03, 2017 at 08:36:10AM -0800, James Bottomley wrote:

> > I'm not sure about this. Why you couldn't have a very thin daemon 
> > that prepares the file descriptor and sends it through UDS socket to 
> > a client.
> 
> So I'm a bit soured on daemons from the trousers experience: tcsd
> crashed regularly and when it did it took all the TPM connections down
> irrecoverably.  I'm not saying we can't write a stateless daemon to fix
> most of the trousers issues, but I think it's valuable first to ask the
> question, "can we manage without a daemon at all?"  I actually think
> the answer is "yes", so I'm interested in seeing how far that line of
> research gets us.

There is clearly no need for a daemon to be involved when working on
simple tasks like key load and key sign/enc/dec actions, adding such a
thing only increases the complexity.

If we discover a reason to have a daemon down the road then it should
work in some way where the user space can call out to the daemon over
a different path than the kernel. (eg dbus or something)

> Do you have a link to the presentation?  The Plumbers etherpad doesn't
> contain it.  I've been trying to work out whether a properly set up TPM
> actually does need any protections at all.  As far as I can tell, once
> you've set all the hierarchy authorities and the lockout one, you're
> pretty well protected.

I think we should also consider TPM 1.2 support in all of this, it is
still a very popular peice of hardware and it is equally able to
support a RM.

So, in general, I'd prefer to see the unprivileged char dev hard
prevented by the kernel from doing certain things:

- Wipe the TPM
- Manipulate the SRK, nvram, tpm flags, change passwords etc
- Read back the EK
- Write to PCRs
- etc.

Even if TPM 2 has a stronger password based model, I still think the
kernel should hard prevent those sorts of actions even if the user
knows the TPM password.

Realistically people in less senstive environments will want to use
the well known TPM passwords and still have reasonable safety in their
unprivileged accounts.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Ken Goldman]

On 1/3/2017 4:47 PM, Jason Gunthorpe wrote:
>
> I think we should also consider TPM 1.2 support in all of this, it is
> still a very popular piece of hardware and it is equally able to
> support a RM.

I suspect that TPM 2.0 and TPM 1.2 are so different that there may be 
little or no code in common.

My immediate need is for a 2.0 resource manager, since it's a gap in the 
technology, while 1.2 does have tcsd.

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Tue, Jan 03, 2017 at 05:21:28PM -0500, Ken Goldman wrote:
> On 1/3/2017 4:47 PM, Jason Gunthorpe wrote:
> >
> > I think we should also consider TPM 1.2 support in all of this, it is
> > still a very popular piece of hardware and it is equally able to
> > support a RM.
> 
> I suspect that TPM 2.0 and TPM 1.2 are so different that there may be 
> little or no code in common.

Sure, but the uapi should make sense for both versions, ie, I don't want
to see a tpm 2.0 specific char dev.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Tue, 2017-01-03 at 14:47 -0700, Jason Gunthorpe wrote:
> On Tue, Jan 03, 2017 at 08:36:10AM -0800, James Bottomley wrote:
> 
> > > I'm not sure about this. Why you couldn't have a very thin daemon
> > > that prepares the file descriptor and sends it through UDS socket 
> > > to a client.
> > 
> > So I'm a bit soured on daemons from the trousers experience: tcsd
> > crashed regularly and when it did it took all the TPM connections 
> > down irrecoverably.  I'm not saying we can't write a stateless 
> > daemon to fix most of the trousers issues, but I think it's 
> > valuable first to ask the question, "can we manage without a daemon 
> > at all?"  I actually think the answer is "yes", so I'm interested 
> > in seeing how far that line of research gets us.
> 
> There is clearly no need for a daemon to be involved when working on
> simple tasks like key load and key sign/enc/dec actions, adding such 
> a thing only increases the complexity.
> 
> If we discover a reason to have a daemon down the road then it should
> work in some way where the user space can call out to the daemon over
> a different path than the kernel. (eg dbus or something)

Agreed ... I think the only reason I can currently see for needing a
daemon is if we need it to sort out access security (which I'm hoping
we don't).

> > Do you have a link to the presentation?  The Plumbers etherpad 
> > doesn't contain it.  I've been trying to work out whether a 
> > properly set up TPM actually does need any protections at all.  As 
> > far as I can tell, once you've set all the hierarchy authorities 
> > and the lockout one, you're pretty well protected.
> 
> I think we should also consider TPM 1.2 support in all of this, it is
> still a very popular peice of hardware and it is equally able to
> support a RM.

I've been running with the openssl and gnome-keyring patches in 1.2 for
months now.  The thing about 1.2 is that the volatile store is much
larger, so there's a lot less of a need for a RM.  It's only a
requirement in 2.0 because most shipping TPMs only seem to have room
for about 3 objects.

> So, in general, I'd prefer to see the unprivileged char dev hard
> prevented by the kernel from doing certain things:
> 
> - Wipe the TPM
> - Manipulate the SRK, nvram, tpm flags, change passwords etc
> - Read back the EK

These are all things that the TPM itself is capable of enforcing a
policy for.  I think we should aim for correct setup of the TPM in the
first place so it enforces the policy in a standard manner rather than
having an artificial policy enforcement in the kernel.

> - Write to PCRs

The design of a TPM is mostly that it's up to user space to deal with
this.  Userspace can, of course, kill the TPM ability to quote and seal
to PCRs by inappropriately extending them.  However, there are a lot of
responsible applications that want to use PCRs in userspace; for
instance cloud boot and attestation.  We don't really want to restrict
their ability arbitrarily.

> - etc.

> 
> Even if TPM 2 has a stronger password based model, I still think the
> kernel should hard prevent those sorts of actions even if the user
> knows the TPM password.

That would make us different from TPM1.2: there, if you know the owner
authorisation, trousers will pretty much let you do anything.

> Realistically people in less senstive environments will want to use
> the well known TPM passwords and still have reasonable safety in 
> their unprivileged accounts.

Can we not do most of this with localities?  In theory locality 0 is
supposed to be only the bios and the boot manager and the OS gets to
access 1-3.  We could reserve one for the internal kernel and still
have a couple for userspace (I'll have to go back and check numbers; I
seem to remember there were odd restrictions on which PCR you can reset
and extend in which locality).  If we have two devices (one for each
locality) we could define a UNIX ACL on the devices to achieve what you
want.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Tue, Jan 03, 2017 at 02:39:58PM -0800, James Bottomley wrote:

> > I think we should also consider TPM 1.2 support in all of this, it is
> > still a very popular peice of hardware and it is equally able to
> > support a RM.
> 
> I've been running with the openssl and gnome-keyring patches in 1.2 for
> months now.  The thing about 1.2 is that the volatile store is much
> larger, so there's a lot less of a need for a RM.  It's only a
> requirement in 2.0 because most shipping TPMs only seem to have room
> for about 3 objects.

It would be great if the 1.2 RM could support just enough to allow RSA
key operations from userspace, without key virtualization. That would
allow the plugins that already exist to move to the RM interface and
we can get rid of the hard dependency on trousers.

I honestly don't think this should be much work beyond what Jarkko has
already done...

> > So, in general, I'd prefer to see the unprivileged char dev hard
> > prevented by the kernel from doing certain things:
> > 
> > - Wipe the TPM
> > - Manipulate the SRK, nvram, tpm flags, change passwords etc
> > - Read back the EK
> 
> These are all things that the TPM itself is capable of enforcing a
> policy for.  I think we should aim for correct setup of the TPM in the
> first place so it enforces the policy in a standard manner rather than
> having an artificial policy enforcement in the kernel.

Well, by policy you mean 'know the owner password' which at least I am
*very* nervous about exposing beyond the super user - certainly in my
embedded systems.

On a desktop I think these actions should be protected by the usual
'sudo' scheme dbus has *in addition* to the owner password.

It is rare that anyone would want to do these actions this seems like
the right choice from a security perspective.

> > - Write to PCRs
> 
> The design of a TPM is mostly that it's up to user space to deal with
> this.  Userspace can, of course, kill the TPM ability to quote and seal
> to PCRs by inappropriately extending them.  However, there are a lot of
> responsible applications that want to use PCRs in userspace; for
> instance cloud boot and attestation.  We don't really want to restrict
> their ability arbitrarily.

The entire RM model is that of a sandbox, so if extending the PCR is
viewable by other RM clients it must be prevented. We don't want a
user to be able to DOS other users by extending a PCR and breaking
system attestation or unsealing.

Like you say below localities may be part of the answer here, and I
also recall that various PCRs become read-only at certain localities.

However, until we figure out a security model for writing PCRs I think
the RM has to ban them.

> > Even if TPM 2 has a stronger password based model, I still think the
> > kernel should hard prevent those sorts of actions even if the user
> > knows the TPM password.
> 
> That would make us different from TPM1.2: there, if you know the owner
> authorisation, trousers will pretty much let you do anything.

Well, I also think trousers is wrong to do that. :)

But this is not trousers, this is an in-kernel 0666 char dev that will
be active on basically every Linux system with a TPM. I think we have
a duty to be very conservative here.

This is why I want to see a command white list in Jarkko's patches to
start. Every command exposed needs a very careful security analysis
first, and we should start with only the commands we know are safe :\

> > Realistically people in less senstive environments will want to use
> > the well known TPM passwords and still have reasonable safety in 
> > their unprivileged accounts.
> 
> Can we not do most of this with localities?  In theory locality 0 is
> supposed to be only the bios and the boot manager and the OS gets to
> access 1-3.  We could reserve one for the internal kernel and still
> have a couple for userspace (I'll have to go back and check numbers; I
> seem to remember there were odd restrictions on which PCR you can reset
> and extend in which locality).  If we have two devices (one for each
> locality) we could define a UNIX ACL on the devices to achieve what you
> want.

Good point, yes, localities should be thought about when designing
this new RM char dev uAPI...

Our support for localities in the kernel today uses some really gross
sysfs file and is basically insane, IMHO.

Maybe there should be a /dev/tpmrm for each locality? If so then only
the safe one with unwritable localities can be 0666 by default..

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Tue, 2017-01-03 at 17:17 -0700, Jason Gunthorpe wrote:
> On Tue, Jan 03, 2017 at 02:39:58PM -0800, James Bottomley wrote:
> 
> > > I think we should also consider TPM 1.2 support in all of this, 
> > > it is still a very popular peice of hardware and it is equally 
> > > able to support a RM.
> > 
> > I've been running with the openssl and gnome-keyring patches in 1.2 
> > for months now.  The thing about 1.2 is that the volatile store is 
> > much larger, so there's a lot less of a need for a RM.  It's only a
> > requirement in 2.0 because most shipping TPMs only seem to have 
> > room for about 3 objects.
> 
> It would be great if the 1.2 RM could support just enough to allow 
> RSA key operations from userspace, without key virtualization. That 
> would allow the plugins that already exist to move to the RM 
> interface and we can get rid of the hard dependency on trousers.
[getting long, let's divide into separate issues]

They actually already do: Trousers, for all its annoying complexity,
doesn't actually implement a resource manager, so we should be able to
do all the RSA operations we want today with the current 1.2 interface
and no RM.  The difficulty is no API ... unless you want to speak at
the TPM command level and do all the HMAC calculations yourself.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Tue, Jan 03, 2017 at 04:29:59PM -0800, James Bottomley wrote:
> On Tue, 2017-01-03 at 17:17 -0700, Jason Gunthorpe wrote:
> > On Tue, Jan 03, 2017 at 02:39:58PM -0800, James Bottomley wrote:
> > 
> > > > I think we should also consider TPM 1.2 support in all of this, 
> > > > it is still a very popular peice of hardware and it is equally 
> > > > able to support a RM.
> > > 
> > > I've been running with the openssl and gnome-keyring patches in 1.2 
> > > for months now.  The thing about 1.2 is that the volatile store is 
> > > much larger, so there's a lot less of a need for a RM.  It's only a
> > > requirement in 2.0 because most shipping TPMs only seem to have 
> > > room for about 3 objects.
> > 
> > It would be great if the 1.2 RM could support just enough to allow 
> > RSA key operations from userspace, without key virtualization. That 
> > would allow the plugins that already exist to move to the RM 
> > interface and we can get rid of the hard dependency on trousers.
> [getting long, let's divide into separate issues]
> 
> They actually already do: Trousers, for all its annoying complexity,
> doesn't actually implement a resource manager, so we should be able to
> do all the RSA operations we want today with the current 1.2 interface
> and no RM.

The current interface cannot be used by unprivileged users. I want to
see the kernel provide an unprivileged safe interface for both TPM 1.2
and TPM 2.0

> The difficulty is no API ... unless you want to speak at
> the TPM command level and do all the HMAC calculations yourself.

I think the openssl RSA method could certainly do the TPM command
level with not really a big problem.

That would avoid all these crazy dependencies and debate :|

I have a very good idea what that would look like for tpm 1.2 and I
would estimate < 500 lines....

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 05:17:32PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 03, 2017 at 02:39:58PM -0800, James Bottomley wrote:
> 
> > > I think we should also consider TPM 1.2 support in all of this, it is
> > > still a very popular peice of hardware and it is equally able to
> > > support a RM.
> > 
> > I've been running with the openssl and gnome-keyring patches in 1.2 for
> > months now.  The thing about 1.2 is that the volatile store is much
> > larger, so there's a lot less of a need for a RM.  It's only a
> > requirement in 2.0 because most shipping TPMs only seem to have room
> > for about 3 objects.
> 
> It would be great if the 1.2 RM could support just enough to allow RSA
> key operations from userspace, without key virtualization. That would
> allow the plugins that already exist to move to the RM interface and
> we can get rid of the hard dependency on trousers.
> 
> I honestly don't think this should be much work beyond what Jarkko has
> already done...
> 
> > > So, in general, I'd prefer to see the unprivileged char dev hard
> > > prevented by the kernel from doing certain things:
> > > 
> > > - Wipe the TPM
> > > - Manipulate the SRK, nvram, tpm flags, change passwords etc
> > > - Read back the EK
> > 
> > These are all things that the TPM itself is capable of enforcing a
> > policy for.  I think we should aim for correct setup of the TPM in the
> > first place so it enforces the policy in a standard manner rather than
> > having an artificial policy enforcement in the kernel.
> 
> Well, by policy you mean 'know the owner password' which at least I am
> *very* nervous about exposing beyond the super user - certainly in my
> embedded systems.
> 
> On a desktop I think these actions should be protected by the usual
> 'sudo' scheme dbus has *in addition* to the owner password.
> 
> It is rare that anyone would want to do these actions this seems like
> the right choice from a security perspective.
> 
> > > - Write to PCRs
> > 
> > The design of a TPM is mostly that it's up to user space to deal with
> > this.  Userspace can, of course, kill the TPM ability to quote and seal
> > to PCRs by inappropriately extending them.  However, there are a lot of
> > responsible applications that want to use PCRs in userspace; for
> > instance cloud boot and attestation.  We don't really want to restrict
> > their ability arbitrarily.
> 
> The entire RM model is that of a sandbox, so if extending the PCR is
> viewable by other RM clients it must be prevented. We don't want a
> user to be able to DOS other users by extending a PCR and breaking
> system attestation or unsealing.
> 
> Like you say below localities may be part of the answer here, and I
> also recall that various PCRs become read-only at certain localities.
> 
> However, until we figure out a security model for writing PCRs I think
> the RM has to ban them.
> 
> > > Even if TPM 2 has a stronger password based model, I still think the
> > > kernel should hard prevent those sorts of actions even if the user
> > > knows the TPM password.
> > 
> > That would make us different from TPM1.2: there, if you know the owner
> > authorisation, trousers will pretty much let you do anything.
> 
> Well, I also think trousers is wrong to do that. :)
> 
> But this is not trousers, this is an in-kernel 0666 char dev that will
> be active on basically every Linux system with a TPM. I think we have
> a duty to be very conservative here.
> 
> This is why I want to see a command white list in Jarkko's patches to
> start. Every command exposed needs a very careful security analysis
> first, and we should start with only the commands we know are safe :\
> 
> > > Realistically people in less senstive environments will want to use
> > > the well known TPM passwords and still have reasonable safety in 
> > > their unprivileged accounts.
> > 
> > Can we not do most of this with localities?  In theory locality 0 is
> > supposed to be only the bios and the boot manager and the OS gets to
> > access 1-3.  We could reserve one for the internal kernel and still
> > have a couple for userspace (I'll have to go back and check numbers; I
> > seem to remember there were odd restrictions on which PCR you can reset
> > and extend in which locality).  If we have two devices (one for each
> > locality) we could define a UNIX ACL on the devices to achieve what you
> > want.
> 
> Good point, yes, localities should be thought about when designing
> this new RM char dev uAPI...
> 
> Our support for localities in the kernel today uses some really gross
> sysfs file and is basically insane, IMHO.
> 
> Maybe there should be a /dev/tpmrm for each locality? If so then only
> the safe one with unwritable localities can be 0666 by default..

Do you see that it would be possible to have ioctl for setting the
locality, or is it out of the question? I'm planning to have an ioctl
for the whitelist anyway.

> Jason

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Wed, 2017-01-04 at 14:50 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 03, 2017 at 05:17:32PM -0700, Jason Gunthorpe wrote:
> > On Tue, Jan 03, 2017 at 02:39:58PM -0800, James Bottomley wrote:
[...]
> > > > Even if TPM 2 has a stronger password based model, I still
> > > > think the kernel should hard prevent those sorts of actions
> > > > even if the user knows the TPM password.
> > > 
> > > That would make us different from TPM1.2: there, if you know the 
> > > owner authorisation, trousers will pretty much let you do
> > > anything.
> > 
> > Well, I also think trousers is wrong to do that. :)
> > 
> > But this is not trousers, this is an in-kernel 0666 char dev that 
> > will be active on basically every Linux system with a TPM. I think 
> > we have a duty to be very conservative here.

Just to note on this that trousers *is* effectively an 0666 kernel
device: all tcsd does is run with root privileges on the real /dev/tpm0
and mediate the calls.  It doesn't seem to police them at all.

I realise you want better than this, and I definitely think this is a
worthy goal, but the point I want to make is that an 0666 device and
trousers are basically equivalent.

> > This is why I want to see a command white list in Jarkko's patches 
> > to start. Every command exposed needs a very careful security 
> > analysis first, and we should start with only the commands we know 
> > are safe :\
> > 
> > > > Realistically people in less senstive environments will want to 
> > > > use the well known TPM passwords and still have reasonable 
> > > > safety in their unprivileged accounts.
> > > 
> > > Can we not do most of this with localities?  In theory locality 0 
> > > is supposed to be only the bios and the boot manager and the OS 
> > > gets to access 1-3.  We could reserve one for the internal kernel 
> > > and still have a couple for userspace (I'll have to go back and 
> > > check numbers; I seem to remember there were odd restrictions on 
> > > which PCR you can reset and extend in which locality).  If we 
> > > have two devices (one for each locality) we could define a UNIX 
> > > ACL on the devices to achieve what you want.
> > 
> > Good point, yes, localities should be thought about when designing
> > this new RM char dev uAPI...
> > 
> > Our support for localities in the kernel today uses some really 
> > gross sysfs file and is basically insane, IMHO.
> > 
> > Maybe there should be a /dev/tpmrm for each locality? If so then 
> > only the safe one with unwritable localities can be 0666 by
> > default..
> 
> Do you see that it would be possible to have ioctl for setting the
> locality, or is it out of the question? I'm planning to have an ioctl
> for the whitelist anyway.

For localities, assuming they can have real meaning in terms of the
protection model, I think one device per locality is better than an
ioctl because device policy is settable in underspace via the UNIX ACL
and hence locality policy is too.  If we have an ioctl, we then have to
introduce a "who's allowed to do this?" policy in the kernel.

I also think the command filter actually needs more thought.  Right at
the moment, if we go with the current proposals, the kernel will create
two devices: /dev/tpm<n> and /dev/tpms<n>.  By default they'll both be
root owned and 0600, so the current patch adequately protects the TPM.

I think we go with this now and do the filter later.

On the filter design:

Now if we look at use cases, for my laptop, where I'm the only user, I
want unrestricted access  to the TPM.  I can achieve that by making
/dev/tpms0 0666 (or changing its ownership to me).

Jason's use case is devices running non-root apps that need restricted
TPM access.  For them, a single filter on /dev/tpms0 might work,
although there might be unrestricted apps needing a broader range of
tpm access (perhaps not all running as root?)

For the cloud use case, we're going to have a variety of applications
each with a variety of restrictions (for instance, the orchestration
system is definitely going to need PCR extensions if it's doing
attestations, but the guests might not want this) etc.

I think all this points to multiple potential users each with their own
filter, so I think the actual architecture for the filter is an ioctl
which adds a new filtered device connected to the RM which may be
executed many times.  That way the creator of the device can decide the
filter policy and the use policy via the standard device UNIX ACL and
you can have lots of them to make this fine grained.  It could also be
done with something /dev/ptmx like, so perhaps a filesystem may be the
answer as well?

If you want, I can commit to building this once we have all the
requirements and we can get Jarkko's patch set reviewed now without it.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Wed, Jan 04, 2017 at 06:53:03AM -0800, James Bottomley wrote:

> > > But this is not trousers, this is an in-kernel 0666 char dev that 
> > > will be active on basically every Linux system with a TPM. I think 
> > > we have a duty to be very conservative here.
> 
> Just to note on this that trousers *is* effectively an 0666 kernel
> device: all tcsd does is run with root privileges on the real /dev/tpm0
> and mediate the calls.  It doesn't seem to police them at all.

That may be, but IHMO trousers is simply not relevant. Real systems do
not seem to use trousers. I don't use it. Google doesn't use it. You
report it is crashy.

To me it just doesn't represent a reasonable way to use the TPM
hardware.

> For localities, assuming they can have real meaning in terms of the
> protection model, I think one device per locality is better than an
> ioctl because device policy is settable in underspace via the UNIX ACL
> and hence locality policy is too.

Yes.

> I also think the command filter actually needs more thought.  Right at
> the moment, if we go with the current proposals, the kernel will create
> two devices: /dev/tpm<n> and /dev/tpms<n>.  By default they'll both be
> root owned and 0600, so the current patch adequately protects the TPM.

Yes, but, considering the goals here I'd rather see the default kernel
permissions for tpms be 0666 ....

You are doing all this work to get the user space side in shape, I'd
like to see matching kernel support. To me that means out-of-the-box
a user can just use your plugins, the plugins will access /dev/tmps
and everything will work fine for RSA key storage.

No messing with udev, no opt-in to TPM usage, no daemon to install,
no chmod on a dev node. It Just Works.

> Now if we look at use cases, for my laptop, where I'm the only user, I
> want unrestricted access  to the TPM.  I can achieve that by making
> /dev/tpms0 0666 (or changing its ownership to me).

This usecase is already handled by making /dev/tmp0 accessible to the
user. Keeping the 'enable RM' ioctl makes that a little wonky but
entirely workable..

> Jason's use case is devices running non-root apps that need restricted
> TPM access.  For them, a single filter on /dev/tpms0 might work,
> although there might be unrestricted apps needing a broader range of
> tpm access (perhaps not all running as root?)

Yes, I have a range of usage restrictions.

As an example: My systems support key migration, so I want to make it
very hard for an attacker to use the migration machinery to steal an
RSA key. The user controls the migration password, I hope it is
strong, but even so the system is currently desgined so that only a
ssh user can even issue migration commands to the tpm. Someone hacking
a network daemon simply cannot.

This is the sort of defence in depth I think is imporant in a security
system like this.

> For the cloud use case, we're going to have a variety of applications
> each with a variety of restrictions (for instance, the orchestration
> system is definitely going to need PCR extensions if it's doing
> attestations, but the guests might not want this) etc.

To me a design for how the PCRs actually need to work is what is
missing here. I only minimially understand this use case...

And it seems like a big leap that orchestration software *needs*
unprivileged TPM access.

> I think all this points to multiple potential users each with their own
> filter, so I think the actual architecture for the filter is an ioctl
> which adds a new filtered device connected to the RM which may be
> executed many times.

Maybe, but that also seems like over kill.. It entirely depends on
what the PCR use model actually is.

Here is an alternative starting idea:

- Introduce /dev/tpms a single cdev node that can talk to all chips
  and all localities.
- By default it is 0666
- By default it has a very strong filter allowing only key load,
  some key ops and any thing else we can identify as unambiguously safe.
- It has a root-only ioctl that can change the filter (op, chip)
- It has a root-only ioctl that can change the locality
- Keep the enable-RM ioctl on /dev/tmpX, except that enable-RM
  would switch the /dev/tpm0 FD to only use the above uAPI and
  set an all-permissive filter.

The followup would be a TPM namespace design that meets the needs of
people working on containers and related. We already know we need this
today for IMA in containers and vtpm isolation.

TPM namespaces would basically control the filtering options on
/dev/tmps and set the default TPM. So one could run orchestration
software unprivileged inside a TPM namespace with greater access.

This should be enough stuff for people to explore different security
models in user space.

The root-only ioctls and /dev/tpm0 are enough to let user space create
FDs with whatever properties are needed and pass them to unprivileged,
eg via fd passing or via open-as-root/drop privs techniques.

The default configuration is enough to enable the RSA key model that
we actually do understand well and almost have code for :) I think
this is very important..

We don't introduce any new security risks we don't understand.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Wed, 2017-01-04 at 11:31 -0700, Jason Gunthorpe wrote:
> On Wed, Jan 04, 2017 at 06:53:03AM -0800, James Bottomley wrote:
> 
> > > > But this is not trousers, this is an in-kernel 0666 char dev 
> > > > that will be active on basically every Linux system with a TPM. 
> > > > I think we have a duty to be very conservative here.
> > 
> > Just to note on this that trousers *is* effectively an 0666 kernel
> > device: all tcsd does is run with root privileges on the real 
> > /dev/tpm0 and mediate the calls.  It doesn't seem to police them at
> > all.
> 
> That may be, but IHMO trousers is simply not relevant. Real systems 
> do not seem to use trousers. I don't use it. Google doesn't use it. 
> You report it is crashy.
> 
> To me it just doesn't represent a reasonable way to use the TPM
> hardware.

It basically represents the only current way until there's a new API,
so all our current key handling tools use it.  Given how I slammed it
in Plumbers, I'd be the last one to defend its actual API as usable ...
we just don't have another (yet).

> > For localities, assuming they can have real meaning in terms of the
> > protection model, I think one device per locality is better than an
> > ioctl because device policy is settable in underspace via the UNIX 
> > ACL and hence locality policy is too.
> 
> Yes.
> 
> > I also think the command filter actually needs more thought.  Right 
> > at the moment, if we go with the current proposals, the kernel will
> > create two devices: /dev/tpm<n> and /dev/tpms<n>.  By default 
> > they'll both be root owned and 0600, so the current patch 
> > adequately protects the TPM.
> 
> Yes, but, considering the goals here I'd rather see the default 
> kernel permissions for tpms be 0666 ....
> 
> You are doing all this work to get the user space side in shape, I'd
> like to see matching kernel support. To me that means out-of-the-box
> a user can just use your plugins, the plugins will access /dev/tmps
> and everything will work fine for RSA key storage.

Actually, not necessarily; you're not considering the setup issue:
right at the moment users get delivered TPMs mostly in the cleared
state (thankfully they no longer have to clear via bios).  So the first
thing a new user has to do is set all the authorizations and create an
SRK equivalent primary object at 0x81000001.  I think in the interests
of best practice we want to make that as easy as possible; saying they
have to do this as root and use a different device is problematic.

You can say they don't have to use a different device because the
filter can be lifted for root, but then how do I lock down root apps
for this untrusted root setup secure boot has going on?

I suppose we could use TPMA_PERMANENT for this. The first three bits
indicate whether the authorizations are set, so if they're all clear,
we can assume an unowned TPM and lift the filter?  A sort of trust on
first use model.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Wed, Jan 04, 2017 at 10:57:51AM -0800, James Bottomley wrote:

> > You are doing all this work to get the user space side in shape, I'd
> > like to see matching kernel support. To me that means out-of-the-box
> > a user can just use your plugins, the plugins will access /dev/tmps
> > and everything will work fine for RSA key storage.
> 
> Actually, not necessarily; you're not considering the setup issue:
> right at the moment users get delivered TPMs mostly in the cleared

I have no problem with users being instructed to do 'sudo
tpm2-provision' or having that happen via GUI using the usual
privilege escalation techniques.

> state (thankfully they no longer have to clear via bios).  So the first
> thing a new user has to do is set all the authorizations and create an
> SRK equivalent primary object at 0x81000001.  I think in the interests
> of best practice we want to make that as easy as possible; saying they
> have to do this as root and use a different device is problematic.

The device names should never be exposed to the user. The user should
specify a chip number (default to 0) and the tools should select the
correct available device to do what the user is asking.

First try /dev/tpms and elevate filter, then try /dev/tpmX, then fail.

> You can say they don't have to use a different device because the
> filter can be lifted for root, but then how do I lock down root apps
> for this untrusted root setup secure boot has going on?

Presumably the same way you lock down /dev/tpm0 today?

selinux I guess?

> I suppose we could use TPMA_PERMANENT for this. The first three bits
> indicate whether the authorizations are set, so if they're all clear,
> we can assume an unowned TPM and lift the filter?  A sort of trust on
> first use model.

I feel tpm provisioning is something that should only be done by the
system owner, and that means root in unix parlance.

I don't want random end-users provisioning the TPM in my server, for
instance.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Tue, Jan 03, 2017 at 02:47:02PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 03, 2017 at 08:36:10AM -0800, James Bottomley wrote:
> 
> > > I'm not sure about this. Why you couldn't have a very thin daemon 
> > > that prepares the file descriptor and sends it through UDS socket to 
> > > a client.
> > 
> > So I'm a bit soured on daemons from the trousers experience: tcsd
> > crashed regularly and when it did it took all the TPM connections down
> > irrecoverably.  I'm not saying we can't write a stateless daemon to fix
> > most of the trousers issues, but I think it's valuable first to ask the
> > question, "can we manage without a daemon at all?"  I actually think
> > the answer is "yes", so I'm interested in seeing how far that line of
> > research gets us.
> 
> There is clearly no need for a daemon to be involved when working on
> simple tasks like key load and key sign/enc/dec actions, adding such a
> thing only increases the complexity.
> 
> If we discover a reason to have a daemon down the road then it should
> work in some way where the user space can call out to the daemon over
> a different path than the kernel. (eg dbus or something)
> 
> > Do you have a link to the presentation?  The Plumbers etherpad doesn't
> > contain it.  I've been trying to work out whether a properly set up TPM
> > actually does need any protections at all.  As far as I can tell, once
> > you've set all the hierarchy authorities and the lockout one, you're
> > pretty well protected.
> 
> I think we should also consider TPM 1.2 support in all of this, it is
> still a very popular peice of hardware and it is equally able to
> support a RM.

I'm not against considering TPM 1.2 support but getting both in the
same patch set would be too much.

> 
> So, in general, I'd prefer to see the unprivileged char dev hard
> prevented by the kernel from doing certain things:
> 
> - Wipe the TPM
> - Manipulate the SRK, nvram, tpm flags, change passwords etc
> - Read back the EK
> - Write to PCRs
> - etc.

I rather have an ioctl where you can supply a list of CCs that you
want to allow a client to do.

/Jarkko

> Even if TPM 2 has a stronger password based model, I still think the
> kernel should hard prevent those sorts of actions even if the user
> knows the TPM password.
> 
> Realistically people in less senstive environments will want to use
> the well known TPM passwords and still have reasonable safety in their
> unprivileged accounts.
> 
> Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > This patch set adds support for TPM spaces that provide a context
> > for isolating and swapping transient objects. This patch set does
> > not yet include support for isolating policy and HMAC sessions but
> > it is trivial to add once the basic approach is settled (and that's
> > why I created an RFC patch set).
> 
> The approach looks fine to me.  The only basic query I have is about
> the default: shouldn't it be with resource manager on rather than off? 
>  I can't really think of a use case that wants the RM off (even if
> you're running your own, having another doesn't hurt anything, and it's
> still required to share with in-kernel uses).

I haven't looked too closely at TPM 2.0 stuff, but at least for 1.2 we
should have a kernel white-list of allowed commands within a RM
context, so having the RM on by default would break all of the user
space.

I really think the only way forward here is a new char dev that is
safe for unprivileged/concurrent use and migrate the user space stack
to use it instead.

> And with that, I've TPM 2 enabled both gnome-keyring and openssl:
> 
> https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/gnome-keyring
> https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/openssl_tpm_engine
 
> I'm running them in production on my day to day laptop and so far
> everything's working nicely (better than 1.2, in fact, since tcsd
> periodically crashes necessitating a restart of everything).

You granted your unprivileged user access to /dev/tpm0 then? FYI I
think that is a dangerous idea..

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Tue, 2017-01-03 at 14:32 -0700, Jason Gunthorpe wrote:
> On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > This patch set adds support for TPM spaces that provide a context
> > > for isolating and swapping transient objects. This patch set does
> > > not yet include support for isolating policy and HMAC sessions 
> > > but it is trivial to add once the basic approach is settled (and
> > > that's why I created an RFC patch set).
> > 
> > The approach looks fine to me.  The only basic query I have is 
> > about the default: shouldn't it be with resource manager on rather 
> > than off?  I can't really think of a use case that wants the RM off 
> > (even if you're running your own, having another doesn't hurt 
> > anything, and it's still required to share with in-kernel uses).
> 
> I haven't looked too closely at TPM 2.0 stuff, but at least for 1.2 
> we should have a kernel white-list of allowed commands within a RM
> context, so having the RM on by default would break all of the user
> space.
> 
> I really think the only way forward here is a new char dev that is
> safe for unprivileged/concurrent use and migrate the user space stack
> to use it instead.

That's effectively what /dev/tpms0 would be, with /dev/tpm0 giving full
fledged access.

> > And with that, I've TPM 2 enabled both gnome-keyring and openssl:
> > 
> > https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/gnome
> > -keyring
> > https://build.opensuse.org/package/show/home:jejb1:Tumbleweed/opens
> > sl_tpm_engine
>  
> > I'm running them in production on my day to day laptop and so far
> > everything's working nicely (better than 1.2, in fact, since tcsd
> > periodically crashes necessitating a restart of everything).
> 
> You granted your unprivileged user access to /dev/tpm0 then? FYI I
> think that is a dangerous idea..

No, I granted access to the resource manager device (I'm running with a
combination of Jarkko's and my patches).

James

RE: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Fuchs, Andreas]

Great to see this coming along so well. Thanks a lot to Jarkko !
I just wanted to point out a few things I deem important at this point:

- Number of virtual handles:
>From what I see there are currently 14 slots for virtual objects in the RFC (if I'm mistaking, please correct me).
I'd advice to ask the TPM2_GetCapabilities(TPM_CAP_TPM_PROPERTIES, TPM_PT_HR_TRANSIENT_MIN or TPM_PT_HR_TRANSIENT_AVAIL)
[Note: there is no actual max, i.e. the TPM will allow more transient objects that e.g. 3 if they are small] 
and provide each TPM space with the same amount as the TPM will tell them is available.
If an application needs more objects, I'd see a per-fd mini-RM module inside the TSS-libraries handling that job quite well.
Same would apply for Session with TPM_PT_HR_LOADED_MIN and TPM_PT_HR_LOADED_AVAIL.
This will reduce the memory consumption inside the kernel and provide userspace with a consistent view on the GetCapabilities vs its actual Allocations.

- Enumeration of loaded (virtual) handles:
The TPM allows an application to get the list of currently loaded handles TPM2_GetCapabilities(TPM_CAP_HANDLES).
It would be great to have the RM be as transparent to userspace as possible. The RM spec of TCG therefore says that you need to intercept and override this command (unless it is run in an authentication session where you cannot override it, which is disadviced). It's a design choice, but I'd advice for it after long discussions.

- Constant session handles (hurray):
Sessions do not change their handles on contextsave/contextload, so you do not need to virtualize session handles.
In fact you must not do so, because cpHash-calculation needs to know the TPM's handle number for sessions on at least 1 command.
So this simplifies session handling inside the kernel since you do not need to alter the handle area for session handles.

- Session Limits (here it gets ugly):
Even thought the TPM supports the same swapping-scheme for sessions as it does for transient objects, it only allows for a limited number of session to be opened (64 in case of PC-Client), called active sessions.
This means that a single process can still DoS the TPM if it allocates 64 sessions, or 64 processes can DoS the TPM if they allocate 1 session each.
There are two principle solutions:
a) Limit the number of active sessions per fd, process, user and hope for the best. Of course this will not really protect you from DoS'ed TPMs.
b) Kick out old sessions whenever new sessions are requested and TPM is currently full (the TCG RM spec approach). Of course applications need to handle "randomly vanishing" hmac sessions in this case.

- Session ungaping (here it gets REALLY ugly):
The TPM has some scheme for handling sessions that are swapped (contextSaved) out. In this scheme, it can run into the case where it will deny actions on a session handle with a TPM2_RC_GAP error.
This error means that the time between last usage of the oldest session and the current session is too far apart.
The reaction needs to be that the RM loads this oldest sesssion (or in my implementation all swaped sessions) into the TPM and contextsave them back right away.
This becomes especially ugly, when enabling the ability of userspace to contextsave a session on one fd and contextload this session on another fd (or even from another process).


I hope your can parse what I wrote, feel free to ask for more details on any point.
I'll be more than happy to help concepting around these problems but atm unfortunately I'm unable to help with actual code ... for reasons...

Best regards,
Andreas





________________________________________
From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
Sent: Monday, January 02, 2017 14:22
To: tpmdd-devel@lists.sourceforge.net
Cc: linux-security-module@vger.kernel.org; open list
Subject: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

This patch set adds support for TPM spaces that provide a context
for isolating and swapping transient objects. This patch set does
not yet include support for isolating policy and HMAC sessions but
it is trivial to add once the basic approach is settled (and that's
why I created an RFC patch set).

There's a test script for trying out TPM spaces in

  git://git.infradead.org/users/jjs/tpm2-scripts.git

A simple smoke test can be run by

  sudo python -m unittest -v tpm2_smoke.SpaceTest

Jarkko Sakkinen (4):
  tpm: migrate struct tpm_buf to struct tpm_chip
  tpm: validate TPM 2.0 commands
  tpm: export tpm2_flush_context_cmd
  tpm: add the infrastructure for TPM space for TPM 2.0

 drivers/char/tpm/Makefile        |   2 +-
 drivers/char/tpm/tpm-chip.c      |  15 ++
 drivers/char/tpm/tpm-dev.c       |  80 ++++++++++-
 drivers/char/tpm/tpm-interface.c |  93 +++++++++----
 drivers/char/tpm/tpm-sysfs.c     |   2 +-
 drivers/char/tpm/tpm.h           | 106 ++++++++------
 drivers/char/tpm/tpm2-cmd.c      | 232 ++++++++++++++++---------------
 drivers/char/tpm/tpm2-space.c    | 288 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/tpm.h         |  23 ++++
 9 files changed, 662 insertions(+), 179 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2-space.c
 create mode 100644 include/uapi/linux/tpm.h

--
2.9.3


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Thu, Jan 05, 2017 at 03:52:02PM +0000, Fuchs, Andreas wrote:
> Great to see this coming along so well. Thanks a lot to Jarkko !

> The TPM allows an application to get the list of currently loaded
> handles TPM2_GetCapabilities(TPM_CAP_HANDLES).  It would be great to
> have the RM be as transparent to userspace as possible. The RM spec
> of TCG therefore says that you need to intercept and override this

I'd rather just ban unnecessary stuff like this on the RM fd.
Tracking active handles can be done in userspace by the app
itself. Debugging can be done by using the non-RM fd or debugfs.

IMHO we need to focus narrowly on enabling *specific* unpriviledged
user space use models and safe co-existence with kernel users.

> - Constant session handles (hurray):

> Sessions do not change their handles on contextsave/contextload, so
> you do not need to virtualize session handles.

The kernel still needs to track them, so they can be cleaned up and
access controlled.

> - Session Limits (here it gets ugly):

> Even thought the TPM supports the same swapping-scheme for sessions
> as it does for transient objects, it only allows for a limited
> number of session to be opened (64 in case of PC-Client), called
> active sessions.  This means that a single process can still DoS the
> TPM if it allocates 64 sessions, or 64 processes can DoS the TPM if

Well, if we have an unpriv fd then it should not be able to DOS the
system - that would suggest either that FD cannot use sessions or we
need some kernel solution to guarentee the DOS is not possible.

A combo ioctl that could setup the session, issue an operation in it
and then delete the session, for instance.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Thu, 2017-01-05 at 10:27 -0700, Jason Gunthorpe wrote:
> On Thu, Jan 05, 2017 at 03:52:02PM +0000, Fuchs, Andreas wrote:
> > Great to see this coming along so well. Thanks a lot to Jarkko !
> 
> > The TPM allows an application to get the list of currently loaded
> > handles TPM2_GetCapabilities(TPM_CAP_HANDLES).  It would be great 
> > to have the RM be as transparent to userspace as possible. The RM 
> > spec of TCG therefore says that you need to intercept and override
> > this
> 
> I'd rather just ban unnecessary stuff like this on the RM fd.
> Tracking active handles can be done in userspace by the app
> itself. Debugging can be done by using the non-RM fd or debugfs.

Yes, we basically agreed on not doing this.  The only handles that
actually need translating are the transient 0x80 ones.  Since the RM
effectively segregates them, you can't see anyone else's, so the only
query could be about the application's own transient handles and it's
difficult to see how it could lose track of them and want to issue this
query.  So the best course is to leave it unimplemented (less code) and
see if anyone complains because they have an actual use case.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Andreas Fuchs]

Am 05.01.2017 um 19:06 schrieb James Bottomley:
> On Thu, 2017-01-05 at 10:27 -0700, Jason Gunthorpe wrote:
>> On Thu, Jan 05, 2017 at 03:52:02PM +0000, Fuchs, Andreas wrote:
>>> Great to see this coming along so well. Thanks a lot to Jarkko !
>>> The TPM allows an application to get the list of currently loaded
>>> handles TPM2_GetCapabilities(TPM_CAP_HANDLES).  It would be great
>>> to have the RM be as transparent to userspace as possible. The RM
>>> spec of TCG therefore says that you need to intercept and override
>>> this
>> I'd rather just ban unnecessary stuff like this on the RM fd.
>> Tracking active handles can be done in userspace by the app
>> itself. Debugging can be done by using the non-RM fd or debugfs.
> Yes, we basically agreed on not doing this.  The only handles that
> actually need translating are the transient 0x80 ones.  Since the RM
> effectively segregates them, you can't see anyone else's, so the only
> query could be about the application's own transient handles and it's
> difficult to see how it could lose track of them and want to issue this
> query.  So the best course is to leave it unimplemented (less code) and
> see if anyone complains because they have an actual use case.

Then how about blocking
TPM2_GetCapabilities(TPM_CAP_HANDLES, 0x80000000) ?

My concern is with a consistent view, so you either get the correct
result or no result, but please no false results...

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Thu, 2017-01-05 at 10:27 -0700, Jason Gunthorpe wrote:
> On Thu, Jan 05, 2017 at 03:52:02PM +0000, Fuchs, Andreas wrote:
[...]
> > - Session Limits (here it gets ugly):
> 
> > Even thought the TPM supports the same swapping-scheme for sessions
> > as it does for transient objects, it only allows for a limited
> > number of session to be opened (64 in case of PC-Client), called
> > active sessions.  This means that a single process can still DoS 
> > the TPM if it allocates 64 sessions, or 64 processes can DoS the
> > TPM if
> 
> Well, if we have an unpriv fd then it should not be able to DOS the
> system - that would suggest either that FD cannot use sessions or we
> need some kernel solution to guarentee the DOS is not possible.

The sessions issue is a bit of a monster nightmare.  The basic problem
is this issue that they're not fully virtualizable: the TPM retains
knowledge that a session existed on this handle even if the session
context is offloaded. It's this space for "session exists" that the TPM
has a limited space for.

> A combo ioctl that could setup the session, issue an operation in it
> and then delete the session, for instance.

This would work for encryption or HMAC sessions, but probably not for
policy sessions, because they can have an arbitrarily large command
sequence to construct them.

The other issue we're likely to run into if we do it this way is
delayed error reporting.

How about a more traditional approach which would be leasing (basically
what we use for NFS).  Any application opening a session would have a
certain time (probably in ms) to complete it or we'd close the handle
and flush it.  We'd store the jiffies time the session was first
requested and loop over all the extent sessions to find ones which get
too old.  We know there can only be TPM_PT_ACTIVE_SESSIONS_MAX of
these, so it's a cheap operation. It's only a small extra bit of logic
to take care of the GAP problem as well, I think.  If we're full when
you try and start a session, we block you until a handle becomes free
and the max lease time guarantees when this is.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Thu, Jan 05, 2017 at 10:33:43AM -0800, James Bottomley wrote:

> > A combo ioctl that could setup the session, issue an operation in it
> > and then delete the session, for instance.
> 
> This would work for encryption or HMAC sessions, but probably not for
> policy sessions, because they can have an arbitrarily large command
> sequence to construct them.

I'd rather give up features (eg policy sessions, if necessary) for the
unpriv fd than give up security of the unpriv fd.

We always have the out that special stuff can go down the priv path.

> The other issue we're likely to run into if we do it this way is
> delayed error reporting.

Not sure I follow.

> How about a more traditional approach which would be leasing (basically
> what we use for NFS).  Any application opening a session would have

Doesn't this just change the DOS vector? Now the attacker has to delay
execution of TPM commands long enough to force session leases to time
out. That isn't too hard to do, asking the TPM to make a RSA key can
take seconds, for instance.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Thu, 2017-01-05 at 12:20 -0700, Jason Gunthorpe wrote:
> On Thu, Jan 05, 2017 at 10:33:43AM -0800, James Bottomley wrote:
> 
> > > A combo ioctl that could setup the session, issue an operation in
> > > it
> > > and then delete the session, for instance.
> > 
> > This would work for encryption or HMAC sessions, but probably not 
> > for policy sessions, because they can have an arbitrarily large 
> > command sequence to construct them.
> 
> I'd rather give up features (eg policy sessions, if necessary) for 
> the unpriv fd than give up security of the unpriv fd.

We don't really have that choice: Keys require authorization, so you
have to have an auth session.  If you want things like PCR sealed or
time limited keys, you don't really have a choice on policy sessions
either.

I think we've got to the point where arguing about our divergent use
requirements shows the default should be 0600 and every command enabled
so that whatever changes the device to 0666 also applies the command
filter policy.  It's the fully safe default that satisfies everyone.
Once you have the command blacklist, you can blacklist every policy
command before you make your device 0666.  That effecively achieves
what you want because no-one can now initiate a policy session.

> We always have the out that special stuff can go down the priv path.
> 
> > The other issue we're likely to run into if we do it this way is
> > delayed error reporting.
> 
> Not sure I follow.

If we try to issue a load of commands as a transaction, you only get
the error when the transaction is executed, even if the entity causing
the problem is way back in the command sequence.

> > How about a more traditional approach which would be leasing 
> > (basically what we use for NFS).  Any application opening a session
> > would have
> 
> Doesn't this just change the DOS vector? Now the attacker has to 
> delay execution of TPM commands long enough to force session leases 
> to time out. That isn't too hard to do, asking the TPM to make a RSA 
> key can take seconds, for instance.

I really don't think we can prevent all types of DoS in the TPM.  After
all, in a lot of current silicon, it can take up to 90 seconds to
generate an RSA key using the KDF ... the TPM is unavailable while this
is happening, so there's your DoS on a standard command.

What we need to aim for is effective resource sharing.  The 64 handle
limit is one of the nasty ones that a bunch of applications could
accidentally overflow.  A lease model bounds the TPM blocked time while
you're waiting for an available handle and coping with a lease timeout
can be done in the TSS.  TPM execution time could be added to lease
expiry, so even if someone's doing a sequence of RSA KDFs, you
eventually get your job executed as long as we do the execution of
blocked commands in a strict order.  In this model, you can "DoS" me by
substantially delaying the execution of my commands, but you can't
prevent it from executing within a bounded time.  I suppose we could
even make TPM execution time an RLIMIT.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Thu, Jan 05, 2017 at 11:55:49AM -0800, James Bottomley wrote:

> We don't really have that choice: Keys require authorization, so you
> have to have an auth session.

I know, this is why I suggested a combo op (kernel level atomicity
is clearly DOS safe)..

> If you want things like PCR sealed or time limited keys, you don't
> really have a choice on policy sessions either.

.. and advanced stuff like is what I was talking about giving up for
unpriv if it can't be allowed safely ...

> I think we've got to the point where arguing about our divergent use
> requirements shows the default should be 0600 and every command enabled
> so that whatever changes the device to 0666 also applies the command

Well, that is what we already have with /dev/tpm0.

I'm very surprised by this level of disagreement, so I'm inclined to
drop the idea that the kernel can directly support a 0666 cdev at all.

Lets stick with the user space broker process and just introduce
enough kernel RM to enable co-existance with kernel users and clean-up
on crash. This should be enough to make a user space broker much
simpler.

So Jarkko's uapi is basically fine.. No need for a kernel white list/etc

I had really hoped we could have a secure default 0666 cdev that would
be able to support the basic use of your user space plugins without a
daemon :(

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Thu, 2017-01-05 at 15:21 -0700, Jason Gunthorpe wrote:
> On Thu, Jan 05, 2017 at 11:55:49AM -0800, James Bottomley wrote:
> 
> > We don't really have that choice: Keys require authorization, so 
> > you have to have an auth session.
> 
> I know, this is why I suggested a combo op (kernel level atomicity
> is clearly DOS safe)..

Transactions are a hard thing to guarantee to be DoS safe and the more
complex they get, the more difficult they are to police within the
kernel.  Plus we have to keep the R/W interface for backwards
compatibility now that we have it and I just don't see how we could
layer transactions into it without having some sort of in-kernel
emulator.

> > If you want things like PCR sealed or time limited keys, you don't
> > really have a choice on policy sessions either.
> 
> .. and advanced stuff like is what I was talking about giving up for
> unpriv if it can't be allowed safely ...
> 
> > I think we've got to the point where arguing about our divergent 
> > use requirements shows the default should be 0600 and every command
> > enabled so that whatever changes the device to 0666 also applies 
> > the command
> 
> Well, that is what we already have with /dev/tpm0.

Except that doesn't have the RM.

> I'm very surprised by this level of disagreement, so I'm inclined to
> drop the idea that the kernel can directly support a 0666 cdev at
> all.

Great.  We'll keep it at 0600 and let userspace sort it out; that way
policy becomes flexible too.

> Lets stick with the user space broker process and just introduce
> enough kernel RM to enable co-existance with kernel users and clean
> -up on crash. This should be enough to make a user space broker much
> simpler.

I wouldn't go that far.  I'm still planning a userspace tss2 without
any access broker daemon, but let's see how far I get on top of the RM.
 I think building in stages is a good way to get actual use experience
to guide the next stage.

> So Jarkko's uapi is basically fine.. No need for a kernel white
> list/etc

I suspect we'll eventually get to needing one, but I'm happy to begin
without and see what that experience tells us before we try to build
it.  This is actually a better way of doing stuff because we can add to
an API based on what we find in the field; the hard thing is pulling
back an API that doesn't work.

> I had really hoped we could have a secure default 0666 cdev that
> would be able to support the basic use of your user space plugins
> without a daemon :(

I think we can; I just don't think we can define a single in-kernel use
policy that supports everyone's use case, so punting to userspace and
letting it sort out the desired policy for the platform will work for
everyone.

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Thu, Jan 05, 2017 at 02:58:46PM -0800, James Bottomley wrote:
> On Thu, 2017-01-05 at 15:21 -0700, Jason Gunthorpe wrote:
> > On Thu, Jan 05, 2017 at 11:55:49AM -0800, James Bottomley wrote:
> > 
> > > We don't really have that choice: Keys require authorization, so 
> > > you have to have an auth session.
> > 
> > I know, this is why I suggested a combo op (kernel level atomicity
> > is clearly DOS safe)..
> 
> Transactions are a hard thing to guarantee to be DoS safe and the more
> complex they get, the more difficult they are to police within the
> kernel.  Plus we have to keep the R/W interface for backwards
> compatibility now that we have it and I just don't see how we could
> layer transactions into it without having some sort of in-kernel
> emulator.

Again, this was only to make the unpriv FD usable and safe against
session DOS and that FD wouldn't use the legacy r/w interface. I don't
care if root can DOS the TPM via /dev/tpm0. combo ops would need to be
simple enough to reason about. (in my TPM libaries API calls are
combo'd with session anyhow, and that works quite well for my use
models)

> > Lets stick with the user space broker process and just introduce
> > enough kernel RM to enable co-existance with kernel users and clean
> > -up on crash. This should be enough to make a user space broker much
> > simpler.
> 
> I wouldn't go that far.  I'm still planning a userspace tss2 without
> any access broker daemon, but let's see how far I get on top of the RM.
>  I think building in stages is a good way to get actual use experience
> to guide the next stage.

I'm sure you can implement what you are doing on top of the RM -
that isn't a question in my mind.

My question has always been how does your plugin deliver messages to
the kernel RM in a way that does not compromise the security of the
TPM system.

> > So Jarkko's uapi is basically fine.. No need for a kernel white
> > list/etc
> 
> I suspect we'll eventually get to needing one, but I'm happy to
> begin

IMHO the problem with trousers as a broker is just how horribly
complex it was.

With the kernel RM this problem becomes very simple:

 - 1:1 relationship between incoming clients and kernel fds (Jarkko's
   existing design allows a broker to safely create many RM fds)
 - Trivial inspection of messages to determine op and check whitelist
   (just the first couple bytes give you the opcode, easy to deep
    inspect things like get capability)
 - No marshal/demarshal, no virtualization, no crypto.
   (kernel does virtualization, client does marshal and crypto)
   
I'm not sure what reason would be big enough to put it in the kernel
when we seem to have irreconcilable use models for the security policy
it needs to implement...

Maybe that is OK, but it isn't what I was hoping for at the start :)

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[James Bottomley]

On Thu, 2017-01-05 at 16:50 -0700, Jason Gunthorpe wrote:
> On Thu, Jan 05, 2017 at 02:58:46PM -0800, James Bottomley wrote:
> > On Thu, 2017-01-05 at 15:21 -0700, Jason Gunthorpe wrote:
> > > On Thu, Jan 05, 2017 at 11:55:49AM -0800, James Bottomley wrote:
> > > 
> > > > We don't really have that choice: Keys require authorization, 
> > > > so you have to have an auth session.
> > > 
> > > I know, this is why I suggested a combo op (kernel level 
> > > atomicity is clearly DOS safe)..
> > 
> > Transactions are a hard thing to guarantee to be DoS safe and the 
> > more complex they get, the more difficult they are to police within 
> > the kernel.  Plus we have to keep the R/W interface for backwards
> > compatibility now that we have it and I just don't see how we could
> > layer transactions into it without having some sort of in-kernel
> > emulator.
> 
> Again, this was only to make the unpriv FD usable and safe against
> session DOS and that FD wouldn't use the legacy r/w interface. I 
> don't care if root can DOS the TPM via /dev/tpm0. combo ops would 
> need to be simple enough to reason about. (in my TPM libaries API 
> calls are combo'd with session anyhow, and that works quite well for 
> my use models)

In Ken's TSS2 they are for simple authorisation.  However, policy auth
is where it gets tricky and right at the moment I believe I need policy
based authorisation for keys.

> > > Lets stick with the user space broker process and just introduce
> > > enough kernel RM to enable co-existance with kernel users and 
> > > clean -up on crash. This should be enough to make a user space 
> > > broker much simpler.
> > 
> > I wouldn't go that far.  I'm still planning a userspace tss2 
> > without any access broker daemon, but let's see how far I get on 
> > top of the RM.  I think building in stages is a good way to get 
> > actual use experience to guide the next stage.
> 
> I'm sure you can implement what you are doing on top of the RM -
> that isn't a question in my mind.
> 
> My question has always been how does your plugin deliver messages to
> the kernel RM in a way that does not compromise the security of the
> TPM system.

So currently, it doesn't: I have to either run as root or run a udev
script to give me access to the device, neither of which will work out
of the box for distributions because of the security risks you
identified.  I think this is OK for now because the security issue only
has to be sorted out before we make this ready for general release
(i.e. advise the distros how to expose the TPM) and we need to gather
use case data before we do that.

> > > So Jarkko's uapi is basically fine.. No need for a kernel white
> > > list/etc
> > 
> > I suspect we'll eventually get to needing one, but I'm happy to
> > begin
> 
> IMHO the problem with trousers as a broker is just how horribly
> complex it was.
> 
> With the kernel RM this problem becomes very simple:
> 
>  - 1:1 relationship between incoming clients and kernel fds (Jarkko's
>    existing design allows a broker to safely create many RM fds)
>  - Trivial inspection of messages to determine op and check whitelist
>    (just the first couple bytes give you the opcode, easy to deep
>     inspect things like get capability)
>  - No marshal/demarshal, no virtualization, no crypto.
>    (kernel does virtualization, client does marshal and crypto)
>    
> I'm not sure what reason would be big enough to put it in the kernel
> when we seem to have irreconcilable use models for the security 
> policy it needs to implement...
> 
> Maybe that is OK, but it isn't what I was hoping for at the start :)

I actually think this is OK for now.  I have theories about how I'm
going to use policy in TPM keys but I haven't written any code yet. 
 The only actual code I have is the openssl and gnome-keyring patches I
posted to make use of password authorized TPM based RSA keys.

I'd really rather not say what is and isn't safe to blacklist until I
have some use experience of policy based keys: I'm going to continue
working on them (although it looks like a hard problem because we need
to standardise an ASN.1 form of key policy as well) so hopefully I can
acquire the necessary experience over the next few weeks.

I'm seriously pissed of with trousers and will port the trousers based
TPM1.2 RSA key patches I've done to whatever direct connect API you
come up with (just send me a link to the git tree or package or
whatever), so this should generate some use experience for 1.2, like
whether we actually need a RM without trousers and also what the safety
issues might be.  My main laptop is now TPM2, but I have a backup one
that's TPM1.2 and also has all the TPM key stuff as well.

Perhaps our models in practice may turn out not to be as diverse as
they appear in theory, so let's try it out.  This way we can build on
actual use experience for extending the API (and in the mean time we
keep the devices root only like they were previously)

James

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Andreas Fuchs]

Am 06.01.2017 um 01:36 schrieb James Bottomley:
> On Thu, 2017-01-05 at 16:50 -0700, Jason Gunthorpe wrote:
>> On Thu, Jan 05, 2017 at 02:58:46PM -0800, James Bottomley wrote:
>>> On Thu, 2017-01-05 at 15:21 -0700, Jason Gunthorpe wrote:
>>>> On Thu, Jan 05, 2017 at 11:55:49AM -0800, James Bottomley wrote:
>>>>
>>>>> We don't really have that choice: Keys require authorization,
>>>>> so you have to have an auth session.
>>>> I know, this is why I suggested a combo op (kernel level
>>>> atomicity is clearly DOS safe)..
>>> Transactions are a hard thing to guarantee to be DoS safe and the
>>> more complex they get, the more difficult they are to police within
>>> the kernel.  Plus we have to keep the R/W interface for backwards
>>> compatibility now that we have it and I just don't see how we could
>>> layer transactions into it without having some sort of in-kernel
>>> emulator.
>> Again, this was only to make the unpriv FD usable and safe against
>> session DOS and that FD wouldn't use the legacy r/w interface. I
>> don't care if root can DOS the TPM via /dev/tpm0. combo ops would
>> need to be simple enough to reason about. (in my TPM libaries API
>> calls are combo'd with session anyhow, and that works quite well for
>> my use models)
> In Ken's TSS2 they are for simple authorisation.  However, policy auth
> is where it gets tricky and right at the moment I believe I need policy
> based authorisation for keys.

I don't think this is TSS-specific. It also holds true for the TCG-TSS.

The thing is that you will need both, HMAC and Policy Sessions in
standard userspace applications (not only the exotic ones)...
... and they must not be time-based leases, because other IO or
user-UI might happen.

Reasons:
1. PolicyPCR is an essential feature of TPM used all over the place,
so we need support for policy sessions.
2. PolicySigned allows authentication of the user via SmartCard.
So the application will have to challenge back using the challenge
from the TPM. So no timeouts please.
3. HMAC-sessions could also be handled via authentication tokens
that take the cpHash and session as challenge and provide an
HMAC. Again we'd have IO and user interaction.

In summary, in TCG we came to the conclusion that kicking out
sessions is the best approach. IMHO session quotas per process,
user, cgroup is even better, but was not possible in the TCG spec
for a user-space cross-OS RM (no notion of such capabilities).

We spent several f2f-meetings, confcalls and emails on this topic
inside the TCG. I'd advice everyone to look into the RM-spec and
coming updates, and especially TCG members to look into potential
unrelease documents on these issues. Feel free to contact me as
well for any TCG-insights.

>>>> Lets stick with the user space broker process and just introduce
>>>> enough kernel RM to enable co-existance with kernel users and
>>>> clean -up on crash. This should be enough to make a user space
>>>> broker much simpler.
>>> I wouldn't go that far.  I'm still planning a userspace tss2
>>> without any access broker daemon, but let's see how far I get on
>>> top of the RM.  I think building in stages is a good way to get
>>> actual use experience to guide the next stage.
>> I'm sure you can implement what you are doing on top of the RM -
>> that isn't a question in my mind.
>>
>> My question has always been how does your plugin deliver messages to
>> the kernel RM in a way that does not compromise the security of the
>> TPM system.
> So currently, it doesn't: I have to either run as root or run a udev
> script to give me access to the device, neither of which will work out
> of the box for distributions because of the security risks you
> identified.  I think this is OK for now because the security issue only
> has to be sorted out before we make this ready for general release
> (i.e. advise the distros how to expose the TPM) and we need to gather
> use case data before we do that.

The all-defeating reason for having in-kernel-RM is trusted keyrings
or IMA/EVM appraise/protect or similar. They will want to use sealing
to PCRs which in turn requires policy sessions from inside the kernel
and thus RM inside the kernel to play nicely with the TSS. TCG's TSS
had to add an ugly quirk into its spec in order to work with current
trusted keyrings uapi that I really want to deprecate.
And IMHO nobody wants the kernel security modules to call back
to a userspace RM-daemon.

If everyone agrees with this presumption the only question becomes
how to do this, such that we don't need a second RM in userspace
for the 99% of use cases.

P.S. This fact should also be given some thought when discussing the
priviledged 0600 node, i.e. /dev/tpm0 without the s in the middle.

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Fri, Jan 06, 2017 at 09:59:57AM +0100, Andreas Fuchs wrote:

> 1. PolicyPCR is an essential feature of TPM used all over the place,
> so we need support for policy sessions.
> 2. PolicySigned allows authentication of the user via SmartCard.

Are smart cards 0666 in linux?

> The all-defeating reason for having in-kernel-RM is trusted keyrings
> or IMA/EVM appraise/protect or similar. They will want to use sealing
> to PCRs which in turn requires policy sessions from inside the kernel
> and thus RM inside the kernel to play nicely with the TSS.

Yes. I had hoped the in-kernel-RM could also provide safe 0666 access,
but lets move on from that idea and focus on kernel/user TPM
application co-existence...

> And IMHO nobody wants the kernel security modules to call back to a
> userspace RM-daemon.

Yep.

> If everyone agrees with this presumption the only question becomes
> how to do this, such that we don't need a second RM in userspace
> for the 99% of use cases.

Yep.

> P.S. This fact should also be given some thought when discussing the
> priviledged 0600 node, i.e. /dev/tpm0 without the s in the middle.

We are stuck with the non-RM interface for compat. There could be a
kernel option/module option/sysctl/whatever of some kind to disable it
I guess.

Jason

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jason Gunthorpe]

On Thu, Jan 05, 2017 at 04:36:42PM -0800, James Bottomley wrote:

> I'm seriously pissed of with trousers and will port the trousers based
> TPM1.2 RSA key patches I've done to whatever direct connect API you
> come up with (just send me a link to the git tree or package or
> whatever), so this should generate some use experience for 1.2, like
> whether we actually need a RM without trousers and also what the safety
> issues might be.  My main laptop is now TPM2, but I have a backup one
> that's TPM1.2 and also has all the TPM key stuff as well.

I would like to add basic 1.2 support to Jarkko's RM. I'm not sure if
my 5mth old will ever let me..

If we do that we write a simple broker as I described that works on
both 1.2 and 2.0 and trousers can go away.

Jason

Re: [PATCH RFC 0/4] RFC: in-kernel resource manager

[Ken Goldman]

On 1/5/2017 2:20 PM, Jason Gunthorpe wrote:
>
> I'd rather give up features (eg policy sessions, if necessary) for the
> unpriv fd than give up security of the unpriv fd.

Please don't give up policy.  Nearly every use case of that we think of 
for TPM 2.0 uses policy sessions.

E.g.,

In 1.2, PCR authorization was built in to the object.  In 2.0, it's a 
policy.

In 1.2, key types were restricted to certain commands.  In 2.0, it's a 
policy.

Then there are all the new use cases - time restricted keys, use count 
restricted keys, keys with a PIN, etc., all use policy.

Even use of the EK primary key requires a policy, and that's needed for 
salt (getting the first password in securely) and attestation (proof 
that the TPM is authentic).

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Jarkko Sakkinen]

On Thu, Jan 05, 2017 at 03:52:02PM +0000, Fuchs, Andreas wrote:
> Great to see this coming along so well. Thanks a lot to Jarkko !
> I just wanted to point out a few things I deem important at this point:
> 
> - Number of virtual handles:
> From what I see there are currently 14 slots for virtual objects in the RFC (if I'm mistaking, please correct me).
> I'd advice to ask the TPM2_GetCapabilities(TPM_CAP_TPM_PROPERTIES, TPM_PT_HR_TRANSIENT_MIN or TPM_PT_HR_TRANSIENT_AVAIL)
> [Note: there is no actual max, i.e. the TPM will allow more transient objects that e.g. 3 if they are small] 
> and provide each TPM space with the same amount as the TPM will tell them is available.
> If an application needs more objects, I'd see a per-fd mini-RM module inside the TSS-libraries handling that job quite well.
> Same would apply for Session with TPM_PT_HR_LOADED_MIN and TPM_PT_HR_LOADED_AVAIL.
> This will reduce the memory consumption inside the kernel and provide userspace with a consistent view on the GetCapabilities vs its actual Allocations.

I rather have a fixed size object. It keeps the implementation simple
compact and stupid and that is what we want at this point. 

Even if I did what you proposed there would not be 1:1 match with
GetCapability provided data because we need to virtual handle values.

Leaving the virtualization of message bodies in the user space is a
design choice from my side. The kernel will provide only basic mechanism
for implementing easily an RM, not a full fledged implementation.

> - Enumeration of loaded (virtual) handles:
> The TPM allows an application to get the list of currently loaded handles TPM2_GetCapabilities(TPM_CAP_HANDLES).
> It would be great to have the RM be as transparent to userspace as possible. The RM spec of TCG therefore says that you need to intercept and override this command (unless it is run in an authentication session where you cannot override it, which is disadviced). It's a design choice, but I'd advice for it after long discussions.

I don't buy this because it doesn't scale (new commands in the standard,
vendor specific commands). It's just something that is factors easier
to do in the user space.

It's not an uncommon design in the Linux kernel to have basic mechanism
in the kernel and do some of the  heavy lifting in the user space. For example,
graphics drivers are like that.

> - Session Limits (here it gets ugly):
> Even thought the TPM supports the same swapping-scheme for sessions as it does for transient objects, it only allows for a limited number of session to be opened (64 in case of PC-Client), called active sessions.
> This means that a single process can still DoS the TPM if it allocates 64 sessions, or 64 processes can DoS the TPM if they allocate 1 session each.
> There are two principle solutions:
> a) Limit the number of active sessions per fd, process, user and hope for the best. Of course this will not really protect you from DoS'ed TPMs.
> b) Kick out old sessions whenever new sessions are requested and TPM is currently full (the TCG RM spec approach). Of course applications need to handle "randomly vanishing" hmac sessions in this case.

I'll think about this. The next patch set version will include
session isolation.

> - Session ungaping (here it gets REALLY ugly):
> The TPM has some scheme for handling sessions that are swapped (contextSaved) out. In this scheme, it can run into the case where it will deny actions on a session handle with a TPM2_RC_GAP error.
> This error means that the time between last usage of the oldest session and the current session is too far apart.
> The reaction needs to be that the RM loads this oldest sesssion (or in my implementation all swaped sessions) into the TPM and contextsave them back right away.
> This becomes especially ugly, when enabling the ability of userspace to contextsave a session on one fd and contextload this session on another fd (or even from another process).

This something we are not going to support in the first production
version. I'm happy review patches that try to do this nicely after
the first version of the feature has landed. I don't care about this
feature all that much.

/Jarkko

Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager

[Andreas Fuchs]

Am 09.01.2017 um 23:39 schrieb Jarkko Sakkinen:
> On Thu, Jan 05, 2017 at 03:52:02PM +0000, Fuchs, Andreas wrote:
>> Great to see this coming along so well. Thanks a lot to Jarkko !
>> I just wanted to point out a few things I deem important at this point:
>>
>> - Number of virtual handles:
>>  From what I see there are currently 14 slots for virtual objects in the RFC (if I'm mistaking, please correct me).
>> I'd advice to ask the TPM2_GetCapabilities(TPM_CAP_TPM_PROPERTIES, TPM_PT_HR_TRANSIENT_MIN or TPM_PT_HR_TRANSIENT_AVAIL)
>> [Note: there is no actual max, i.e. the TPM will allow more transient objects that e.g. 3 if they are small]
>> and provide each TPM space with the same amount as the TPM will tell them is available.
>> If an application needs more objects, I'd see a per-fd mini-RM module inside the TSS-libraries handling that job quite well.
>> Same would apply for Session with TPM_PT_HR_LOADED_MIN and TPM_PT_HR_LOADED_AVAIL.
>> This will reduce the memory consumption inside the kernel and provide userspace with a consistent view on the GetCapabilities vs its actual Allocations.
> I rather have a fixed size object. It keeps the implementation simple
> compact and stupid and that is what we want at this point.
>
> Even if I did what you proposed there would not be 1:1 match with
> GetCapability provided data because we need to virtual handle values.
>
> Leaving the virtualization of message bodies in the user space is a
> design choice from my side. The kernel will provide only basic mechanism
> for implementing easily an RM, not a full fledged implementation.
>
>> - Enumeration of loaded (virtual) handles:
>> The TPM allows an application to get the list of currently loaded handles TPM2_GetCapabilities(TPM_CAP_HANDLES).
>> It would be great to have the RM be as transparent to userspace as possible. The RM spec of TCG therefore says that you need to intercept and override this command (unless it is run in an authentication session where you cannot override it, which is disadviced). It's a design choice, but I'd advice for it after long discussions.
> I don't buy this because it doesn't scale (new commands in the standard,
> vendor specific commands). It's just something that is factors easier
> to do in the user space.
>
> It's not an uncommon design in the Linux kernel to have basic mechanism
> in the kernel and do some of the  heavy lifting in the user space. For example,
> graphics drivers are like that.
>
>> - Session Limits (here it gets ugly):
>> Even thought the TPM supports the same swapping-scheme for sessions as it does for transient objects, it only allows for a limited number of session to be opened (64 in case of PC-Client), called active sessions.
>> This means that a single process can still DoS the TPM if it allocates 64 sessions, or 64 processes can DoS the TPM if they allocate 1 session each.
>> There are two principle solutions:
>> a) Limit the number of active sessions per fd, process, user and hope for the best. Of course this will not really protect you from DoS'ed TPMs.
>> b) Kick out old sessions whenever new sessions are requested and TPM is currently full (the TCG RM spec approach). Of course applications need to handle "randomly vanishing" hmac sessions in this case.
> I'll think about this. The next patch set version will include
> session isolation.
>
>> - Session ungaping (here it gets REALLY ugly):
>> The TPM has some scheme for handling sessions that are swapped (contextSaved) out. In this scheme, it can run into the case where it will deny actions on a session handle with a TPM2_RC_GAP error.
>> This error means that the time between last usage of the oldest session and the current session is too far apart.
>> The reaction needs to be that the RM loads this oldest sesssion (or in my implementation all swaped sessions) into the TPM and contextsave them back right away.
>> This becomes especially ugly, when enabling the ability of userspace to contextsave a session on one fd and contextload this session on another fd (or even from another process).
> This something we are not going to support in the first production
> version. I'm happy review patches that try to do this nicely after
> the first version of the feature has landed. I don't care about this
> feature all that much.

I guess all of the above is more or less debatable.

This point here however is not. It's a necessity, otherwise you
will get weird machines that (seemingly) randomly stop working
after a long time; i.e. months. So nobody will find this in tests
or be able to reproduce.

These are the kind of bugs nobody will ever track down.
So please, do not release session-RM-support without taking
care of ungaping...
This would be highly irresponsible !

Thanks,
Andreas

>
> /Jarkko