From: James Bottomley <jejb@linux.vnet.ibm.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-security-module@vger.kernel.org,
tpmdd-devel@lists.sourceforge.net,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager
Date: Mon, 02 Jan 2017 21:26:58 -0800 [thread overview]
Message-ID: <1483421218.19261.4.camel@linux.vnet.ibm.com> (raw)
In-Reply-To: <1483393248.2458.32.camel@HansenPartnership.com>
On Mon, 2017-01-02 at 13:40 -0800, James Bottomley wrote:
> On Mon, 2017-01-02 at 21:33 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 02, 2017 at 08:36:20AM -0800, James Bottomley wrote:
> > > On Mon, 2017-01-02 at 15:22 +0200, Jarkko Sakkinen wrote:
> > > > This patch set adds support for TPM spaces that provide a
> > > > context for isolating and swapping transient objects. This
> > > > patch set does not yet include support for isolating policy and
> > > > HMAC sessions but it is trivial to add once the basic approach
> > > > is settled (and that's why I created an RFC patch set).
> > >
> > > The approach looks fine to me. The only basic query I have is
> > > about the default: shouldn't it be with resource manager on
> > > rather than off? I can't really think of a use case that wants
> > > the RM off (even if you're running your own, having another
> > > doesn't hurt anything, and it's still required to share with in
> > > -kernel uses).
> >
> > This is a valid question and here's a longish explanation.
> >
> > In TPM2_GetCapability and maybe couple of other commands you can
> > get handles in the response body. I do not want to have special
> > cases in the kernel for response bodies because there is no a
> > generic way to do the substitution. What's worse, new commands in
> > the standard future revisions could have such commands requiring
> > special cases. In addition, vendor specific commans could have
> > handles in the response bodies.
>
> OK, in general I buy this ... what you're effectively saying is that
> we need a non-RM interface for certain management type commands.
>
> However, let me expand a bit on why I'm fretting about the non-RM use
> case. Right at the moment, we have a single TPM device which you use
> for access to the kernel TPM. The current tss2 just makes direct use
> of this, meaning it has to have 0666 permissions. This means that
> any local user can simply DoS the TPM by running us out of transient
> resources if they don't activate the RM. If they get a connection
> always via the RM, this isn't a worry. Perhaps the best way of
> fixing this is to expose two separate device nodes: one raw to the
> TPM which we could keep at 0600 and one with an always RM connection
> which we can set to 0666. That would mean that access to the non-RM
> connection is either root only or governed by a system set ACL.
OK, so I put a patch together that does this (see below). It all works
nicely (with a udev script that sets the resource manager device to
0666):
jejb@jarvis:~> ls -l /dev/tpm*
crw------- 1 root root 10, 224 Jan 2 20:54 /dev/tpm0
crw-rw-rw- 1 root root 246, 65536 Jan 2 20:54 /dev/tpm0rm
I've modified the tss to connect to /dev/tpm0rm by default and it all
seems to work.
The patch applies on top of your tabrm branch, by the way.
James
---
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ac4c05f..25b8d30 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -33,6 +33,7 @@ DEFINE_IDR(dev_nums_idr);
static DEFINE_MUTEX(idr_lock);
struct class *tpm_class;
+struct class *tpm_rm_class;
dev_t tpm_devt;
/**
@@ -169,27 +170,39 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
chip->dev_num = rc;
device_initialize(&chip->dev);
+ device_initialize(&chip->devrm);
chip->dev.class = tpm_class;
chip->dev.release = tpm_dev_release;
chip->dev.parent = pdev;
chip->dev.groups = chip->groups;
+ chip->devrm.parent = pdev;
+ chip->devrm.class = tpm_rm_class;
+
if (chip->dev_num == 0)
chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
else
chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
+ chip->devrm.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num + TPM_NUM_DEVICES);
+
rc = dev_set_name(&chip->dev, "tpm%d", chip->dev_num);
if (rc)
goto out;
+ rc = dev_set_name(&chip->devrm, "tpm%drm", chip->dev_num);
+ if (rc)
+ goto out;
if (!pdev)
chip->flags |= TPM_CHIP_FLAG_VIRTUAL;
cdev_init(&chip->cdev, &tpm_fops);
+ cdev_init(&chip->cdevrm, &tpm_rm_fops);
chip->cdev.owner = THIS_MODULE;
+ chip->cdevrm.owner = THIS_MODULE;
chip->cdev.kobj.parent = &chip->dev.kobj;
+ chip->cdevrm.kobj.parent = &chip->devrm.kobj;
chip->tr_buf.data = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
if (!chip->tr_buf.data) {
@@ -208,6 +221,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
out:
put_device(&chip->dev);
+ put_device(&chip->devrm);
return ERR_PTR(rc);
}
EXPORT_SYMBOL_GPL(tpm_chip_alloc);
@@ -252,7 +266,7 @@ static int tpm_add_char_device(struct tpm_chip *chip)
dev_name(&chip->dev), MAJOR(chip->dev.devt),
MINOR(chip->dev.devt), rc);
- return rc;
+ goto err_1;
}
rc = device_add(&chip->dev);
@@ -262,16 +276,44 @@ static int tpm_add_char_device(struct tpm_chip *chip)
dev_name(&chip->dev), MAJOR(chip->dev.devt),
MINOR(chip->dev.devt), rc);
- cdev_del(&chip->cdev);
- return rc;
+ goto err_2;
+ }
+
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ rc = cdev_add(&chip->cdevrm, chip->devrm.devt, 1);
+ if (rc) {
+ dev_err(&chip->dev,
+ "unable to cdev_add() %s, major %d, minor %d, err=%d\n",
+ dev_name(&chip->devrm), MAJOR(chip->devrm.devt),
+ MINOR(chip->devrm.devt), rc);
+
+ goto err_3;
}
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ rc = device_add(&chip->devrm);
+ if (rc) {
+ dev_err(&chip->dev,
+ "unable to device_register() %s, major %d, minor %d, err=%d\n",
+ dev_name(&chip->devrm), MAJOR(chip->devrm.devt),
+ MINOR(chip->devrm.devt), rc);
+
+ goto err_4;
+ }
/* Make the chip available. */
mutex_lock(&idr_lock);
idr_replace(&dev_nums_idr, chip, chip->dev_num);
mutex_unlock(&idr_lock);
return rc;
+ err_4:
+ cdev_del(&chip->cdevrm);
+ err_3:
+ device_del(&chip->dev);
+ err_2:
+ cdev_del(&chip->cdev);
+ err_1:
+ return rc;
}
static void tpm_del_char_device(struct tpm_chip *chip)
@@ -279,6 +321,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)
cdev_del(&chip->cdev);
device_del(&chip->dev);
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ cdev_del(&chip->cdevrm);
+ device_del(&chip->devrm);
+ }
+
/* Make the chip unavailable. */
mutex_lock(&idr_lock);
idr_replace(&dev_nums_idr, NULL, chip->dev_num);
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 139638b..bed29f9 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -54,23 +54,28 @@ static void timeout_work(struct work_struct *work)
mutex_unlock(&priv->buffer_mutex);
}
-static int tpm_open(struct inode *inode, struct file *file)
+static int tpm_open_internal(struct inode *inode, struct file *file, bool is_rm)
{
- struct tpm_chip *chip =
- container_of(inode->i_cdev, struct tpm_chip, cdev);
+ struct tpm_chip *chip;
struct file_priv *priv;
+ if (is_rm)
+ chip = container_of(inode->i_cdev, struct tpm_chip, cdevrm);
+ else
+ chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
+
/* It's assured that the chip will be opened just once,
* by the check of is_open variable, which is protected
* by driver_lock. */
- if (test_and_set_bit(0, &chip->is_open)) {
+ if (!is_rm && test_and_set_bit(0, &chip->is_open)) {
dev_dbg(&chip->dev, "Another process owns this TPM\n");
return -EBUSY;
}
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (priv == NULL) {
- clear_bit(0, &chip->is_open);
+ if (!is_rm)
+ clear_bit(0, &chip->is_open);
return -ENOMEM;
}
@@ -82,9 +87,27 @@ static int tpm_open(struct inode *inode, struct file *file)
INIT_WORK(&priv->work, timeout_work);
file->private_data = priv;
+
+ if (is_rm) {
+ priv->space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!priv->space.context_buf)
+ return -ENOMEM;
+ priv->has_space = true;
+ }
+
return 0;
}
+static int tpm_open(struct inode *inode, struct file *file)
+{
+ return tpm_open_internal(inode, file, false);
+}
+
+static int tpm_rm_open(struct inode *inode, struct file *file)
+{
+ return tpm_open_internal(inode, file, true);
+}
+
static ssize_t tpm_read(struct file *file, char __user *buf,
size_t size, loff_t *off)
{
@@ -169,65 +192,6 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
return in_size;
}
-/**
- * tpm_ioc_new_space - handler for %SGX_IOC_NEW_SPACE ioctl
- *
- * Creates a new TPM space that can hold a set of transient objects. The space
- * is isolated with virtual handles that are mapped into physical handles by the
- * driver.
- */
-static long tpm_ioc_new_space(struct file *file, unsigned int ioctl,
- unsigned long arg)
-{
- struct file_priv *priv = file->private_data;
- struct tpm_chip *chip = priv->chip;
- int rc = 0;
-
- if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
- return -EOPNOTSUPP;
-
- mutex_lock(&priv->buffer_mutex);
-
- if (priv->has_space) {
- rc = -EBUSY;
- goto out;
- }
-
- priv->space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
- if (!priv->space.context_buf) {
- rc = -ENOMEM;
- goto out;
- }
-
- /* The TPM device can be opened again as this file has been moved to a
- * TPM handle space.
- */
- priv->has_space = true;
- clear_bit(0, &chip->is_open);
-out:
- mutex_unlock(&priv->buffer_mutex);
- return rc;
-}
-
-static long tpm_ioctl(struct file *file, unsigned int ioctl,
- unsigned long arg)
-{
- switch (ioctl) {
- case TPM_IOC_NEW_SPACE:
- return tpm_ioc_new_space(file, ioctl, arg);
- default:
- return -ENOIOCTLCMD;
- }
-}
-
-#ifdef CONFIG_COMPAT
-static long tpm_compat_ioctl(struct file *file, unsigned int ioctl,
- unsigned long arg)
-{
- return tpm_ioctl(file, ioctl, arg);
-}
-#endif
-
/*
* Called on file close
*/
@@ -247,7 +211,8 @@ static int tpm_release(struct inode *inode, struct file *file)
flush_work(&priv->work);
file->private_data = NULL;
atomic_set(&priv->data_pending, 0);
- clear_bit(0, &priv->chip->is_open);
+ if (!priv->has_space)
+ clear_bit(0, &priv->chip->is_open);
kfree(priv);
return 0;
}
@@ -258,10 +223,15 @@ const struct file_operations tpm_fops = {
.open = tpm_open,
.read = tpm_read,
.write = tpm_write,
- .unlocked_ioctl = tpm_ioctl,
-#ifdef CONFIG_COMPAT
- .compat_ioctl = tpm_compat_ioctl,
-#endif
+ .release = tpm_release,
+};
+
+const struct file_operations tpm_rm_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .open = tpm_rm_open,
+ .read = tpm_read,
+ .write = tpm_write,
.release = tpm_release,
};
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index a1ae57e..c1829de 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1194,9 +1194,17 @@ static int __init tpm_init(void)
return PTR_ERR(tpm_class);
}
- rc = alloc_chrdev_region(&tpm_devt, 0, TPM_NUM_DEVICES, "tpm");
+ tpm_rm_class = class_create(THIS_MODULE, "tpmrm");
+ if (IS_ERR(tpm_rm_class)) {
+ pr_err("couldn't create tpmrm class\n");
+ class_destroy(tpm_class);
+ return PTR_ERR(tpm_rm_class);
+ }
+
+ rc = alloc_chrdev_region(&tpm_devt, 0, 2*TPM_NUM_DEVICES, "tpm");
if (rc < 0) {
pr_err("tpm: failed to allocate char dev region\n");
+ class_destroy(tpm_rm_class);
class_destroy(tpm_class);
return rc;
}
@@ -1208,7 +1216,8 @@ static void __exit tpm_exit(void)
{
idr_destroy(&dev_nums_idr);
class_destroy(tpm_class);
- unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES);
+ class_destroy(tpm_rm_class);
+ unregister_chrdev_region(tpm_devt, 2*TPM_NUM_DEVICES);
}
subsys_initcall(tpm_init);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index c6171e5..890fb6b 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,8 +186,8 @@ struct tpm_buf {
};
struct tpm_chip {
- struct device dev;
- struct cdev cdev;
+ struct device dev, devrm;
+ struct cdev cdev, cdevrm;
/* A driver callback under ops cannot be run unless ops_sem is held
* (sometimes implicitly, eg for the sysfs code). ops becomes null
@@ -493,8 +493,10 @@ static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
}
extern struct class *tpm_class;
+extern struct class *tpm_rm_class;
extern dev_t tpm_devt;
extern const struct file_operations tpm_fops;
+extern const struct file_operations tpm_rm_fops;
extern struct idr dev_nums_idr;
enum tpm_transmit_flags {
next prev parent reply other threads:[~2017-01-03 5:27 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-02 13:22 [PATCH RFC 0/4] RFC: in-kernel resource manager Jarkko Sakkinen
2017-01-02 13:22 ` [PATCH RFC 1/4] tpm: migrate struct tpm_buf to struct tpm_chip Jarkko Sakkinen
2017-01-02 21:01 ` Jason Gunthorpe
2017-01-03 0:57 ` Jarkko Sakkinen
2017-01-03 19:13 ` Jason Gunthorpe
2017-01-04 12:29 ` Jarkko Sakkinen
2017-01-02 13:22 ` [PATCH RFC 2/4] tpm: validate TPM 2.0 commands Jarkko Sakkinen
[not found] ` <OF8D508BD2.EAB22BFD-ON0025809E.0062B40C-8525809E.006356C3@notes.na.collabserv.com>
2017-01-04 18:19 ` [tpmdd-devel] " James Bottomley
2017-01-04 18:44 ` Jason Gunthorpe
2017-01-02 13:22 ` [PATCH RFC 3/4] tpm: export tpm2_flush_context_cmd Jarkko Sakkinen
2017-01-02 13:22 ` [PATCH RFC 4/4] tpm: add the infrastructure for TPM space for TPM 2.0 Jarkko Sakkinen
2017-01-02 21:09 ` Jason Gunthorpe
2017-01-03 0:37 ` Jarkko Sakkinen
2017-01-03 18:46 ` Jason Gunthorpe
2017-01-04 12:43 ` Jarkko Sakkinen
2017-01-03 19:16 ` Jason Gunthorpe
2017-01-04 12:45 ` Jarkko Sakkinen
[not found] ` <OF9C3EE9AE.65978870-ON0025809E.0061E7AF-8525809E.0061FFDA@notes.na.collabserv.com>
2017-01-09 22:11 ` [tpmdd-devel] " Jarkko Sakkinen
2017-01-02 16:36 ` [tpmdd-devel] [PATCH RFC 0/4] RFC: in-kernel resource manager James Bottomley
2017-01-02 19:33 ` Jarkko Sakkinen
2017-01-02 21:40 ` James Bottomley
2017-01-03 5:26 ` James Bottomley [this message]
2017-01-03 13:41 ` Jarkko Sakkinen
2017-01-03 16:14 ` James Bottomley
2017-01-03 18:36 ` Jarkko Sakkinen
2017-01-03 19:14 ` Jarkko Sakkinen
2017-01-03 19:34 ` James Bottomley
2017-01-03 21:54 ` Jason Gunthorpe
2017-01-04 12:58 ` Jarkko Sakkinen
2017-01-04 16:55 ` Jason Gunthorpe
2017-01-04 5:47 ` Andy Lutomirski
2017-01-04 13:00 ` Jarkko Sakkinen
2017-01-03 13:51 ` Jarkko Sakkinen
2017-01-03 16:36 ` James Bottomley
2017-01-03 18:40 ` Jarkko Sakkinen
2017-01-03 21:47 ` Jason Gunthorpe
2017-01-03 22:21 ` Ken Goldman
2017-01-03 23:20 ` Jason Gunthorpe
2017-01-03 22:39 ` James Bottomley
2017-01-04 0:17 ` Jason Gunthorpe
2017-01-04 0:29 ` James Bottomley
2017-01-04 0:56 ` Jason Gunthorpe
2017-01-04 12:50 ` Jarkko Sakkinen
2017-01-04 14:53 ` James Bottomley
2017-01-04 18:31 ` Jason Gunthorpe
2017-01-04 18:57 ` James Bottomley
2017-01-04 19:24 ` Jason Gunthorpe
2017-01-04 12:48 ` Jarkko Sakkinen
2017-01-03 21:32 ` Jason Gunthorpe
2017-01-03 22:03 ` James Bottomley
2017-01-05 15:52 ` Fuchs, Andreas
2017-01-05 17:27 ` Jason Gunthorpe
2017-01-05 18:06 ` James Bottomley
2017-01-06 8:43 ` Andreas Fuchs
2017-01-05 18:33 ` James Bottomley
2017-01-05 19:20 ` Jason Gunthorpe
2017-01-05 19:55 ` James Bottomley
2017-01-05 22:21 ` Jason Gunthorpe
2017-01-05 22:58 ` James Bottomley
2017-01-05 23:50 ` Jason Gunthorpe
2017-01-06 0:36 ` James Bottomley
2017-01-06 8:59 ` Andreas Fuchs
2017-01-06 19:10 ` Jason Gunthorpe
2017-01-06 19:02 ` Jason Gunthorpe
2017-01-10 19:03 ` Ken Goldman
2017-01-09 22:39 ` [tpmdd-devel] " Jarkko Sakkinen
2017-01-11 10:03 ` Andreas Fuchs
2017-01-04 16:12 Dr. Greg Wettstein
2017-01-09 23:16 ` Jarkko Sakkinen
2017-01-10 19:29 ` Ken Goldman
2017-01-11 11:36 ` Jarkko Sakkinen
2017-01-10 20:05 ` Jason Gunthorpe
2017-01-11 10:00 ` Andreas Fuchs
2017-01-11 18:03 ` Jason Gunthorpe
2017-01-11 18:27 ` Stefan Berger
2017-01-11 19:18 ` Jason Gunthorpe
2017-01-11 11:34 ` Jarkko Sakkinen
2017-01-11 15:39 ` James Bottomley
2017-01-11 17:56 ` Jason Gunthorpe
2017-01-11 18:25 ` James Bottomley
2017-01-11 19:04 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1483421218.19261.4.camel@linux.vnet.ibm.com \
--to=jejb@linux.vnet.ibm.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=tpmdd-devel@lists.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).