Re: [tpmdd-devel] [PATCH v2 6/7] tpm: expose spaces via a device link /dev/tpms<n>

[James Bottomley]

On Fri, 2017-02-24 at 12:29 +0530, Nayna wrote:
> 
> On 02/17/2017 12:55 AM, Jarkko Sakkinen wrote:
> > From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> > 
> > Currently the tpm spaces are not exposed to userspace.  Make this
> > exposure via a separate device, which can now be opened multiple 
> > times because each read/write transaction goes separately via the
> > space.
> > 
> > Concurrency is protected by the chip->tpm_mutex for each read/write
> > transaction separately.  The TPM is cleared of all transient 
> > objects by the time the mutex is dropped, so there should be no
> > interference between the kernel and userspace.
> 
> To understand, I have two questions:
> 
> 1. How would a userspace application using TPM know whether to use 
> /dev/tpm0 or /dev/tpms0 ?

Likely they can't use /dev/tpm0 becuase it will be root only, but the
major indicator will be whether /dev/tpms0 exists or not.

> 2. How would a userspace RM know to build on top of /dev/tpm0 or 
> /dev/tpms0. And if it is built on top of /dev/tpms0, can there be 
> issues with one RM on top of other RM.

There's a known problem with RMs in that they're not fully stackable,
so I suspect the answer is that if tpms0 exists you won't use an RM,
but this is currently an area of active research.  The other potential
problem is that if you build a RM on tpm0 in userspace, it will fight
with the kernel when the kernel uses sessions.

James

