From: Jason G. <jgu...@ob...> - 2009-11-03 00:57:21
|
How to use the TPM is really a user space policy choice, if the environment wants to use middleware then fine, but it is possible to make correct TPM apps without using middleware. So, remove the kernel restriction that only one process may open the TPM. - TPM low level functions (in kernel users) are already locked proprely and can run in parallel with the user space interface anyhow. - Move the user space data buffer and related goop into a struct tpm_file, create one struct tpm_file per open file. Signed-off-by: Jason Gunthorpe <jgu...@ob...> --- drivers/char/tpm/tpm.c | 87 +++++++++++++++++++++-------------------------- drivers/char/tpm/tpm.h | 24 ++++++++----- 2 files changed, 53 insertions(+), 58 deletions(-) Mainly for embedded where the middleware is perhaps not appropriate. Tested with a winbond TPM and tpm_tis. diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c index 196bc48..0625404 100644 --- a/drivers/char/tpm/tpm.c +++ b/drivers/char/tpm/tpm.c @@ -31,7 +31,6 @@ enum tpm_const { TPM_MINOR = 224, /* officially assigned */ - TPM_BUFSIZE = 2048, TPM_NUM_DEVICES = 256, }; @@ -321,19 +320,19 @@ static const u8 tpm_ordinal_duration[TPM_MAX_ORDINAL] = { static void user_reader_timeout(unsigned long ptr) { - struct tpm_chip *chip = (struct tpm_chip *) ptr; + struct tpm_file *fl = (struct tpm_file *) ptr; - schedule_work(&chip->work); + schedule_work(&fl->work); } static void timeout_work(void *ptr) { - struct tpm_chip *chip = ptr; + struct tpm_file *fl = ptr; - mutex_lock(&chip->buffer_mutex); - atomic_set(&chip->data_pending, 0); - memset(chip->data_buffer, 0, TPM_BUFSIZE); - mutex_unlock(&chip->buffer_mutex); + mutex_lock(&fl->buffer_mutex); + atomic_set(&fl->data_pending, 0); + memset(fl->data_bufferx, 0, sizeof(fl->data_bufferx)); + mutex_unlock(&fl->buffer_mutex); } /* @@ -962,6 +961,7 @@ int tpm_open(struct inode *inode, struct file *file) { int minor = iminor(inode); struct tpm_chip *chip = NULL, *pos; + struct tpm_file *fl; rcu_read_lock(); list_for_each_entry_rcu(pos, &tpm_chip_list, list) { @@ -976,22 +976,19 @@ int tpm_open(struct inode *inode, struct file *file) if (!chip) return -ENODEV; - if (test_and_set_bit(0, &chip->is_open)) { - dev_dbg(chip->dev, "Another process owns this TPM\n"); - put_device(chip->dev); - return -EBUSY; - } - - chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL); - if (chip->data_buffer == NULL) { - clear_bit(0, &chip->is_open); + fl = kzalloc(sizeof(*fl), GFP_KERNEL); + if (fl == NULL) { put_device(chip->dev); return -ENOMEM; } - atomic_set(&chip->data_pending, 0); + fl->chip = chip; + mutex_init(&fl->buffer_mutex); + setup_timer(&fl->user_read_timer, user_reader_timeout, + (unsigned long)fl); + INIT_WORK(&fl->work, timeout_work, fl); - file->private_data = chip; + file->private_data = fl; return 0; } EXPORT_SYMBOL_GPL(tpm_open); @@ -1001,14 +998,14 @@ EXPORT_SYMBOL_GPL(tpm_open); */ int tpm_release(struct inode *inode, struct file *file) { - struct tpm_chip *chip = file->private_data; + struct tpm_file *fl = file->private_data; + struct tpm_chip *chip = fl->chip; - del_singleshot_timer_sync(&chip->user_read_timer); + del_singleshot_timer_sync(&fl->user_read_timer); flush_scheduled_work(); + mutex_destroy(&fl->buffer_mutex); + kfree(file->private_data); file->private_data = NULL; - atomic_set(&chip->data_pending, 0); - kfree(chip->data_buffer); - clear_bit(0, &chip->is_open); put_device(chip->dev); return 0; } @@ -1017,33 +1014,33 @@ EXPORT_SYMBOL_GPL(tpm_release); ssize_t tpm_write(struct file *file, const char __user *buf, size_t size, loff_t *off) { - struct tpm_chip *chip = file->private_data; + struct tpm_file *fl = file->private_data; size_t in_size = size, out_size; /* cannot perform a write until the read has cleared either via tpm_read or a user_read_timer timeout */ - while (atomic_read(&chip->data_pending) != 0) + while (atomic_read(&fl->data_pending) != 0) msleep(TPM_TIMEOUT); - mutex_lock(&chip->buffer_mutex); + mutex_lock(&fl->buffer_mutex); - if (in_size > TPM_BUFSIZE) - in_size = TPM_BUFSIZE; + in_size = min(sizeof(fl->data_bufferx), in_size); if (copy_from_user - (chip->data_buffer, (void __user *) buf, in_size)) { - mutex_unlock(&chip->buffer_mutex); + (fl->data_bufferx, (void __user *) buf, in_size)) { + mutex_unlock(&fl->buffer_mutex); return -EFAULT; } /* atomic tpm command send and result receive */ - out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE); + out_size = tpm_transmit(fl->chip, fl->data_bufferx, + sizeof(fl->data_bufferx)); - atomic_set(&chip->data_pending, out_size); - mutex_unlock(&chip->buffer_mutex); + atomic_set(&fl->data_pending, out_size); + mutex_unlock(&fl->buffer_mutex); /* Set a timeout by which the reader must come claim the result */ - mod_timer(&chip->user_read_timer, jiffies + (60 * HZ)); + mod_timer(&fl->user_read_timer, jiffies + (60 * HZ)); return in_size; } @@ -1052,21 +1049,21 @@ EXPORT_SYMBOL_GPL(tpm_write); ssize_t tpm_read(struct file *file, char __user *buf, size_t size, loff_t *off) { - struct tpm_chip *chip = file->private_data; + struct tpm_file *fl = file->private_data; ssize_t ret_size; - del_singleshot_timer_sync(&chip->user_read_timer); + del_singleshot_timer_sync(&fl->user_read_timer); flush_scheduled_work(); - ret_size = atomic_read(&chip->data_pending); - atomic_set(&chip->data_pending, 0); + ret_size = atomic_read(&fl->data_pending); + atomic_set(&fl->data_pending, 0); if (ret_size > 0) { /* relay data */ if (size < ret_size) ret_size = size; - mutex_lock(&chip->buffer_mutex); - if (copy_to_user(buf, chip->data_buffer, ret_size)) + mutex_lock(&fl->buffer_mutex); + if (copy_to_user(buf, fl->data_bufferx, ret_size)) ret_size = -EFAULT; - mutex_unlock(&chip->buffer_mutex); + mutex_unlock(&fl->buffer_mutex); } return ret_size; @@ -1182,15 +1179,9 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, if (chip == NULL || devname == NULL) goto out_free; - mutex_init(&chip->buffer_mutex); mutex_init(&chip->tpm_mutex); INIT_LIST_HEAD(&chip->list); - INIT_WORK(&chip->work, timeout_work, chip); - - setup_timer(&chip->user_read_timer, user_reader_timeout, - (unsigned long)chip); - memcpy(&chip->vendor, entry, sizeof(struct tpm_vendor_specific)); chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 8e00b4d..0fa262d 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -91,16 +91,6 @@ struct tpm_chip { struct device *dev; /* Device stuff */ int dev_num; /* /dev/tpm# */ - unsigned long is_open; /* only one allowed */ - int time_expired; - - /* Data passed to and from the tpm via the read/write calls */ - u8 *data_buffer; - atomic_t data_pending; - struct mutex buffer_mutex; - - struct timer_list user_read_timer; /* user needs to claim result */ - struct work_struct work; struct mutex tpm_mutex; /* tpm is processing */ struct tpm_vendor_specific vendor; @@ -113,6 +103,20 @@ struct tpm_chip { #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor) +/* Private data structure for struct file */ +struct tpm_file { + 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_bufferx[2048]; +}; + static inline int tpm_read_index(int base, int index) { outb(index, base); -- 1.5.4.2 |
From: Jason G. <jgu...@ob...> - 2012-09-30 23:49:03
|
How to use the TPM is really a user space policy choice, if the environment wants to use middleware then fine, but it is possible to make correct TPM apps without using middleware. So, remove the kernel restriction that only one process may open the TPM. - TPM low level functions (in kernel users) are already locked proprely and can run in parallel with the user space interface anyhow. - Move the user space data buffer and related goop into a struct tpm_file, create one struct tpm_file per open file. Signed-off-by: Jason Gunthorpe <jgu...@ob...> --- drivers/char/tpm/tpm.c | 97 +++++++++++++++++++++--------------------------- drivers/char/tpm/tpm.h | 23 ++++++----- 2 files changed, 55 insertions(+), 65 deletions(-) This is rebase, retest, resend of a patch I sent two years ago. The discussion on that earlier patch fizzled out. Resending incase there is renewed interest :) diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c index 60e8442..a161be9 100644 --- a/drivers/char/tpm/tpm.c +++ b/drivers/char/tpm/tpm.c @@ -33,7 +33,6 @@ enum tpm_const { TPM_MINOR = 224, /* officially assigned */ - TPM_BUFSIZE = 4096, TPM_NUM_DEVICES = 256, }; @@ -333,19 +332,19 @@ static const u8 tpm_ordinal_duration[TPM_MAX_ORDINAL] = { static void user_reader_timeout(unsigned long ptr) { - struct tpm_chip *chip = (struct tpm_chip *) ptr; + struct tpm_file *fl = (struct tpm_file *) ptr; - schedule_work(&chip->work); + schedule_work(&fl->work); } static void timeout_work(struct work_struct *work) { - struct tpm_chip *chip = container_of(work, struct tpm_chip, work); + struct tpm_file *fl = container_of(work, struct tpm_file, work); - mutex_lock(&chip->buffer_mutex); - atomic_set(&chip->data_pending, 0); - memset(chip->data_buffer, 0, TPM_BUFSIZE); - mutex_unlock(&chip->buffer_mutex); + mutex_lock(&fl->buffer_mutex); + atomic_set(&fl->data_pending, 0); + memset(fl->data_bufferx, 0, sizeof(fl->data_bufferx)); + mutex_unlock(&fl->buffer_mutex); } /* @@ -384,9 +383,6 @@ static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, u32 count, ordinal; unsigned long stop; - if (bufsiz > TPM_BUFSIZE) - bufsiz = TPM_BUFSIZE; - count = be32_to_cpu(*((__be32 *) (buf + 2))); ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); if (count == 0) @@ -1161,6 +1157,7 @@ int tpm_open(struct inode *inode, struct file *file) { int minor = iminor(inode); struct tpm_chip *chip = NULL, *pos; + struct tpm_file *fl; rcu_read_lock(); list_for_each_entry_rcu(pos, &tpm_chip_list, list) { @@ -1175,22 +1172,19 @@ int tpm_open(struct inode *inode, struct file *file) if (!chip) return -ENODEV; - if (test_and_set_bit(0, &chip->is_open)) { - dev_dbg(chip->dev, "Another process owns this TPM\n"); - put_device(chip->dev); - return -EBUSY; - } - - chip->data_buffer = kzalloc(TPM_BUFSIZE, GFP_KERNEL); - if (chip->data_buffer == NULL) { - clear_bit(0, &chip->is_open); + fl = kzalloc(sizeof(*fl), GFP_KERNEL); + if (fl == NULL) { put_device(chip->dev); return -ENOMEM; } - atomic_set(&chip->data_pending, 0); + fl->chip = chip; + mutex_init(&fl->buffer_mutex); + setup_timer(&fl->user_read_timer, user_reader_timeout, + (unsigned long)fl); + INIT_WORK(&fl->work, timeout_work); - file->private_data = chip; + file->private_data = fl; return 0; } EXPORT_SYMBOL_GPL(tpm_open); @@ -1200,14 +1194,14 @@ EXPORT_SYMBOL_GPL(tpm_open); */ int tpm_release(struct inode *inode, struct file *file) { - struct tpm_chip *chip = file->private_data; + struct tpm_file *fl = file->private_data; + struct tpm_chip *chip = fl->chip; - del_singleshot_timer_sync(&chip->user_read_timer); - flush_work_sync(&chip->work); + del_singleshot_timer_sync(&fl->user_read_timer); + flush_work_sync(&fl->work); + mutex_destroy(&fl->buffer_mutex); + kfree(file->private_data); file->private_data = NULL; - atomic_set(&chip->data_pending, 0); - kfree(chip->data_buffer); - clear_bit(0, &chip->is_open); put_device(chip->dev); return 0; } @@ -1216,33 +1210,33 @@ EXPORT_SYMBOL_GPL(tpm_release); ssize_t tpm_write(struct file *file, const char __user *buf, size_t size, loff_t *off) { - struct tpm_chip *chip = file->private_data; + struct tpm_file *fl = file->private_data; size_t in_size = size, out_size; /* cannot perform a write until the read has cleared either via tpm_read or a user_read_timer timeout */ - while (atomic_read(&chip->data_pending) != 0) + while (atomic_read(&fl->data_pending) != 0) msleep(TPM_TIMEOUT); - mutex_lock(&chip->buffer_mutex); + mutex_lock(&fl->buffer_mutex); - if (in_size > TPM_BUFSIZE) - in_size = TPM_BUFSIZE; + in_size = min(sizeof(fl->data_bufferx), in_size); if (copy_from_user - (chip->data_buffer, (void __user *) buf, in_size)) { - mutex_unlock(&chip->buffer_mutex); + (fl->data_bufferx, (void __user *) buf, in_size)) { + mutex_unlock(&fl->buffer_mutex); return -EFAULT; } /* atomic tpm command send and result receive */ - out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE); + out_size = tpm_transmit(fl->chip, fl->data_bufferx, + sizeof(fl->data_bufferx)); - atomic_set(&chip->data_pending, out_size); - mutex_unlock(&chip->buffer_mutex); + atomic_set(&fl->data_pending, out_size); + mutex_unlock(&fl->buffer_mutex); /* Set a timeout by which the reader must come claim the result */ - mod_timer(&chip->user_read_timer, jiffies + (60 * HZ)); + mod_timer(&fl->user_read_timer, jiffies + (60 * HZ)); return in_size; } @@ -1251,26 +1245,25 @@ EXPORT_SYMBOL_GPL(tpm_write); ssize_t tpm_read(struct file *file, char __user *buf, size_t size, loff_t *off) { - struct tpm_chip *chip = file->private_data; + struct tpm_file *fl = file->private_data; ssize_t ret_size; int rc; - del_singleshot_timer_sync(&chip->user_read_timer); - flush_work_sync(&chip->work); - ret_size = atomic_read(&chip->data_pending); - atomic_set(&chip->data_pending, 0); + del_singleshot_timer_sync(&fl->user_read_timer); + flush_work_sync(&fl->work); + ret_size = atomic_read(&fl->data_pending); + atomic_set(&fl->data_pending, 0); if (ret_size > 0) { /* relay data */ ssize_t orig_ret_size = ret_size; if (size < ret_size) ret_size = size; - mutex_lock(&chip->buffer_mutex); - rc = copy_to_user(buf, chip->data_buffer, ret_size); - memset(chip->data_buffer, 0, orig_ret_size); + mutex_lock(&fl->buffer_mutex); + rc = copy_to_user(buf, fl->data_bufferx, ret_size); + memset(fl->data_bufferx, 0, orig_ret_size); if (rc) ret_size = -EFAULT; - - mutex_unlock(&chip->buffer_mutex); + mutex_unlock(&fl->buffer_mutex); } return ret_size; @@ -1413,15 +1406,9 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, if (chip == NULL || devname == NULL) goto out_free; - mutex_init(&chip->buffer_mutex); mutex_init(&chip->tpm_mutex); INIT_LIST_HEAD(&chip->list); - INIT_WORK(&chip->work, timeout_work); - - setup_timer(&chip->user_read_timer, user_reader_timeout, - (unsigned long)chip); - memcpy(&chip->vendor, entry, sizeof(struct tpm_vendor_specific)); chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 2d583ef..58bde7c 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -106,16 +106,6 @@ struct tpm_chip { struct device *dev; /* Device stuff */ int dev_num; /* /dev/tpm# */ - unsigned long is_open; /* only one allowed */ - int time_expired; - - /* Data passed to and from the tpm via the read/write calls */ - u8 *data_buffer; - atomic_t data_pending; - struct mutex buffer_mutex; - - struct timer_list user_read_timer; /* user needs to claim result */ - struct work_struct work; struct mutex tpm_mutex; /* tpm is processing */ struct tpm_vendor_specific vendor; @@ -132,6 +122,19 @@ static inline void tpm_chip_put(struct tpm_chip *chip) { module_put(chip->dev->driver->owner); } +/* Private data structure for struct file */ +struct tpm_file { + 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_bufferx[2048]; +}; static inline int tpm_read_index(int base, int index) { -- 1.7.4.1 |
From: <Pet...@in...> - 2012-10-01 09:07:26
|
Hi Jason, one quick question: - TPM_BUFSIZE = 4096, [...] + u8 data_bufferx[2048]; Why do you half the buffer size? Thanks, Peter |
From: Jason G. <jgu...@ob...> - 2012-10-01 16:09:24
|
On Mon, Oct 01, 2012 at 09:07:11AM +0000, Pet...@in... wrote: > Hi Jason, > > one quick question: > - TPM_BUFSIZE = 4096, > [...] > + u8 data_bufferx[2048]; > > Why do you half the buffer size? I missed 7f366784f5c2b8fc065 when I rebased the patch, thanks! Jason |
From: Kent Y. <ke...@li...> - 2012-10-10 16:37:20
|
On Sun, Sep 30, 2012 at 05:33:45PM -0600, Jason Gunthorpe wrote: > How to use the TPM is really a user space policy choice, if the > environment wants to use middleware then fine, but it is possible to > make correct TPM apps without using middleware. I'm not sure how I feel about this. The single open rule doesn't prevent replacement of the middleware, it just requires a open()/close() around any use of the device node. That seems simple enough to me. In places where you do want TSS to be the sole opener, it can't enforce that rule itself, so I think we need to preserve the option of a single open at a minimum. Kent > So, remove the kernel restriction that only one process may open the TPM. > - TPM low level functions (in kernel users) are already locked proprely > and can run in parallel with the user space interface anyhow. > - Move the user space data buffer and related goop into a > struct tpm_file, create one struct tpm_file per open file. > > Signed-off-by: Jason Gunthorpe <jgu...@ob...> > --- > drivers/char/tpm/tpm.c | 97 +++++++++++++++++++++--------------------------- > drivers/char/tpm/tpm.h | 23 ++++++----- > 2 files changed, 55 insertions(+), 65 deletions(-) > > This is rebase, retest, resend of a patch I sent two years ago. The > discussion on that earlier patch fizzled out. Resending incase there > is renewed interest :) > |
From: Jason G. <jgu...@ob...> - 2012-10-12 20:57:07
|
On Wed, Oct 10, 2012 at 11:33:24AM -0500, Kent Yoder wrote: > On Sun, Sep 30, 2012 at 05:33:45PM -0600, Jason Gunthorpe wrote: > > How to use the TPM is really a user space policy choice, if the > > environment wants to use middleware then fine, but it is possible to > > make correct TPM apps without using middleware. > > I'm not sure how I feel about this. The single open rule doesn't > prevent replacement of the middleware, it just requires a > open()/close() I'm not interested in replacing the middleware, our designs do not use any middleware and several processes are required to access the TPM at once. Using open/close is an interesting idea, but it wouldn't work. open() is coded to return EBUSY if another process has it open, rather than block, and spinning on open would be unacceptable. > around any use of the device node. That seems simple enough to me. In > places where you do want TSS to be the sole opener, it can't enforce > that rule itself, so I think we need to preserve the option of a single > open at a minimum. I agree, but I'm not sure how to expose this function? I've seen other places abuse O_EXCL for this, but that would not be compatible with existing implementations. Three things come to mind - O_EXCL means the open only succeeds if it the only open and prevents others (not compatible) - Some other O_ flag is hijacked to mean the opposite of the above (yuk) - A sysfs flag is added to turn on the new O_EXCL behavior What do you think? Jason |
From: Kent Y. <ke...@li...> - 2012-10-15 22:03:15
|
> Using open/close is an interesting idea, but it wouldn't work. open() > is coded to return EBUSY if another process has it open, rather than > block, and spinning on open would be unacceptable. I'm trying to imagine when spinning would be unacceptable? Keygen could take several seconds, regardless of whether you can get the command to the chip right away or not... > > around any use of the device node. That seems simple enough to me. In > > places where you do want TSS to be the sole opener, it can't enforce > > that rule itself, so I think we need to preserve the option of a single > > open at a minimum. > > I agree, but I'm not sure how to expose this function? I've seen other > places abuse O_EXCL for this, but that would not be compatible with > existing implementations. > > Three things come to mind > - O_EXCL means the open only succeeds if it the only open and > prevents others (not compatible) > - Some other O_ flag is hijacked to mean the opposite of the above > (yuk) > - A sysfs flag is added to turn on the new O_EXCL behavior > > What do you think? My first thought was a sysfs var to turn off exclusivity that defaulted to today's behavior. Kent > Jason > |
From: <Pet...@in...> - 2012-10-15 08:35:21
|
-----Original Message----- > From: Jason Gunthorpe [mailto:jgu...@ob...] > Using open/close is an interesting idea, but it wouldn't work. open() > is coded to return EBUSY if another process has it open, rather than > block, and spinning on open would be unacceptable. Hmm, maybe write a small pass through program which opens /dev/tpm once and accepts its data via a socket or pipe? Thanks, Peter |
From: Jason G. <jgu...@ob...> - 2012-10-15 16:40:00
|
On Mon, Oct 15, 2012 at 08:35:09AM +0000, Pet...@in... wrote: > > From: Jason Gunthorpe [mailto:jgu...@ob...] > > > Using open/close is an interesting idea, but it wouldn't work. open() > > is coded to return EBUSY if another process has it open, rather than > > block, and spinning on open would be unacceptable. > > Hmm, maybe write a small pass through program which opens /dev/tpm > once and accepts its data via a socket or pipe? I believe the kernel should not be enforcing this kind of policy into userspace. Plus, some of our embedded system are memory constrained so an unnecessary process is not welcome.. Jason |
From: Alan C. <al...@lx...> - 2012-10-15 16:44:32
|
On Mon, 15 Oct 2012 10:39:45 -0600 Jason Gunthorpe <jgu...@ob...> wrote: > On Mon, Oct 15, 2012 at 08:35:09AM +0000, Pet...@in... wrote: > > > From: Jason Gunthorpe [mailto:jgu...@ob...] > > > > > Using open/close is an interesting idea, but it wouldn't work. open() > > > is coded to return EBUSY if another process has it open, rather than > > > block, and spinning on open would be unacceptable. > > > > Hmm, maybe write a small pass through program which opens /dev/tpm > > once and accepts its data via a socket or pipe? > > I believe the kernel should not be enforcing this kind of policy into > userspace. Plus, some of our embedded system are memory constrained > so an unnecessary process is not welcome.. Sane device drivers for devices where contention is meaningful block on an open that is busy, or return an error if the O_NONBLOCK option is specified. That's the normal case. Alan |
From: Jason G. <jgu...@ob...> - 2012-10-15 16:56:35
|
On Mon, Oct 15, 2012 at 05:49:22PM +0100, Alan Cox wrote: > > > > Using open/close is an interesting idea, but it wouldn't work. open() > > > > is coded to return EBUSY if another process has it open, rather than > > > > block, and spinning on open would be unacceptable. > > > > > > Hmm, maybe write a small pass through program which opens /dev/tpm > > > once and accepts its data via a socket or pipe? > > > > I believe the kernel should not be enforcing this kind of policy into > > userspace. Plus, some of our embedded system are memory constrained > > so an unnecessary process is not welcome.. > > Sane device drivers for devices where contention is meaningful block on > an open that is busy, or return an error if the O_NONBLOCK option is > specified. That's the normal case. We have here a situation where there is no kernel or hardware requirement for exclusivity, but the current driver enforces it, for userspace only. Today the kernel and user space can access the TPM device concurrently. So, I would like to migrate the userspace interface to allow non-exclusivity, but how do we do this? Jason |
From: Alan C. <al...@lx...> - 2009-11-03 11:04:04
|
> - Move the user space data buffer and related goop into a > struct tpm_file, create one struct tpm_file per open file. Is that really sufficient ? You can open a file once, dup it (or inherit it and get multiple callers from the same file handle. Ditto consider threated apps Alan |
From: Jason G. <jgu...@ob...> - 2009-11-03 17:16:05
|
On Tue, Nov 03, 2009 at 10:43:52AM +0000, Alan Cox wrote: > > - Move the user space data buffer and related goop into a > > struct tpm_file, create one struct tpm_file per open file. > > Is that really sufficient ? Yes, I think so. Basically, all I did was take the per-device locking that was already there and make it per-file. The locking hasn't changed. If there was a multi-threading bug in the old code, it is still in this version - but I looked for such a thing and didn't see anything. > You can open a file once, dup it (or inherit it and get multiple callers > from the same file handle. Ditto consider threated apps Yes, I did, I think it is OK. All the operations involving the file's buffer are atomic or protected by a per-file mutex. Do you see something specific? It would be dangerous to do that anyhow, the way the TPM interface works requires matched write/read pairs. The driver has a hacky way to halt a write until the matching write happens, but it is based on timeouts and probably isn't 100% reliable. Thanks, Jason |
From: Hal F. <hal...@gm...> - 2009-11-03 17:32:15
|
What if you don't want it accessible by user mode apps, you only want your middleware (ie tcs daemon, tcsd) to open it? Will this still allow that to be enforced, so nobody can interfere with tcsd's exclusive access to the device? Hal Finney |
From: Jason G. <jgu...@ob...> - 2009-11-03 17:46:51
|
On Tue, Nov 03, 2009 at 09:31:55AM -0800, Hal Finney wrote: > What if you don't want it accessible by user mode apps, you only want > your middleware (ie tcs daemon, tcsd) to open it? Will this still > allow that to be enforced, so nobody can interfere with tcsd's > exclusive access to the device? I did not attempt to implement something like that. Most cases in Linux seem to leave that to user space.. Jason |
From: <Val...@vt...> - 2009-11-03 18:14:50
|
On Tue, 03 Nov 2009 09:31:55 PST, Hal Finney said: > What if you don't want it accessible by user mode apps, you only want > your middleware (ie tcs daemon, tcsd) to open it? Will this still > allow that to be enforced, so nobody can interfere with tcsd's > exclusive access to the device? Couldn't tcsd just open the device with O_EXCL? Or am I missing something subtle here? |
From: Jason G. <jgu...@ob...> - 2009-11-03 22:42:19
|
On Tue, Nov 03, 2009 at 01:14:28PM -0500, Val...@vt... wrote: > On Tue, 03 Nov 2009 09:31:55 PST, Hal Finney said: > > What if you don't want it accessible by user mode apps, you only want > > your middleware (ie tcs daemon, tcsd) to open it? Will this still > > allow that to be enforced, so nobody can interfere with tcsd's > > exclusive access to the device? > > Couldn't tcsd just open the device with O_EXCL? Or am I missing something > subtle here? O_EXCL isn't a locking flag... O_EXCL Ensure that this call creates the file: if this flag is specified in conjunction with O_CREAT, and pathname already exists, then open() will fail. The behavior of O_EXCL is undefined if O_CREAT is not specified. Jason |
From: <Val...@vt...> - 2009-11-04 03:24:47
|
On Tue, 03 Nov 2009 15:41:58 MST, Jason Gunthorpe said: > On Tue, Nov 03, 2009 at 01:14:28PM -0500, Val...@vt... wrote: > > On Tue, 03 Nov 2009 09:31:55 PST, Hal Finney said: > > > What if you don't want it accessible by user mode apps, you only want > > > your middleware (ie tcs daemon, tcsd) to open it? Will this still > > > allow that to be enforced, so nobody can interfere with tcsd's > > > exclusive access to the device? > > > > Couldn't tcsd just open the device with O_EXCL? Or am I missing something > > subtle here? > > O_EXCL isn't a locking flag... Sorry, getting over the flu, my brain isn't totally online yet. I was thinking of TIOCEXCL which is (a) an ioctl() and (b) apparently tty-specific. A number of other things under drivers/ implement "only one open" semantics, but those are hard-coded into the driver. But for the TPM, it's unclear if exclusive or non-exclusive is the right model. Maybe the right answer is to default to multiple opens, but have an ioctl() that turns on exclusive mode. If you have a 'tcsd' daemon, it will need to get launched early enough to do the open/ioctl before somebody else gets running anyhow, so it's not adding to any raciness. Yeah, it's a hack. And there's still a small DoS issue - if the system is supposed to allow multiple opens, an abusive process can ioctl() it and break it. |
From: Jason G. <jgu...@ob...> - 2009-11-04 04:27:33
|
On Tue, Nov 03, 2009 at 10:24:29PM -0500, Val...@vt... wrote: > A number of other things under drivers/ implement "only one open" semantics, > but those are hard-coded into the driver. But for the TPM, it's unclear if > exclusive or non-exclusive is the right model. The underlying hardware already supports multiplexing multiple clients in the same command stream - I'm not sure why this shouldn't be exported to user space as-is. The kernel already accesses the TPM without going through the middleware for in kernel features.. > Maybe the right answer is to default to multiple opens, but have an > ioctl() that turns on exclusive mode. If you have a 'tcsd' daemon, > it will need to get launched early enough to do the open/ioctl Why is this an issue? /dev/tpm is root only accessible. There are a lot of things that can go horribly wrong if root does improper things, and you can create quite reasonable multi-process tpm using applications without the middleware. Even if another root process does open /dev/tpm - what is the worst it can do? Jason |
From: Alan C. <al...@lx...> - 2009-11-04 09:56:52
|
> but those are hard-coded into the driver. But for the TPM, it's unclear if > exclusive or non-exclusive is the right model. Maybe the right answer is to > default to multiple opens, but have an ioctl() that turns on exclusive mode. > If you have a 'tcsd' daemon, it will need to get launched early enough > to do the open/ioctl before somebody else gets running anyhow, so it's > not adding to any raciness. Yeah, it's a hack. And there's still a small > DoS issue - if the system is supposed to allow multiple opens, an abusive > process can ioctl() it and break it. What's wrong with "chmod" ? BTW some drivers do also implement O_EXCL = one open semantics. It's not exactly what POSIX expects but these are drivers and its a fairly logical extension thereto. Alan |