Message ID | 20170122234438.12102-5-jarkko.sakkinen@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote: > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > Signed-off-by: James Bottomley > <James.Bottomley@HansenPartnership.com> I really think we should not use the ugly read/write interface for any new things. Still unconvinced we should add a new cdev at this point.. But seeing seesion support certainl is encouraging.. 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 Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote: > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote: > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > Signed-off-by: James Bottomley > > <James.Bottomley@HansenPartnership.com> > > I really think we should not use the ugly read/write interface for > any new things. The R/W interface is needed for backward compat, so we don't really have a choice (well, it could go in for long term deprecation, but I found in SCSI that "long term" == "never"). I think no-one objects to the ioctl interface ... it's just no-one feels strongly enough to build and test it. I'm sure if you send patches, Jarkko will include them. James > Still unconvinced we should add a new cdev at this point.. But seeing > seesion support certainl is encouraging.. -- 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, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote: > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote: > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote: > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > Signed-off-by: James Bottomley > > > <James.Bottomley@HansenPartnership.com> > > > > I really think we should not use the ugly read/write interface for > > any new things. > > The R/W interface is needed for backward compat, With what? This is a new cdev with different semantics. 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 Mon, 2017-01-23 at 15:49 -0700, Jason Gunthorpe wrote: > On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote: > > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote: > > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote: > > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > > > Signed-off-by: James Bottomley > > > > <James.Bottomley@HansenPartnership.com> > > > > > > I really think we should not use the ugly read/write interface > > > for > > > any new things. > > > > The R/W interface is needed for backward compat, > > With what? This is a new cdev with different semantics. If you set TPM_DEVICE=/dev/tpms0 the old software just works. If we remove the R/W interface, nothing will work. The point being the new cdev has the same interface semantics, it just has different global behavour. 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 Mon, Jan 23, 2017 at 02:57:11PM -0800, James Bottomley wrote: > On Mon, 2017-01-23 at 15:49 -0700, Jason Gunthorpe wrote: > > On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote: > > > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote: > > > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote: > > > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > > > > > Signed-off-by: James Bottomley > > > > > <James.Bottomley@HansenPartnership.com> > > > > > > > > I really think we should not use the ugly read/write interface > > > > for > > > > any new things. > > > > > > The R/W interface is needed for backward compat, > > > > With what? This is a new cdev with different semantics. > > If you set TPM_DEVICE=/dev/tpms0 the old software just works. If we > remove the R/W interface, nothing will work. The point being the new > cdev has the same interface semantics, it just has different global > behavour. So you are saying there is so much already deployed TPM2 software that has this TPM_DEVICE env var convention that we need to support it with compat? I'm really surprised by that.. But OK. Can you at least remove the 'user_read_timer' junk from the new cdev? 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 Mon, 2017-01-23 at 16:04 -0700, Jason Gunthorpe wrote: > On Mon, Jan 23, 2017 at 02:57:11PM -0800, James Bottomley wrote: > > On Mon, 2017-01-23 at 15:49 -0700, Jason Gunthorpe wrote: > > > On Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote: > > > > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote: > > > > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen > > > > > wrote: > > > > > > From: James Bottomley < > > > > > > James.Bottomley@HansenPartnership.com> > > > > > > > > > > > > Signed-off-by: James Bottomley > > > > > > <James.Bottomley@HansenPartnership.com> > > > > > > > > > > I really think we should not use the ugly read/write > > > > > interface for any new things. > > > > > > > > The R/W interface is needed for backward compat, > > > > > > With what? This is a new cdev with different semantics. > > > > If you set TPM_DEVICE=/dev/tpms0 the old software just works. If > > we remove the R/W interface, nothing will work. The point being > > the new cdev has the same interface semantics, it just has > > different global behavour. > > So you are saying there is so much already deployed TPM2 software > that has this TPM_DEVICE env var convention that we need to support > it with compat? > > I'm really surprised by that.. But OK. > > Can you at least remove the 'user_read_timer' junk from the new cdev? What's the problem with it? Can we not just fix whatever the issue is? I'd rather reuse all the R/W machinery as is. If I start trying to special case it so that we only use some parts on some control flows, the chances are I'll introduce additional bugs as well. 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 Mon, Jan 23, 2017 at 03:20:12PM -0800, James Bottomley wrote: > > So you are saying there is so much already deployed TPM2 software > > that has this TPM_DEVICE env var convention that we need to support > > it with compat? > > > > I'm really surprised by that.. But OK. > > > > Can you at least remove the 'user_read_timer' junk from the new cdev? > > What's the problem with it? Can we not just fix whatever the issue is? The issue is that it exists at all. I've been unwilling to remove it because some crazy userspace might rely on it, but I really don't want to see it continue in any new stuff. If you know the existing TPM1 userspace is safe then lets just delete it entirely. Otherwise lets be sure no new users crop up by disabling it. > I'd rather reuse all the R/W machinery as is. If I start trying to > special case it so that we only use some parts on some control flows, > the chances are I'll introduce additional bugs as well. Sure, this is part of the pain of compat.. 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 Mon, 2017-01-23 at 16:30 -0700, Jason Gunthorpe wrote: > On Mon, Jan 23, 2017 at 03:20:12PM -0800, James Bottomley wrote: > > > > So you are saying there is so much already deployed TPM2 software > > > that has this TPM_DEVICE env var convention that we need to > > > support it with compat? > > > > > > I'm really surprised by that.. But OK. > > > > > > Can you at least remove the 'user_read_timer' junk from the new > > > cdev? > > > > What's the problem with it? Can we not just fix whatever the issue > > is? > > The issue is that it exists at all. > > I've been unwilling to remove it because some crazy userspace might > rely on it, but I really don't want to see it continue in any new > stuff. All it does is clear the pending read after 60s ... like you, I suspect it could just be removed but I don't think having it present causes problems. > If you know the existing TPM1 userspace is safe then lets just delete > it entirely. Otherwise lets be sure no new users crop up by disabling > it. > > > I'd rather reuse all the R/W machinery as is. If I start trying to > > special case it so that we only use some parts on some control > > flows, the chances are I'll introduce additional bugs as well. > > Sure, this is part of the pain of compat.. Except for the added complexity and possibility of extra bugs, nothing is gained by the special casing. That tells me we should either remove this interface behaviour globally or not at all. Removing it globally would be independent of the space patches, because they'd simply inherit whatever was done. Why don't you start by doubling the timeout? If nothing notices, chances are nothing relies on this aspect of the interface and it can be easily removed. 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 Mon, Jan 23, 2017 at 03:45:58PM -0800, James Bottomley wrote: > Why don't you start by doubling the timeout? If nothing notices, > chances are nothing relies on this aspect of the interface and it can > be easily removed. Okay, fair enough, with a print I think it solves my concern. I sent a patch. 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 Mon, Jan 23, 2017 at 09:47:54AM -0700, Jason Gunthorpe wrote: > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote: > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > Signed-off-by: James Bottomley > > <James.Bottomley@HansenPartnership.com> > > I really think we should not use the ugly read/write interface for any > new things. > > Still unconvinced we should add a new cdev at this point.. But seeing > seesion support certainl is encouraging.. > > Jason We can then revisit the interface once my patch set includes the full functionality inluding the session management. It's only a tiny portion of the implementation and can be switched at any point. /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 Mon, Jan 23, 2017 at 02:28:23PM -0800, James Bottomley wrote: > On Mon, 2017-01-23 at 09:47 -0700, Jason Gunthorpe wrote: > > On Mon, Jan 23, 2017 at 01:44:32AM +0200, Jarkko Sakkinen wrote: > > > From: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > Signed-off-by: James Bottomley > > > <James.Bottomley@HansenPartnership.com> > > > > I really think we should not use the ugly read/write interface for > > any new things. > > The R/W interface is needed for backward compat, so we don't really > have a choice (well, it could go in for long term deprecation, but I > found in SCSI that "long term" == "never"). I think no-one objects to > the ioctl interface ... it's just no-one feels strongly enough to build > and test it. I'm sure if you send patches, Jarkko will include them. > > James I feel that it is incorrect to speak backwards compatibility because we do no touch /dev/tpm0. We can only speak about backwards compatibility only after the API for tpms0 is in a kernel release. If someone uses that device, she must know the constraints that it has. /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
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index 251d0ed..13ff5da 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_eventlog.o tpm2-space.o tpm-dev-common.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-dev-common.c b/drivers/char/tpm/tpm-dev-common.c new file mode 100644 index 0000000..0156562 --- /dev/null +++ b/drivers/char/tpm/tpm-dev-common.c @@ -0,0 +1,145 @@ +/* + * Copyright (C) 2004 IBM Corporation + * Authors: + * Leendert van Doorn <leendert@watson.ibm.com> + * Dave Safford <safford@watson.ibm.com> + * Reiner Sailer <sailer@watson.ibm.com> + * Kylene Hall <kjhall@us.ibm.com> + * + * Copyright (C) 2013 Obsidian Research Corp + * Jason Gunthorpe <jgunthorpe@obsidianresearch.com> + * + * Device file system interface to the TPM + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation, version 2 of the + * License. + * + */ +#include <linux/slab.h> +#include <linux/uaccess.h> +#include "tpm.h" +#include "tpm-dev.h" + +static void user_reader_timeout(unsigned long ptr) +{ + struct file_priv *priv = (struct file_priv *)ptr; + + schedule_work(&priv->work); +} + +static void timeout_work(struct work_struct *work) +{ + struct file_priv *priv = container_of(work, struct file_priv, work); + + mutex_lock(&priv->buffer_mutex); + atomic_set(&priv->data_pending, 0); + memset(priv->data_buffer, 0, sizeof(priv->data_buffer)); + mutex_unlock(&priv->buffer_mutex); +} + +void tpm_common_open(struct file *file, struct tpm_chip *chip, + struct file_priv *priv) +{ + priv->chip = chip; + atomic_set(&priv->data_pending, 0); + mutex_init(&priv->buffer_mutex); + setup_timer(&priv->user_read_timer, user_reader_timeout, + (unsigned long)priv); + INIT_WORK(&priv->work, timeout_work); + + file->private_data = priv; +} + +ssize_t tpm_common_read(struct file *file, char __user *buf, + size_t size, loff_t *off) +{ + struct file_priv *priv = file->private_data; + ssize_t ret_size; + ssize_t orig_ret_size; + int rc; + + del_singleshot_timer_sync(&priv->user_read_timer); + flush_work(&priv->work); + ret_size = atomic_read(&priv->data_pending); + if (ret_size > 0) { /* relay data */ + orig_ret_size = ret_size; + if (size < ret_size) + ret_size = size; + + mutex_lock(&priv->buffer_mutex); + rc = copy_to_user(buf, priv->data_buffer, ret_size); + memset(priv->data_buffer, 0, orig_ret_size); + if (rc) + ret_size = -EFAULT; + + mutex_unlock(&priv->buffer_mutex); + } + + atomic_set(&priv->data_pending, 0); + + return ret_size; +} + +ssize_t tpm_common_write(struct file *file, const char __user *buf, + size_t size, loff_t *off, struct tpm_space *space) +{ + struct file_priv *priv = file->private_data; + size_t in_size = size; + ssize_t out_size; + + /* Cannot perform a write until the read has cleared either via + * tpm_read or a user_read_timer timeout. This also prevents split + * buffered writes from blocking here. + */ + if (atomic_read(&priv->data_pending) != 0) + return -EBUSY; + + if (in_size > TPM_BUFSIZE) + return -E2BIG; + + mutex_lock(&priv->buffer_mutex); + + if (copy_from_user + (priv->data_buffer, (void __user *) buf, in_size)) { + mutex_unlock(&priv->buffer_mutex); + return -EFAULT; + } + + /* atomic tpm command send and result receive. We only hold the ops + * lock during this period so that the tpm can be unregistered even if + * the char dev is held open. + */ + if (tpm_try_get_ops(priv->chip)) { + mutex_unlock(&priv->buffer_mutex); + return -EPIPE; + } + out_size = tpm_transmit(priv->chip, space, priv->data_buffer, + sizeof(priv->data_buffer), 0); + + tpm_put_ops(priv->chip); + if (out_size < 0) { + mutex_unlock(&priv->buffer_mutex); + return out_size; + } + + atomic_set(&priv->data_pending, out_size); + mutex_unlock(&priv->buffer_mutex); + + /* Set a timeout by which the reader must come claim the result */ + mod_timer(&priv->user_read_timer, jiffies + (60 * HZ)); + + return in_size; +} + +/* + * Called on file close + */ +void tpm_common_release(struct file *file, struct file_priv *priv) +{ + del_singleshot_timer_sync(&priv->user_read_timer); + flush_work(&priv->work); + file->private_data = NULL; + atomic_set(&priv->data_pending, 0); +} diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c index 249eeb0..ebd74ab 100644 --- a/drivers/char/tpm/tpm-dev.c +++ b/drivers/char/tpm/tpm-dev.c @@ -18,45 +18,15 @@ * */ #include <linux/slab.h> -#include <linux/uaccess.h> -#include "tpm.h" - -struct file_priv { - struct tpm_chip *chip; - - /* Data passed to and from the tpm via the read/write calls */ - atomic_t data_pending; - struct mutex buffer_mutex; - - struct timer_list user_read_timer; /* user needs to claim result */ - struct work_struct work; - - u8 data_buffer[TPM_BUFSIZE]; -}; - -static void user_reader_timeout(unsigned long ptr) -{ - struct file_priv *priv = (struct file_priv *)ptr; - - schedule_work(&priv->work); -} - -static void timeout_work(struct work_struct *work) -{ - struct file_priv *priv = container_of(work, struct file_priv, work); - - mutex_lock(&priv->buffer_mutex); - atomic_set(&priv->data_pending, 0); - memset(priv->data_buffer, 0, sizeof(priv->data_buffer)); - mutex_unlock(&priv->buffer_mutex); -} +#include "tpm-dev.h" static int tpm_open(struct inode *inode, struct file *file) { - struct tpm_chip *chip = - container_of(inode->i_cdev, struct tpm_chip, cdev); + struct tpm_chip *chip; struct file_priv *priv; + 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. */ @@ -66,100 +36,22 @@ static int tpm_open(struct inode *inode, struct file *file) } priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (priv == NULL) { - clear_bit(0, &chip->is_open); - return -ENOMEM; - } + if (priv == NULL) + goto out; - priv->chip = chip; - atomic_set(&priv->data_pending, 0); - mutex_init(&priv->buffer_mutex); - setup_timer(&priv->user_read_timer, user_reader_timeout, - (unsigned long)priv); - INIT_WORK(&priv->work, timeout_work); + tpm_common_open(file, chip, priv); - file->private_data = priv; return 0; -} - -static ssize_t tpm_read(struct file *file, char __user *buf, - size_t size, loff_t *off) -{ - struct file_priv *priv = file->private_data; - ssize_t ret_size; - int rc; - - del_singleshot_timer_sync(&priv->user_read_timer); - flush_work(&priv->work); - ret_size = atomic_read(&priv->data_pending); - if (ret_size > 0) { /* relay data */ - ssize_t orig_ret_size = ret_size; - if (size < ret_size) - ret_size = size; - mutex_lock(&priv->buffer_mutex); - rc = copy_to_user(buf, priv->data_buffer, ret_size); - memset(priv->data_buffer, 0, orig_ret_size); - if (rc) - ret_size = -EFAULT; - - mutex_unlock(&priv->buffer_mutex); - } - - atomic_set(&priv->data_pending, 0); - - return ret_size; + out: + clear_bit(0, &chip->is_open); + return -ENOMEM; } static ssize_t tpm_write(struct file *file, const char __user *buf, size_t size, loff_t *off) { - struct file_priv *priv = file->private_data; - size_t in_size = size; - ssize_t out_size; - - /* cannot perform a write until the read has cleared - either via tpm_read or a user_read_timer timeout. - This also prevents splitted buffered writes from blocking here. - */ - if (atomic_read(&priv->data_pending) != 0) - return -EBUSY; - - if (in_size > TPM_BUFSIZE) - return -E2BIG; - - mutex_lock(&priv->buffer_mutex); - - if (copy_from_user - (priv->data_buffer, (void __user *) buf, in_size)) { - mutex_unlock(&priv->buffer_mutex); - return -EFAULT; - } - - /* atomic tpm command send and result receive. We only hold the ops - * lock during this period so that the tpm can be unregistered even if - * the char dev is held open. - */ - if (tpm_try_get_ops(priv->chip)) { - mutex_unlock(&priv->buffer_mutex); - return -EPIPE; - } - out_size = tpm_transmit(priv->chip, NULL, priv->data_buffer, - sizeof(priv->data_buffer), 0); - - tpm_put_ops(priv->chip); - if (out_size < 0) { - mutex_unlock(&priv->buffer_mutex); - return out_size; - } - - atomic_set(&priv->data_pending, out_size); - mutex_unlock(&priv->buffer_mutex); - - /* Set a timeout by which the reader must come claim the result */ - mod_timer(&priv->user_read_timer, jiffies + (60 * HZ)); - - return in_size; + return tpm_common_write(file, buf, size, off, NULL); } /* @@ -169,12 +61,10 @@ static int tpm_release(struct inode *inode, struct file *file) { struct file_priv *priv = file->private_data; - del_singleshot_timer_sync(&priv->user_read_timer); - flush_work(&priv->work); - file->private_data = NULL; - atomic_set(&priv->data_pending, 0); + tpm_common_release(file, priv); clear_bit(0, &priv->chip->is_open); kfree(priv); + return 0; } @@ -182,9 +72,7 @@ const struct file_operations tpm_fops = { .owner = THIS_MODULE, .llseek = no_llseek, .open = tpm_open, - .read = tpm_read, + .read = tpm_common_read, .write = tpm_write, .release = tpm_release, }; - - diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h new file mode 100644 index 0000000..ff15cf7 --- /dev/null +++ b/drivers/char/tpm/tpm-dev.h @@ -0,0 +1,27 @@ +#ifndef _TPM_DEV_H +#define _TPM_DEV_H + +#include "tpm.h" + +struct file_priv { + struct tpm_chip *chip; + + /* Data passed to and from the tpm via the read/write calls */ + atomic_t data_pending; + struct mutex buffer_mutex; + + struct timer_list user_read_timer; /* user needs to claim result */ + struct work_struct work; + + u8 data_buffer[TPM_BUFSIZE]; +}; + +void tpm_common_open(struct file *file, struct tpm_chip *chip, + struct file_priv *priv); +ssize_t tpm_common_read(struct file *file, char __user *buf, + size_t size, loff_t *off); +ssize_t tpm_common_write(struct file *file, const char __user *buf, + size_t size, loff_t *off, struct tpm_space *space); +void tpm_common_release(struct file *file, struct file_priv *priv); + +#endif