> Thanks & Regards,
>     - Nayna
> 
> 
> > 
> > Signed-off-by: James Bottomley <
> > James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/char/tpm/Makefile        |  3 +-
> >   drivers/char/tpm/tpm-chip.c      | 73
> > ++++++++++++++++++++++++++++++++++++++--
> >   drivers/char/tpm/tpm-interface.c | 13 +++++--
> >   drivers/char/tpm/tpm.h           |  4 +++
> >   drivers/char/tpm/tpms-dev.c      | 65
> > +++++++++++++++++++++++++++++++++++
> >   5 files changed, 152 insertions(+), 6 deletions(-)
> >   create mode 100644 drivers/char/tpm/tpms-dev.c
> > 
> > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > index 10e5827..bbe6531 100644
> > --- a/drivers/char/tpm/Makefile
> > +++ b/drivers/char/tpm/Makefile
> > @@ -3,7 +3,8 @@
> >   #
> >   obj-$(CONFIG_TCG_TPM) += tpm.o
> >   tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2
> > -cmd.o \
> > -	 tpm-dev-common.o tpm1_eventlog.o tpm2_eventlog.o tpm2
> > -space.o
> > +	 tpm-dev-common.o tpms-dev.o tpm1_eventlog.o
> > tpm2_eventlog.o \
> > +         tpm2-space.o
> >   tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o
> >   tpm-$(CONFIG_OF) += tpm_of.o
> >   obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm
> > -chip.c
> > index 993b9ae..c71c353 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 *tpms_class;
> >   dev_t tpm_devt;
> > 
> >   /**
> > @@ -132,6 +133,14 @@ static void tpm_dev_release(struct device
> > *dev)
> >   	kfree(chip);
> >   }
> > 
> > +static void tpm_devs_release(struct device *dev)
> > +{
> > +	struct tpm_chip *chip = container_of(dev, struct tpm_chip,
> > devs);
> > +
> > +	/* release the master device reference */
> > +	put_device(&chip->dev);
> > +}
> > +
> >   /**
> >    * tpm_chip_alloc() - allocate a new struct tpm_chip instance
> >    * @pdev: device to which the chip is associated
> > @@ -168,27 +177,47 @@ struct tpm_chip *tpm_chip_alloc(struct device
> > *pdev,
> >   	chip->dev_num = rc;
> > 
> >   	device_initialize(&chip->dev);
> > +	device_initialize(&chip->devs);
> > 
> >   	chip->dev.class = tpm_class;
> >   	chip->dev.release = tpm_dev_release;
> >   	chip->dev.parent = pdev;
> >   	chip->dev.groups = chip->groups;
> > 
> > +	chip->devs.parent = pdev;
> > +	chip->devs.class = tpms_class;
> > +	chip->devs.release = tpm_devs_release;
> > +	/* get extra reference on main device to hold on
> > +	 * behalf of devs.  This holds the chip structure
> > +	 * while cdevs is in use.  The corresponding put
> > +	 * is in the tpm_devs_release
> > +	 */
> > +	get_device(&chip->dev);
> > +
> >   	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->devs.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->devs, "tpms%d", chip->dev_num);
> > +	if (rc)
> > +		goto out;
> > 
> >   	if (!pdev)
> >   		chip->flags |= TPM_CHIP_FLAG_VIRTUAL;
> > 
> >   	cdev_init(&chip->cdev, &tpm_fops);
> > +	cdev_init(&chip->cdevs, &tpms_fops);
> >   	chip->cdev.owner = THIS_MODULE;
> > +	chip->cdevs.owner = THIS_MODULE;
> >   	chip->cdev.kobj.parent = &chip->dev.kobj;
> > +	chip->cdevs.kobj.parent = &chip->devs.kobj;
> > 
> >   	chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > GFP_KERNEL);
> >   	if (!chip->work_space.context_buf) {
> > @@ -199,6 +228,7 @@ struct tpm_chip *tpm_chip_alloc(struct device
> > *pdev,
> >   	return chip;
> > 
> >   out:
> > +	put_device(&chip->devs);
> >   	put_device(&chip->dev);
> >   	return ERR_PTR(rc);
> >   }
> > @@ -244,7 +274,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);
> > @@ -254,16 +284,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->cdevs, chip->devs.devt, 1);
> > +	if (rc) {
> > +		dev_err(&chip->dev,
> > +			"unable to cdev_add() %s, major %d, minor
> > %d, err=%d\n",
> > +			dev_name(&chip->devs), MAJOR(chip
> > ->devs.devt),
> > +			MINOR(chip->devs.devt), rc);
> > +
> > +		goto err_3;
> >   	}
> > 
> > +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > +		rc = device_add(&chip->devs);
> > +	if (rc) {
> > +		dev_err(&chip->dev,
> > +			"unable to device_register() %s, major %d,
> > minor %d, err=%d\n",
> > +			dev_name(&chip->devs), MAJOR(chip
> > ->devs.devt),
> > +			MINOR(chip->devs.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->cdevs);
> > + 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)
> > @@ -271,6 +329,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->cdevs);
> > +		device_del(&chip->devs);
> > +	}
> > +
> >   	/* Make the chip unavailable. */
> >   	mutex_lock(&idr_lock);
> >   	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> > @@ -282,6 +345,10 @@ static void tpm_del_char_device(struct
> > tpm_chip *chip)
> >   		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> >   	chip->ops = NULL;
> >   	up_write(&chip->ops_sem);
> > +	/* will release the devs reference to the chip->dev unless
> > +	 * something has cdevs open
> > +	 */
> > +	put_device(&chip->devs);
> >   }
> > 
> >   static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index db5ffe9..deb2021 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -1257,9 +1257,17 @@ static int __init tpm_init(void)
> >   		return PTR_ERR(tpm_class);
> >   	}
> > 
> > -	rc = alloc_chrdev_region(&tpm_devt, 0, TPM_NUM_DEVICES,
> > "tpm");
> > +	tpms_class = class_create(THIS_MODULE, "tpms");
> > +	if (IS_ERR(tpms_class)) {
> > +		pr_err("couldn't create tpms class\n");
> > +		class_destroy(tpm_class);
> > +		return PTR_ERR(tpms_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(tpms_class);
> >   		class_destroy(tpm_class);
> >   		return rc;
> >   	}
> > @@ -1271,7 +1279,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(tpms_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 97e48a4..822ca67 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -182,7 +182,9 @@ struct tpm_chip_seqops {
> > 
> >   struct tpm_chip {
> >   	struct device dev;
> > +	struct device devs;
> >   	struct cdev cdev;
> > +	struct cdev cdevs;
> > 
> >   	/* A driver callback under ops cannot be run unless
> > ops_sem is held
> >   	 * (sometimes implicitly, eg for the sysfs code). ops
> > becomes null
> > @@ -510,8 +512,10 @@ static inline void tpm_buf_append_u32(struct
> > tpm_buf *buf, const u32 value)
> >   }
> > 
> >   extern struct class *tpm_class;
> > +extern struct class *tpms_class;
> >   extern dev_t tpm_devt;
> >   extern const struct file_operations tpm_fops;
> > +extern const struct file_operations tpms_fops;
> >   extern struct idr dev_nums_idr;
> > 
> >   enum tpm_transmit_flags {
> > diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms
> > -dev.c
> > new file mode 100644
> > index 0000000..5720885
> > --- /dev/null
> > +++ b/drivers/char/tpm/tpms-dev.c
> > @@ -0,0 +1,65 @@
> > +/*
> > + * Copyright (C) 2017 James.Bottomley@xxxxxxxxxxxxxxxxxxxxx
> > + *
> > + * GPLv2
> > + */
> > +#include <linux/slab.h>
> > +#include "tpm-dev.h"
> > +
> > +struct tpms_priv {
> > +	struct file_priv priv;
> > +	struct tpm_space space;
> > +};
> > +
> > +static int tpms_open(struct inode *inode, struct file *file)
> > +{
> > +	struct tpm_chip *chip;
> > +	struct tpms_priv *priv;
> > +	int rc;
> > +
> > +	chip = container_of(inode->i_cdev, struct tpm_chip,
> > cdevs);
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (priv == NULL)
> > +		return -ENOMEM;
> > +
> > +	rc = tpm2_init_space(&priv->space);
> > +	if (rc) {
> > +		kfree(priv);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	tpm_common_open(file, chip, &priv->priv);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tpms_release(struct inode *inode, struct file *file)
> > +{
> > +	struct file_priv *fpriv = file->private_data;
> > +	struct tpms_priv *priv = container_of(fpriv, struct
> > tpms_priv, priv);
> > +
> > +	tpm_common_release(file, fpriv);
> > +	tpm2_del_space(&priv->space);
> > +	kfree(priv);
> > +
> > +	return 0;
> > +}
> > +
> > +ssize_t tpms_write(struct file *file, const char __user *buf,
> > +		   size_t size, loff_t *off)
> > +{
> > +	struct file_priv *fpriv = file->private_data;
> > +	struct tpms_priv *priv = container_of(fpriv, struct
> > tpms_priv, priv);
> > +
> > +	return tpm_common_write(file, buf, size, off, &priv
> > ->space);
> > +}
> > +
> > +const struct file_operations tpms_fops = {
> > +	.owner = THIS_MODULE,
> > +	.llseek = no_llseek,
> > +	.open = tpms_open,
> > +	.read = tpm_common_read,
> > +	.write = tpms_write,
> > +	.release = tpms_release,
> > +};
> > +
> > 
>