On Thu, Nov 03, 2016 at 11:14:10PM -0600, Jarkko Sakkinen wrote:
> On Tue, Oct 18, 2016 at 08:49:42PM -0400, Nayna Jain wrote:
> > Currently, the event log file operations are not serialized with
> > tpm_chip_unregister(), which can possibly cause a race condition.
> >
> > This patch fixes the race condition by:
> > - moving read_log() from fops to chip register.
>
> What is "chip register"? Please use exact names.
>
> > - disallowing event log file operations when chip unregister is in
> > progress.
>
> Could you elaborate this sentence?
>
> > - guarding event log memory using chip krefs.
>
> Could you elaborate this sentence?
>
> Please describe how the race condition could happen and provide the
> "Fixes:" line for the commit ID that caused it. Otherwise, your commit
> message won't make any sense. I cannot apply this commit with this
> commit message.
>
> The commit message does not make much sense...
Lets get this moving along, it is hard to keep everything straight
over months..
Nayna: This commit message should work:
tpm: Have eventlog use the tpm_chip
Move the backing memory for the eventlog into tpm_chip and push
the tpm_chip into read_log. This optimizes read_log processing by
only doing it once and prepares things for the next patches in the
series which require the tpm_chip to locate the event log via
ACPI and OF handles instead of searching.
This is straightfoward except for the issue of passing a kref through
i_private with securityfs. Since securityfs_remove does not have any
removal fencing like sysfs we use the inode lock to safely get a
kref on the tpm_chip.
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index eac1f10..813e0d7 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -127,6 +127,7 @@ static void tpm_dev_release(struct device *dev)
> > idr_remove(&dev_nums_idr, chip->dev_num);
> > mutex_unlock(&idr_lock);
> >
> > + kfree(chip->log.bios_event_log);
> > kfree(chip);
> > }
[..]
> > /* read binary bios log */
> > -int read_log(struct tpm_bios_log *log)
> > +int read_log(struct tpm_chip *chip)
[..]
> > +err:
> > + kfree(log->bios_event_log);
>
> Is this kfree() necessary?
Yes, if the log is not loaded then bios_event_log log should be null.
Nayna - there is a bug here, you have to null bios_event_log after
kfree'ing it so that tpm_dev_release does not attempt to kfree a
free'd pointer.
> > + inode_lock(inode);
> > + if (!inode->i_private) {
> > + inode_unlock(inode);
> > + return -ENODEV;
> > + }
>
> Why is this is done (inode_lock + setting to NULL), and if so, why
> it wasn't done in 1/7?
Patch 1 does not use memory backed by tpm_chip, this is the first
patch in the series that introduces that concept, so it is the first
place that needs to do this.
> > - for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--)
> > + for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
> > + inode = d_inode(chip->bios_dir[i]);
> > + inode_lock(inode);
> > + inode->i_private = NULL;
> > + inode_unlock(inode);
>
> Why is this is done (inode_lock + setting to NULL), and if so, why
> it wasn't done in 1/7?
I'm sure I've explained this before, but again, the inode lock is
a work around for lack of removal fencing in securityfs. We null the
i_private here during remove and test for null during open under this
lock.
This design ensures that open either safely gets a kref or fails.
Holding the kref as long as the securityfs file is open by userspace
ensures there are no use-after-frees in this code.
A comment to that effect would be good.
Jason
------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
