TPM 2.0 device driver blocking open

TPM 2.0 device driver blocking open

[Ken Goldman]

It appears that an open() to the TPM doesn't block if another process 
has /dev/tpm0 open.  It returns -1, an error.

Questions:

Is this expected behavior?
Was this also true for 1.2?
Is there any way to change it.  I didn't set O_NOBLOCK.  Is there 
perhaps an ioctl()?
Is this something that should be added?


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: TPM 2.0 device driver blocking open

[James Bottomley]

On Fri, 2016-12-30 at 10:53 -0500, Ken Goldman wrote:
> It appears that an open() to the TPM doesn't block if another process
> has /dev/tpm0 open.  It returns -1, an error.
> 
> Questions:
> 
> Is this expected behavior?

It's enforced in drivers/char/tpm/tpm-dev.c by this check


	/* 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)) {
		dev_dbg(&chip->dev, "Another process owns this TPM\n");
		return -EBUSY;
	}

so yes, it looks to be expected.

> Was this also true for 1.2?

In tpm 1.2 there was a single access broker daemon (tcsd) which opened
the device, so you could have multiple applications using the TPM but
only one device open.

> Is there any way to change it.  I didn't set O_NOBLOCK.  Is there 
> perhaps an ioctl()?
> Is this something that should be added?

I think for the 2.0 model of every application getting direct access,
we should make it so that every open gets a separate read/write stream
to the tpm which we send in via the locked version of tpm_transmit()
and just let the chip->tpm_mutex sort out the accesses.

I can code up a patch if no-one's already done it.

James


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: TPM 2.0 device driver blocking open

[James Bottomley]

On Fri, 2016-12-30 at 08:22 -0800, James Bottomley wrote:
> On Fri, 2016-12-30 at 10:53 -0500, Ken Goldman wrote:
> > Is there any way to change it.  I didn't set O_NOBLOCK.  Is there 
> > perhaps an ioctl()?
> > Is this something that should be added?
> 
> I think for the 2.0 model of every application getting direct access,
> we should make it so that every open gets a separate read/write 
> stream to the tpm which we send in via the locked version of 
> tpm_transmit() and just let the chip->tpm_mutex sort out the
> accesses.
> 
> I can code up a patch if no-one's already done it.

Just FYI this is the temporary hack I'm using to fix my multiple access
problem while Jarkko is working on the RM as the final solution.  All
it does is block a second and subsequent open until the first one
completes.

James

---

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 912ad30..cce67d5 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -51,23 +51,44 @@ static void timeout_work(struct work_struct *work)
 	mutex_unlock(&priv->buffer_mutex);
 }
 
+static int tpm_open_lock(struct tpm_chip *chip, int lock)
+{
+	static DECLARE_COMPLETION(wait);
+
+	if (lock) {
+		while (test_and_set_bit(0, &chip->is_open)) {
+			int ret;
+
+			dev_dbg(&chip->dev, "Another process owns this TPM\n");
+			ret = wait_for_completion_interruptible(&wait);
+			if (ret)
+				return ret;
+		}
+	} else {
+		clear_bit(0, &chip->is_open);
+		complete(&wait);
+	}
+
+	return 0;
+}
+
 static int tpm_open(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip =
 		container_of(inode->i_cdev, struct tpm_chip, cdev);
 	struct file_priv *priv;
+	int ret;
 
 	/* 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)) {
-		dev_dbg(&chip->dev, "Another process owns this TPM\n");
-		return -EBUSY;
-	}
+	ret = tpm_open_lock(chip, 1);
+	if (ret)
+		return ret;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL) {
-		clear_bit(0, &chip->is_open);
+		tpm_open_lock(chip, 0);
 		return -ENOMEM;
 	}
 
@@ -173,7 +194,7 @@ 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);
+	tpm_open_lock(priv->chip, 0);
 	kfree(priv);
 	return 0;
 }


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: TPM 2.0 device driver blocking open

[Jason Gunthorpe]

On Fri, Dec 30, 2016 at 10:46:49AM -0800, James Bottomley wrote:

> Just FYI this is the temporary hack I'm using to fix my multiple access
> problem while Jarkko is working on the RM as the final solution.  All
> it does is block a second and subsequent open until the first one
> completes.

This is how it should have been in the first place, with O_NONBLOCK to
return immediately..

The only reason I never fixed it is because of fears that userspace
will go wrong if the open() starts to block forever. If someone knows
the common userspace is generally OK then let us merge a patch like
this...

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: TPM 2.0 device driver blocking open

[Fuchs, Andreas]

Yes it is expected and was true for TPM 1.2 as well.

O_NOBLOCK only refers to the "blockiness" of read() and write() operations, not open()s.

I would highly discourage using a patch that does not block open()s.
Reason being: It will inherently become racy, when 3 processes want to use 2 transient objects inside the TPM concurrently, since the TPM only has 3 slots total => deadlock.

That's why current TSS 2.0 and TSS 1.2 assumed a resource-manager in UserSpace as signle owner of /dev/tpm0 (enforced by single-open-/dev/tpm0).
Only alternative would be a RM inside the Kernel.

Multi-Open-/dev/tpm can be a local hack, but please never distribute, merge or advocate it.

Cheers,
Andreas

________________________________________
From: Ken Goldman [kgoldman-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org]
Sent: Friday, December 30, 2016 16:53
To: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: [tpmdd-devel] TPM 2.0 device driver blocking open

It appears that an open() to the TPM doesn't block if another process
has /dev/tpm0 open.  It returns -1, an error.

Questions:

Is this expected behavior?
Was this also true for 1.2?
Is there any way to change it.  I didn't set O_NOBLOCK.  Is there
perhaps an ioctl()?
Is this something that should be added?


------------------------------------------------------------------------------
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-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: TPM 2.0 device driver blocking open

[James Bottomley]

On Mon, 2017-01-02 at 15:15 +0000, Fuchs, Andreas wrote:
> That's why current TSS 2.0 and TSS 1.2 assumed a resource-manager in 
> UserSpace

We already discussed this at Plumbers.  the problem is that the kernel
itself needs access to the TPM (in both Linux and Windows as far as I
can tell).  If you put the RM in User Space, the kernel would either
not have access or have some dependency on a user space process which
is never a good idea.

>  as signle owner of /dev/tpm0 (enforced by single-open-/dev/tpm0).
> Only alternative would be a RM inside the Kernel.

Right, so that's what we now have with Jarkko's just posted patches.

James


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot