Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1549178 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-02 14:30 +0100
    [PATCH RFC 2/4] tpm: validate TPM 2.0 commands Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-02 14:30 +0100
      Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 commands James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-04 19:30 +0100
      Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 commands Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-04 19:50 +0100
    [PATCH RFC 3/4] tpm: export tpm2_flush_context_cmd Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-02 14:30 +0100
    [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-02 14:30 +0100
      Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for  TPM 2.0 Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-02 22:20 +0100
        Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for  TPM 2.0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 05:10 +0100
          Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for  TPM 2.0 Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-03 19:50 +0100
            Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for  TPM 2.0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-04 13:50 +0100
          Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for  TPM 2.0 Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-03 20:20 +0100
            Re: [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for  TPM 2.0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-04 13:50 +0100
      Re: [tpmdd-devel] [PATCH RFC 4/4] tpm: add the infrastructure for  TPM    space for TPM 2.0 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-09 23:20 +0100
    [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-02 14:30 +0100
      Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-02 22:10 +0100
        Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 05:10 +0100
          Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-03 20:20 +0100
            Re: [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-04 13:40 +0100
    Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-02 17:40 +0100
      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-02 22:50 +0100
        Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-03 06:30 +0100
          Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 14:50 +0100
            Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-03 17:20 +0100
              Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 19:40 +0100
                Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 20:20 +0100
                  Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-04 02:30 +0100
          Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-03 23:00 +0100
            Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-04 14:10 +0100
              Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-04 18:00 +0100
          Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Andy Lutomirski <luto@kernel.org> - 2017-01-04 06:50 +0100
            Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-04 14:10 +0100
        Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 15:00 +0100
          Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-03 17:40 +0100
            Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 19:50 +0100
            Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-03 22:50 +0100
              Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-03 23:50 +0100
                Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-04 01:20 +0100
                  Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-04 01:40 +0100
                    Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-04 02:00 +0100
                  Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-04 14:00 +0100
                    Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-04 16:00 +0100
                      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-04 20:10 +0100
                        Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-04 20:30 +0100
                      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-04 20:30 +0100
              Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-04 00:30 +0100
              Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-04 13:50 +0100
              Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Ken Goldman <kgoldman@us.ibm.com> - 2017-01-04 15:00 +0100
      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-03 05:10 +0100
      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-03 22:40 +0100
        Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-03 23:10 +0100
    RE: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager "Fuchs, Andreas" <andreas.fuchs@sit.fraunhofer.de> - 2017-01-05 17:10 +0100
      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-05 18:30 +0100
        Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-05 19:10 +0100
          Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Andreas Fuchs <andreas.fuchs@sit.fraunhofer.de> - 2017-01-06 09:50 +0100
        Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-05 19:40 +0100
          Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-05 21:00 +0100
            Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-05 21:00 +0100
              Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-05 23:30 +0100
                Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-06 01:00 +0100
                  Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-06 01:40 +0100
                    Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Andreas Fuchs <andreas.fuchs@sit.fraunhofer.de> - 2017-01-06 10:10 +0100
                      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-06 20:20 +0100
                    Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2017-01-06 20:10 +0100
                Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley <jejb@linux.vnet.ibm.com> - 2017-01-06 02:40 +0100
            Re: [PATCH RFC 0/4] RFC: in-kernel resource manager Ken Goldman <kgoldman@us.ibm.com> - 2017-01-10 20:20 +0100
      Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-09 23:40 +0100
        Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager Andreas Fuchs <andreas.fuchs@sit.fraunhofer.de> - 2017-01-11 11:10 +0100

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

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

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 {

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web