| Message ID | 1484335247.2527.28.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 13, 2017 at 11:20:47AM -0800, James Bottomley wrote: > On Thu, 2017-01-12 at 11:39 -0700, Jason Gunthorpe wrote: > > On Thu, Jan 12, 2017 at 07:46:08PM +0200, Jarkko Sakkinen wrote: > > > > > struct tpm_chip { > > > - struct device dev; > > > - struct cdev cdev; > > > + struct device dev, devrm; > > > > Hum.. devrm adds a new kref but doesn't do anything with the release > > function, so that is going to use after free, ie here: > > > > > put_device(&chip->dev); > > > + put_device(&chip->devrm); > > > return ERR_PTR(rc); > > > > And other places. > > > > One solution is to get_device(chip->dev) after > > device_initialize(dev->rm) and add a devrm->dev.release function to > > do put_device(chip->dev) > > Actually, no, the devrm is a completely lifetime managed device as part > of the chip structure. once you've done a device_del on it, it can be > kfreed because it's no longer visible to anything else. No, that isn't enough. Anything else could have obtained a kref on devrm outside of the sphere the device_del manages. For instance, the cdev does exactly that, via this: > chip->cdev.kobj.parent = &chip->dev.kobj; > + chip->cdevrm.kobj.parent = &chip->devrm.kobj; In the worst case the kref the cdev grabs is not released until after tpm_chip_unregister() returns. Having a kref that doesn't work is just asking for trouble, please make it work properly. 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-01-13 at 12:47 -0700, Jason Gunthorpe wrote: > On Fri, Jan 13, 2017 at 11:20:47AM -0800, James Bottomley wrote: > > On Thu, 2017-01-12 at 11:39 -0700, Jason Gunthorpe wrote: > > > On Thu, Jan 12, 2017 at 07:46:08PM +0200, Jarkko Sakkinen wrote: > > > > > > > struct tpm_chip { > > > > - struct device dev; > > > > - struct cdev cdev; > > > > + struct device dev, devrm; > > > > > > Hum.. devrm adds a new kref but doesn't do anything with the > > > release > > > function, so that is going to use after free, ie here: > > > > > > > put_device(&chip->dev); > > > > + put_device(&chip->devrm); > > > > return ERR_PTR(rc); > > > > > > And other places. > > > > > > One solution is to get_device(chip->dev) after > > > device_initialize(dev->rm) and add a devrm->dev.release function > > > to > > > do put_device(chip->dev) > > > > Actually, no, the devrm is a completely lifetime managed device as > > part > > of the chip structure. once you've done a device_del on it, it can > > be > > kfreed because it's no longer visible to anything else. > > No, that isn't enough. Anything else could have obtained a kref on > devrm outside of the sphere the device_del manages. > > For instance, the cdev does exactly that, via this: > > > chip->cdev.kobj.parent = &chip->dev.kobj; > > + chip->cdevrm.kobj.parent = &chip->devrm.kobj; > > In the worst case the kref the cdev grabs is not released until after > tpm_chip_unregister() returns. chip_unregister doesn't tear down either device. It's the final release of the chip->dev that does that. chip->devrm is simply a subordinate in that process, which is why it doesn't need to be separately managed. We have to be careful to call cdev_del() before device_del on devrm, but we do that, so we're guaranteed no visible references by the time the chip->dev release is called. > Having a kref that doesn't work is just asking for trouble, please > make it work properly. Actually, as shown above, these krefs are managed ... However, they're not actually what holds the tpm module in place. The try_module_get on open via the owner field does that. So, by the time tpm_exit() is called we know there are no devrm references simply because we manage the cdevrm entity as well. Now there is a related problem that the owner is actually the *wrong* module: it holds the tpm module in place not the actual driver module, so I can happily attach tcsd to the TPM device then rmmod tpm_tis, which causes some interesting issues. I can fix this, but it's not a problem of the current patch. 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, Jan 13, 2017 at 12:02:36PM -0800, James Bottomley wrote: > > > Actually, no, the devrm is a completely lifetime managed device as > > > part > > > of the chip structure. once you've done a device_del on it, it can > > > be > > > kfreed because it's no longer visible to anything else. > > > > No, that isn't enough. Anything else could have obtained a kref on > > devrm outside of the sphere the device_del manages. > > > > For instance, the cdev does exactly that, via this: > > > > > chip->cdev.kobj.parent = &chip->dev.kobj; > > > + chip->cdevrm.kobj.parent = &chip->devrm.kobj; > > > > In the worst case the kref the cdev grabs is not released until after > > tpm_chip_unregister() returns. > > chip_unregister doesn't tear down either device. It's the final > release of the chip->dev that does that. I don't think you are seeing the problem. I think you are assuming cdev_del waits for userspace to close any open fds. It does not. Instead cdev independently holds on to a module lock and a kref on the chip, once userspace has done close() then the kref is dropped and the module lock let go. This can happen after chip_unregister, after devm has done the final put_device, and after the driver core has completed driver detach. This is why it is necessary for this: + chip->cdevrm.kobj.parent = &chip->devrm.kobj; To point to a working kref, such as chip->dev.kobj. My point is this patch has two very subtle bugs caused by devrm having a broken kref. Yes we can fix those two cases, but this entire class of bugs is prevented if devrm has a release function that does put_device(chip->dev). We have no idea what will happen down the road; most poeple assume krefs *work*, having a struct with 4 krefs where only 3 work is pretty wild, IMHO. > Now there is a related problem that the owner is actually the *wrong* > module: it holds the tpm module in place not the actual driver module, > so I can happily attach tcsd to the TPM device then rmmod tpm_tis, > which causes some interesting issues. I can fix this, but it's not a > problem of the current patch. No, it is correct as is. The cdev fops rely only on the tpm module. When tpm_chip_unregister returns to the driver the chips->ops is set to NULL with proper locking - the driver code becomes uncallable at that point. 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
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 13ff5da..e50d768 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -3,7 +3,7 @@ # obj-$(CONFIG_TCG_TPM) += tpm.o tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \ - tpm_eventlog.o tpm2-space.o tpm-dev-common.o + tpm_eventlog.o tpm2-space.o tpm-dev-common.o tpms-dev.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..4548df0 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; /** @@ -168,27 +169,40 @@ 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, "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->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->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL); if (!chip->work_space.context_buf) { @@ -244,7 +258,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 +268,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) @@ -271,6 +313,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-interface.c b/drivers/char/tpm/tpm-interface.c index 65fcd04c..f5c9355 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -1197,9 +1197,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, "tpms"); + 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; } @@ -1211,7 +1219,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 0e93b93..61422e6 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -172,8 +172,8 @@ struct tpm_chip_seqops { }; 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 @@ -510,8 +510,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 { diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c new file mode 100644 index 0000000..c10b308 --- /dev/null +++ b/drivers/char/tpm/tpms-dev.c @@ -0,0 +1,62 @@ +/* + * 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; + + chip = container_of(inode->i_cdev, struct tpm_chip, cdevrm); + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (priv == NULL) + return -ENOMEM; + priv->space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL); + if (priv->space.context_buf == NULL) { + 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); + 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 tpm_rm_fops = { + .owner = THIS_MODULE, + .llseek = no_llseek, + .open = tpms_open, + .read = tpm_common_read, + .write = tpms_write, + .release = tpms_release, +}; +