| Message ID | 20170216192529.25467-7-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 16, 2017 at 09:25:19PM +0200, Jarkko Sakkinen wrote: > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > 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. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> Nitpicking but I've been thinking about naming. What about calling the device as tpmrc0 as in resource context. I think that would be a better name than TPM space. You do not mix it up with namespaces and/or virtualization. With resource in front it cannot be easily mixed up with TPM contexts either. This does not require any effort from your side. I could do the renaming. PS. Could you go through my commits and test and review them at some point so we would have the whole patch set peer tested? /Jarkko > --- > 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@HansenPartnership.com > + * > + * 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, > +}; > + > -- > 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/17/2017 12:55 AM, Jarkko Sakkinen wrote: > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > 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 ? 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. Thanks & Regards, - Nayna > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > --- > 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@HansenPartnership.com > + * > + * 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, > +}; > + > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@HansenPartnership.com> > > > > 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@HansenPartnership.com> > > --- > > 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@HansenPartnership.com > > + * > > + * 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, > > +}; > > + > > > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-02-23 at 11:09 +0200, Jarkko Sakkinen wrote: > On Thu, Feb 16, 2017 at 09:25:19PM +0200, Jarkko Sakkinen wrote: > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > 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. > > Signed-off-by: James Bottomley < > > > > James.Bottomley@HansenPartnershp.com> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> > Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> Thanks! > Nitpicking but I've been thinking about naming. What about calling > the device as tpmrc0 as in resource context. I think that would be a > better name than TPM space. Well the original name was tpmrm<n> for TPM with Resource Manager. You wanted it to be tpms<n> for TPM with Spaces. I'm not entirely sold on the Resource Context name ... I think Resource Manager (because it's what the TCG calls it) or Spaces (because it's what all the code comments call it) are better. Resource Context sounds like what TPM2_SaveContext() creates for you rather than the interface. > You do not mix it up with namespaces and/or virtualization. With > resource in front it cannot be easily mixed up with TPM contexts > either. I'm a containers person. What this set of patches does is precisely OS level virtualization in my book, so I don't think you need to pretend it is't; and OS level virtualization is what a namespace does. The only difference between this and the other kernel namespaces is that you get a new namespace automatically when you open the device and you can't enter an existing namespace. I think therefore that tpmns<n> for TPM Namespace would be very appropriate. > This does not require any effort from your side. I could do the > renaming. > > PS. Could you go through my commits and test and review them at some > point so we would have the whole patch set peer tested? Already reviewed, just doing a test build (I'm travelling, so it actually has to be on my physical laptop). James -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 24, 2017 at 08:02:08AM -0500, James Bottomley wrote: > On Thu, 2017-02-23 at 11:09 +0200, Jarkko Sakkinen wrote: > > On Thu, Feb 16, 2017 at 09:25:19PM +0200, Jarkko Sakkinen wrote: > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > 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. > > > Signed-off-by: James Bottomley < > > > > > > James.Bottomley@HansenPartnershp.com> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> > > Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> > > Thanks! > > > Nitpicking but I've been thinking about naming. What about calling > > the device as tpmrc0 as in resource context. I think that would be a > > better name than TPM space. > > Well the original name was tpmrm<n> for TPM with Resource Manager. You > wanted it to be tpms<n> for TPM with Spaces. > > I'm not entirely sold on the Resource Context name ... I think Resource Manager (because it's what the TCG calls it) or Spaces (because it's what all the code comments call it) are better. Resource Context sounds like what TPM2_SaveContext() creates for you rather than the interface. > > > You do not mix it up with namespaces and/or virtualization. With > > resource in front it cannot be easily mixed up with TPM contexts > > either. > > I'm a containers person. What this set of patches does is precisely OS > level virtualization in my book, so I don't think you need to pretend > it is't; and OS level virtualization is what a namespace does. The > only difference between this and the other kernel namespaces is that > you get a new namespace automatically when you open the device and you > can't enter an existing namespace. > > I think therefore that tpmns<n> for TPM Namespace would be very > appropriate. Makes sense. We can go with tpmns. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 24, 2017 at 07:39:22PM +0200, Jarkko Sakkinen wrote: > > I think therefore that tpmns<n> for TPM Namespace would be very > > appropriate. > > Makes sense. We can go with tpmns. When we have talked about TPM namespaces in the past it has been around the idea of restricting which TPMs the namespace has access too and changing the 'kernel tpm' for that namespace. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-02-24 at 11:11 -0700, Jason Gunthorpe wrote: > On Fri, Feb 24, 2017 at 07:39:22PM +0200, Jarkko Sakkinen wrote: > > > > I think therefore that tpmns<n> for TPM Namespace would be very > > > appropriate. > > > > Makes sense. We can go with tpmns. > > When we have talked about TPM namespaces in the past it has been > around the idea of restricting which TPMs the namespace has access > too and changing the 'kernel tpm' for that namespace. Well, you know, nothing in the TPM Space code prevents us from exposing the namespace so that it could be shared. However, I think the namespace follows connect (device open) paradigm is pretty much the behaviour everyone (including the kernel) wants, mostly because TPM2 has such a tiny amount of resources that you're always dealing with loadable keys meaning you don't really want to see anyone else's volatile state. James -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 24, 2017 at 03:29:15PM -0500, James Bottomley wrote: > On Fri, 2017-02-24 at 11:11 -0700, Jason Gunthorpe wrote: > > On Fri, Feb 24, 2017 at 07:39:22PM +0200, Jarkko Sakkinen wrote: > > > > > > I think therefore that tpmns<n> for TPM Namespace would be very > > > > appropriate. > > > > > > Makes sense. We can go with tpmns. > > > > When we have talked about TPM namespaces in the past it has been > > around the idea of restricting which TPMs the namespace has access > > too and changing the 'kernel tpm' for that namespace. > > Well, you know, nothing in the TPM Space code prevents us from exposing > the namespace so that it could be shared. However, I think the > namespace follows connect (device open) paradigm is pretty much the > behaviour everyone (including the kernel) wants, mostly because TPM2 > has such a tiny amount of resources that you're always dealing with > loadable keys meaning you don't really want to see anyone else's > volatile state. I'm not arguing with that use model, I am asking what do you want to call the future feature that restricts which TPMs a process can view if you want to use the word namespace for the resource manager? This is something Stephen B has been exploring in conjunction with vtpm. (eg restrict a container to only use a single vtpm and ban it from the system tpm) Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-02-24 at 13:52 -0700, Jason Gunthorpe wrote: > On Fri, Feb 24, 2017 at 03:29:15PM -0500, James Bottomley wrote: > > On Fri, 2017-02-24 at 11:11 -0700, Jason Gunthorpe wrote: > > > On Fri, Feb 24, 2017 at 07:39:22PM +0200, Jarkko Sakkinen wrote: > > > > > > > > I think therefore that tpmns<n> for TPM Namespace would be > > > > > very > > > > > appropriate. > > > > > > > > Makes sense. We can go with tpmns. > > > > > > When we have talked about TPM namespaces in the past it has been > > > around the idea of restricting which TPMs the namespace has > > > access > > > too and changing the 'kernel tpm' for that namespace. > > > > Well, you know, nothing in the TPM Space code prevents us from > > exposing > > the namespace so that it could be shared. However, I think the > > namespace follows connect (device open) paradigm is pretty much the > > behaviour everyone (including the kernel) wants, mostly because > > TPM2 > > has such a tiny amount of resources that you're always dealing with > > loadable keys meaning you don't really want to see anyone else's > > volatile state. > > I'm not arguing with that use model, I am asking what do you want to > call the future feature that restricts which TPMs a process can view > if you want to use the word namespace for the resource manager? Well, as a glib answer, I'd say the TPM is a device, so the thing which restricts device access to containers is the device cgroup ... that's what we should be plugging into. I'd have to look, but I suspect the device cgroup basically operates on device node appearance, so it should "just work"(tm). I can explore when I'm back home. James > This is something Stephen B has been exploring in conjunction with > vtpm. (eg restrict a container to only use a single vtpm and ban it > from the system tpm) > > Jason > > --------------------------------------------------------------------- > --------- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 24, 2017 at 06:01:00PM -0500, James Bottomley wrote: > Well, as a glib answer, I'd say the TPM is a device, so the thing which > restricts device access to containers is the device cgroup ... that's > what we should be plugging into. I'd have to look, but I suspect the > device cgroup basically operates on device node appearance, so it > should "just work"(tm). I can explore when I'm back home. Seems reasonable.. It just seems confusing to call something a namespace that isn't also a CLONE_NEW* option.. FWIW more background on the topic: Stefan was concerned about information leakage via sysfs of TPM data, eg that a container could still touch the host's TPM. I wonder if device cgroup could be extended to block access to the sysfs directories containing a disallowed 'dev' ? I was also wondering about kernel use from within the container - all kernel consumers are locked to physical tpm0.. But maybe the kernel can consult the right device cgroup to find an allowed TPM? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-02-24 at 16:23 -0700, Jason Gunthorpe wrote: > On Fri, Feb 24, 2017 at 06:01:00PM -0500, James Bottomley wrote: > > > Well, as a glib answer, I'd say the TPM is a device, so the thing > > which restricts device access to containers is the device cgroup > > ... that's what we should be plugging into. I'd have to look, but > > I suspect the device cgroup basically operates on device node > > appearance, so it should "just work"(tm). I can explore when I'm > > back home. > > Seems reasonable.. > > It just seems confusing to call something a namespace that isn't also > a CLONE_NEW* option.. Well, there's namespace behaviour and then there's how you enter them. We have namespace behaviour with the /dev/tpms<n> but the namespace is entered on opening the device, even if the same process opens the device more than once. So we have namespace behaviour with a non clone entry mechanism. Since we're namespaceing a device, that seems to me to be the correct semantic. > FWIW more background on the topic: > > Stefan was concerned about information leakage via sysfs of TPM data, > eg that a container could still touch the host's TPM. I wonder if > device cgroup could be extended to block access to the sysfs > directories containing a disallowed 'dev' ? It doesn't need to. The sysfs entries (those that ask the TPM something) are surrounded by chip->tpm_mutex, so when it asks, we know all the spaces are context saved (i.e. the only TPM visible state is global not anything space local). > I was also wondering about kernel use from within the container - > all kernel consumers are locked to physical tpm0.. But maybe the > kernel can consult the right device cgroup to find an allowed TPM? I'd use the device cgroup to determine what's allowable per container (i.e. what tpm you can see) then within the container I'd open the tpms<n> device ... because the TPM volatile storage is so tiny its not inconceivable that multiple processes, even within a single container, need access ... and they'd each need their own "namespace" (which they get with the current model). However, this is opinion ... we should try it out and see what works best. James -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 24, 2017 at 06:43:27PM -0500, James Bottomley wrote: > > It just seems confusing to call something a namespace that isn't also > > a CLONE_NEW* option.. > > Well, there's namespace behaviour and then there's how you enter them. > We have namespace behaviour with the /dev/tpms<n> but the namespace is > entered on opening the device, even if the same process opens the > device more than once. So we have namespace behaviour with a non clone > entry mechanism. Since we're namespaceing a device, that seems to me > to be the correct semantic. I'm looking at it from a documentation perspective, look at namespaces(7) for instance Lots of FD things have 'namespace behavior' but we don't call them namespaces.. > > Stefan was concerned about information leakage via sysfs of TPM data, > > eg that a container could still touch the host's TPM. I wonder if > > device cgroup could be extended to block access to the sysfs > > directories containing a disallowed 'dev' ? > > It doesn't need to. The sysfs entries (those that ask the TPM > something) are surrounded by chip->tpm_mutex, so when it asks, we know > all the spaces are context saved (i.e. the only TPM visible state is > global not anything space local). Yes, I understand that - the concern is that a container can still read the global state from tpm0 (eg ek/srk/pcrs) even if it is setup to exclusively use a vtpm. device cgroup blocks access to the cdevs of tpm0 but not to the sysfs files. Maybe we should just make those debug files readable only by root and forget about that worry. > > I was also wondering about kernel use from within the container - > > all kernel consumers are locked to physical tpm0.. But maybe the > > kernel can consult the right device cgroup to find an allowed TPM? > > I'd use the device cgroup to determine what's allowable per container > (i.e. what tpm you can see) then within the container I'd open the > tpms<n> device ... I am talking about using a situation like kernel IMA or keyring in the container with a tpm that is not tpm0, eg a vtpm. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-02-24 at 17:25 -0700, Jason Gunthorpe wrote: > On Fri, Feb 24, 2017 at 06:43:27PM -0500, James Bottomley wrote: > > > > It just seems confusing to call something a namespace that isn't > > > also a CLONE_NEW* option.. > > > > Well, there's namespace behaviour and then there's how you enter > > them. We have namespace behaviour with the /dev/tpms<n> but the > > namespace is entered on opening the device, even if the same > > process opens the device more than once. So we have namespace > > behaviour with a non clone entry mechanism. Since we're > > namespaceing a device, that seems to me to be the correct semantic. > > I'm looking at it from a documentation perspective, look at > namespaces(7) for instance The term "namespace" is way broader than that https://en.wikipedia.org/wiki/Namespace > Lots of FD things have 'namespace behavior' but we don't call > them namespaces.. At it's simplest, the virtual memory process model of UNIX is a namespace. That doesn't make the term inapplicable in this case. > > > Stefan was concerned about information leakage via sysfs of TPM > > > data, eg that a container could still touch the host's TPM. I > > > wonder if device cgroup could be extended to block access to the > > > sysfs directories containing a disallowed 'dev' ? > > > > It doesn't need to. The sysfs entries (those that ask the TPM > > something) are surrounded by chip->tpm_mutex, so when it asks, we > > know all the spaces are context saved (i.e. the only TPM visible > > state is global not anything space local). > > Yes, I understand that - the concern is that a container can still > read the global state from tpm0 (eg ek/srk/pcrs) even if it is setup > to exclusively use a vtpm. The TPM2 namespace as laid out by these patches only applies to objects of type 80, 02 and 03. The Storage and Endorsement seeds can't be virtualized because they're one per physical instance. 81 objects could be virtualised, but I don't really think we should. PCR virtualisation is another whole interesting area of study. > device cgroup blocks access to the cdevs of tpm0 but not to the > sysfs files. What the device cgroup currently does for us and what it could do are two different things. It seems if it exported __devcgroup_check_permission, we could use that as a check to gate the sysfs file access. > Maybe we should just make those debug files readable only by root and > forget about that worry. > > > > I was also wondering about kernel use from within the container - > > > all kernel consumers are locked to physical tpm0.. But maybe the > > > kernel can consult the right device cgroup to find an allowed > > > TPM? > > > > I'd use the device cgroup to determine what's allowable per > > container (i.e. what tpm you can see) then within the container I'd > > open the tpms<n> device ... > > I am talking about using a situation like kernel IMA or keyring in > the container with a tpm that is not tpm0, eg a vtpm. a vtpm appears as a tpm device so it can be controlled by the device cgroup ... I think I'm not seeing the issue. I should also say that discussion of mechanisms is usually the wrong way to begin for OS virtualisation. Almost anything can be virtualised in a variety of ways, so to find the best way (or indeed if it should be done at all) it's usually better to start with use cases. So instead of saying we need to virtualize the PCRs we should start with X container has this requirement for attestation of its Y state. Often the best way simply is an extension of the multi user model for the resource ... in this case no-one's really come up with one for PCRs, so that might be the place to begin. James -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 24, 2017 at 08:02:08AM -0500, James Bottomley wrote: > On Thu, 2017-02-23 at 11:09 +0200, Jarkko Sakkinen wrote: > > On Thu, Feb 16, 2017 at 09:25:19PM +0200, Jarkko Sakkinen wrote: > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > 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. > > > Signed-off-by: James Bottomley < > > > > > > James.Bottomley@HansenPartnershp.com> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> > > Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> > > Thanks! > > > Nitpicking but I've been thinking about naming. What about calling > > the device as tpmrc0 as in resource context. I think that would be a > > better name than TPM space. > > Well the original name was tpmrm<n> for TPM with Resource Manager. You > wanted it to be tpms<n> for TPM with Spaces. > > I'm not entirely sold on the Resource Context name ... I think > Resource Manager (because it's what the TCG calls it) or Spaces > (because it's what all the code comments call it) are better. > Resource Context sounds like what TPM2_SaveContext() creates for you > rather than the interface. > > > You do not mix it up with namespaces and/or virtualization. With > > resource in front it cannot be easily mixed up with TPM contexts > > either. > > I'm a containers person. What this set of patches does is precisely OS > level virtualization in my book, so I don't think you need to pretend > it is't; and OS level virtualization is what a namespace does. The > only difference between this and the other kernel namespaces is that > you get a new namespace automatically when you open the device and you > can't enter an existing namespace. > > I think therefore that tpmns<n> for TPM Namespace would be very > appropriate. Sorry for going back and forth with this but I turn it back to your original tpmrm. It's in the end of the day the least confusing option. I think this has been anyway useful to trip a bit around the options because it is hard to rollback API... > > This does not equire any effort from your side. I could do the > > renaming. > > > > PS. Could you go through my commits and test and review them at some > > point so we would have the whole patch set peer tested? > > Already reviewed, just doing a test build (I'm travelling, so it > actually has to be on my physical laptop). > > James There's now tabrm-v3 branch. I had to tweak error handling in your device adding patch because of b4e9d7561a70. I hope I didn't break anything. /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 26, 2017 at 01:44:40PM +0200, Jarkko Sakkinen wrote: Good day, I hope this note finds the week starting well for everyone. > On Fri, Feb 24, 2017 at 08:02:08AM -0500, James Bottomley wrote: > > On Thu, 2017-02-23 at 11:09 +0200, Jarkko Sakkinen wrote: > > > On Thu, Feb 16, 2017 at 09:25:19PM +0200, Jarkko Sakkinen wrote: > > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > > > 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. > > > > Signed-off-by: James Bottomley < > > > > > > > > James.Bottomley@HansenPartnershp.com> > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> > > > Tested-by: Jarkko Sakkinen <jarkko.sakkine@linux.intel.com> > > > > Thanks! > > > > > Nitpicking but I've been thinking about naming. What about calling > > > the device as tpmrc0 as in resource context. I think that would be a > > > better name than TPM space. > > > > Well the original name was tpmrm<n> for TPM with Resource Manager. You > > wanted it to be tpms<n> for TPM with Spaces. > > > > I'm not entirely sold on the Resource Context name ... I think > > Resource Manager (because it's what the TCG calls it) or Spaces > > (because it's what all the code comments call it) are better. > > Resource Context sounds like what TPM2_SaveContext() creates for you > > rather than the interface. > > > > > You do not mix it up with namespaces and/or virtualization. With > > > resource in front it cannot be easily mixed up with TPM contexts > > > either. > > > > I'm a containers person. What this set of patches does is precisely OS > > level virtualization in my book, so I don't think you need to pretend > > it is't; and OS level virtualization is what a namespace does. The > > only difference between this and the other kernel namespaces is that > > you get a new namespace automatically when you open the device and you > > can't enter an existing namespace. > > > > I think therefore that tpmns<n> for TPM Namespace would be very > > appropriate. > Sorry for going back and forth with this but I turn it back to your > original tpmrm. It's in the end of the day the least confusing > option. I think this has been anyway useful to trip a bit around the > options because it is hard to rollback API... Indeed, which is why we have watched this conversation with some interest regarding exactly what the tpm{rm,ns} will represent. In order to get a handle on how the resource manager will affect Trusted Execution Technology (TXT) environments we downloaded your branch and experimented with it a bit. Our overall impression is that it will be important to educate the user community on exactly what the 'tpm spaces' management device actually represents. It is not a TPM device as much as it is a method of surfacing a subset of TPM functionality over the lifespan of a file descriptor. There is certainly nothing wrong with this but users and system administrators will need to understand the implications of this and not confuse the two devices. For example, Ken's tools which come in his TSS2 library, don't work properly with the 'spaces' device due to the virtualization lifetime. As an example, the getcapability call will 'lie' about the number of transient handles which are available through the device. Attempts to string multiple transaction sequences together will fail as well. So I think it will be important to stress that the 'spaces' device is something an individual application will use to gain unimpeded access to TPM functionality and not a representation of the device itself. Given the quality of userspace simulators and their flexibility and operational fidelity, I anticipate we will eventually come to the conclusion that this problem will have been best solved by creating and adopting a framework which provides the anchoring and spawning of userspace TPM2 instances. Just as an aside we still haven't been able to verify the inter-operability of the 'spaces' driver with TXT. There are larger TXT/TPM2 operational issues which we are working to run down before we can determine how the two systems will inter-operate or not. > /Jarkko Have a good day. Greg As always, Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC. 4206 N. 19th Ave. Specializing in information infra-structure Fargo, ND 58102 development. PH: 701-281-1686 FAX: 701-281-3949 EMAIL: greg@enjellic.com ------------------------------------------------------------------------------ "You've got to be kidding me Nate. You've seen the shit that has come through my office in the last two hours. You think I'm even remotely worried about one SATA cable being six inches longer than the other." -- Dr. Greg Wettstein Resurrection -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/24/2017 06:23 PM, James Bottomley wrote: > 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@HansenPartnership.com> >>> >>> 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. Thanks James !! Currently, I see even /dev/tpms0 is also only root accessible. I did see the discussion to make it 0666, and I understand adding command filtering is part of enabling /dev/tpms0 as all-accessible. Sorry, I didn't understand when you said, "major indicator will be whether /dev/tpms0" exists or not ? I mean in what case it might not exist.. > >> 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. Does it imply that there should be restriction to disallow any RM specific commands from userspace on /dev/tpm0 ? Thanks & Regards, - Nayna > > James > >> Thanks & Regards, >> - Nayna >> >> >>> >>> Signed-off-by: James Bottomley < >>> James.Bottomley@HansenPartnership.com> >>> --- >>> 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@HansenPartnership.com >>> + * >>> + * 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, >>> +}; >>> + >>> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-02-27 at 17:16 +0530, Nayna wrote: > > On 02/24/2017 06:23 PM, James Bottomley wrote: > > 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@HansenPartnership.com> > > > > > > > > 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. > > Thanks James !! > Currently, I see even /dev/tpms0 is also only root accessible. I did > see the discussion to make it 0666, and I understand adding command > filtering is part of enabling /dev/tpms0 as all-accessible. I don't think we'd ever do that from the kernel. Accessibility would be a userspace policy. This is what I have on my system to make it accessible: /etc/udev/rules.d/80-tpm-2.rules: # tpm 2 devices need to be world readable SUBSYSTEM=="tpms", ACTION=="add", MODE="0666" > Sorry, I didn't understand when you said, "major indicator will be > whether /dev/tpms0" exists or not ? I mean in what case it might not > exist.. If the kernel is too old or you have a 1.2 TPM. > > > > > 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. > > Does it imply that there should be restriction to disallow any RM > specific commands from userspace on /dev/tpm0 ? The only such command would be session or policy context save. It's debateable whether we should interfere. James -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 25, 2017 at 12:04:49PM -0500, James Bottomley wrote: > > device cgroup blocks access to the cdevs of tpm0 but not to the > > sysfs files. > > What the device cgroup currently does for us and what it could do are > two different things. It seems if it exported > __devcgroup_check_permission, we could use that as a check to gate the > sysfs file access. Make sense, maybe we should be doing that.. Stefan, are you still interested in this? This seems like a fairly simple solution to your problem??? > > I am talking about using a situation like kernel IMA or keyring in > > the container with a tpm that is not tpm0, eg a vtpm. > > a vtpm appears as a tpm device so it can be controlled by the device > cgroup ... I think I'm not seeing the issue. When an in-kernel call opens the TPM it does not go through the cdev, it does something like this: extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf); And hardwires 'chip_num' to TPM_ANY_NUM. Keyring does the same (see trusted_instantiate) Practically speaking this means in-kernel callers pretty much always operate on tpm0. I think we need to change TPM_ANY_NUM to something more container friendly, but I'm not sure what that should be. > be done at all) it's usually better to start with use cases. So > instead of saying we need to virtualize the PCRs we should start with X > container has this requirement for attestation of its Y state. Often > the best way simply is an extension of the multi user model for the > resource ... in this case no-one's really come up with one for PCRs, so > that might be the place to begin. Broadly makes sense to me. Maybe kernel keyring is a better example, it already has a multi-user model. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Feb 26, 2017 at 01:44:40PM +0200, Jarkko Sakkinen wrote: > There's now tabrm-v3 branch. I had to tweak error handling in your > device adding patch because of b4e9d7561a70. I hope I didn't break > anything. Just looked, the flow seemed like it works to me. Just confusing that tpm_add_char_device isn't undone by tpm_del_char_device Jason -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/26/2017 1:30 PM, Dr. Greg Wettstein wrote: > > For example, Ken's tools which come in his TSS2 library, don't work > properly with the 'spaces' device due to the virtualization lifetime. > As an example, the getcapability call will 'lie' about the number of > transient handles which are available through the device. Attempts to > string multiple transaction sequences together will fail as well. Two comments: 1 = The intent of the command line tools was for rapid prototyping scripts against a SW TPM, and then as sample code for writing the application. 2 - If you really want to script against a hardware TPM, it can be done. Simply place a proxy between the TSS and the TPM device driver. The proxy passes commands from the TCP socket to the TPM device driver. It keeps the connection open so the resource manager doesn't flush between transactions. The proxy can be obtained from here. It's from TPM 1.2 days, but it works for TPM 2.0 as well. https://sourceforge.net/projects/ibmswtpm/files/?source=navbar -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@HansenPartnership.com + * + * 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, +}; +