Re: [PATCH 1/1] driver: Tpm hardware enablement

[Kylie Hall <kjhall@us.ibm.com>]

On Thu, 2004-12-09 at 09:48, Arjan van de Ven wrote: 
> On Thu, 2004-12-09 at 09:25 -0600, Kylene Hall wrote:
> > +	/* wait for status */
> > +	add_timer(&status_timer);
> > +	do {
> > +		schedule();
> > +		*data = inb(chip->base + 1);
> > +		if ((*data & mask) == val) {
> > +			del_singleshot_timer_sync(&status_timer);
> > +			return 0;
> > +		}
> > +	} while (!expired);
> 
> this is busy waiting. Can't you do it with msleep() or some such ?
> Or like 100 iterations without delays (in case the chip returns fast),
> and then start sleeping, but please do sleep for a real time, not just
> yield the cpu. Powermanagement and lots of other things really like to
> see that.
I don't see a problem with changing the schedule to an msleep.  I'll
change it.

> > +	/* wait for status */
> > +	add_timer(&status_timer);
> > +	do {
> > +		schedule();
> > +		status = inb(chip->base + NSC_STATUS);
> > +		if (status & NSC_STATUS_OBF)
> > +			status = inb(chip->base + NSC_DATA);
> > +		if (status & NSC_STATUS_RDY) {
> > +			del_singleshot_timer_sync(&status_timer);
> > +			return 0;
> > +		}
> > +	} while (!expired);
> 
> same comment. Also the timer handling looks suspect... can you guarantee
> 100% sure that the timer is gone when the while falls through ?
> 
Since the only way that expired becomes non-zero is for the timer to
expire and since the function that the timer calls on expiration does
not restart the timer the timer will be gone when the while falls
through.

> 
> > +	chip->userspace_buffer =
> > +	    kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> 
> that sounds like a really deceptive name to me ... since it's kernel
> memory ;)
Agreed.  It will be changed.

> > +static int tpm_release(struct inode *inode, struct file *file)
> > +{
> > +	struct tpm_chip *chip = file->private_data;
> > +
> > +	if (chip == NULL)
> > +		return -ENODEV;
> > +
> > +	spin_lock(&driver_lock);
> > +	chip->num_opens--;
> 
> why do you need to keep track of the number of openers? Can't you have
> the kernel fs layer keep track of this ?
The specification only allows the Trusted Software Stack (TSS)
application to open/read/write to the device.  I am keeping track of the
number of opens to only allow one open (for the TSS).

> > +	chip->user_read_timer.function = user_reader_timeout;
> > +	chip->user_read_timer.data = (unsigned long) chip;
> > +	chip->user_read_timer.expires = jiffies + (60 * HZ);
> > +	add_timer(&chip->user_read_timer);
> > +
> > +	atomic_set(&chip->data_pending, out_size);
> > +
> > +	return size;
> 
> what prevents the module from being unloaded ?
> (eg user calls write(); close(); and then does rmmod before the timer
> expires )
> 
You're right this is a problem.  I think the solution is to put locks around the timer 
manipulation code and make a call to timer_pending in the close function.  If the timer
is pending I will then call del_timer from close.  Sound reasonable?
> 
> > +/*
> > + * Resume from a power safe. The BIOS already restored
> > + * the TPM state.
> > + */
> 
> are there any special security things needed after resume ?
> Or maybe at suspend time, to wipe secrets from the TPM or somesuch..
> 
Nothing that I am aware of.

Thanks,
Kylene