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

From: Jarkko Sakkinen
Date: Thu Feb 23 2017 - 04:09:40 EST

Next message: Masami Hiramatsu: "Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it"
Previous message: Jarkko Sakkinen: "Re: [PATCH v2 5/7] tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c"
In reply to: Jarkko Sakkinen: "[PATCH v2 6/7] tpm: expose spaces via a device link /dev/tpms<n>"
Next in thread: Jarkko Sakkinen: "[PATCH v2 4/7] tpm: infrastructure for TPM spaces"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Feb 16, 2017 at 09:25:19PM +0200, 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.
>
> Signed-off-by: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkine@xxxxxxxxxxxxxxx>
Tested-by: Jarkko Sakkinen <jarkko.sakkine@xxxxxxxxxxxxxxx>

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@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,
> +};
> +
> --
> 2.9.3
>

Next message: Masami Hiramatsu: "Re: [PATCH v2 4/5] perf: kretprobes: offset from reloc_sym if kernel supports it"
Previous message: Jarkko Sakkinen: "Re: [PATCH v2 5/7] tpm: split out tpm-dev.c into tpm-dev.c and tpm-common-dev.c"
In reply to: Jarkko Sakkinen: "[PATCH v2 6/7] tpm: expose spaces via a device link /dev/tpms<n>"
Next in thread: Jarkko Sakkinen: "[PATCH v2 4/7] tpm: infrastructure for TPM spaces"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]