Re: [PATCH] Add TPM hardware enablement driver

[Kylene Jo Hall <kjhall@us.ibm.com>]

Thanks for the helpful comments I am working on a patch to fix your
concerns but I have a couple of questions.  

On Wed, 2005-03-09 at 22:51 -0500, Jeff Garzik wrote:
<snip>

> > +	down(&chip->timer_manipulation_mutex);
> > +	chip->time_expired = 0;
> > +	init_timer(&chip->device_timer);
> > +	chip->device_timer.function = tpm_time_expired;
> > +	chip->device_timer.expires = jiffies + 2 * 60 * HZ;
> > +	chip->device_timer.data = (unsigned long) &chip->time_expired;
> > +	add_timer(&chip->device_timer);
> 
> very wrong.  you init_timer() when you initialize 'chip'... once.  then 
> during the device lifetime you add/mod/del the timer.
> 
> calling init_timer() could lead to corruption of state.
> 
> > +	up(&chip->timer_manipulation_mutex);
When calling mod_timer and an occasional del_singleshot_timer_sync is it
necessary to protect this with a mutex like I was doing or not?


> > +	pci_dev_get(chip->pci_dev);
> > +
> > +	spin_unlock(&driver_lock);
> > +
> > +	chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> > +	if (chip->data_buffer == NULL) {
> > +		chip->num_opens--;
> > +		pci_dev_put(chip->pci_dev);
> > +		return -ENOMEM;
> > +	}
> 
> what is the purpose of this pci_dev_get/put?  attempting to prevent 
> hotplug or something?

We were doing reference counting since their is a pci_dev in the chip
structure which set to the file->private_data pointer. Not correct?

> 
> > +	}
> > +
> > +	down(&chip->buffer_mutex);
> > +
> > +	if (in_size > TPM_BUFSIZE)
> > +		in_size = TPM_BUFSIZE;
> > +
> > +	if (copy_from_user
> > +	    (chip->data_buffer, (void __user *) buf, in_size)) {
> > +		up(&chip->buffer_mutex);
> > +		return -EFAULT;
> > +	}
> > +
> > +	/* atomic tpm command send and result receive */
> > +	out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);
> 
> major bug?  in_size may be smaller than TPM_BUFSIZE
> 
Yes in_size might be but the chip->data_buffer will always be this size since it is malloc'd in open.  The operation needs to be atomic so the tpm_transmit function is sending the command to the device and receiving the result which might be bigger than in_size.  The result is reported back to userspace from this buffer on a read.

> > +
> > +ssize_t tpm_read(struct file * file, char __user * buf,
> > +		 size_t size, loff_t * off)
> > +{
> > +	struct tpm_chip *chip = file->private_data;
> > +	int ret_size = -ENODATA;
> > +
> > +	if (atomic_read(&chip->data_pending) != 0) {	/* Result available */
> > +		down(&chip->timer_manipulation_mutex);
> > +		del_singleshot_timer_sync(&chip->user_read_timer);
> > +		up(&chip->timer_manipulation_mutex);
> > +
> > +		down(&chip->buffer_mutex);
> > +
> > +		ret_size = atomic_read(&chip->data_pending);
> > +		atomic_set(&chip->data_pending, 0);
> > +
> > +		if (ret_size == 0)	/* timeout just occurred */
> > +			ret_size = -ETIME;
> > +		else if (ret_size > 0) {	/* relay data */
> > +			if (size < ret_size)
> > +				ret_size = size;
> > +
> > +			if (copy_to_user((void __user *) buf,
> > +					 chip->data_buffer, ret_size)) {
> > +				ret_size = -EFAULT;
> > +			}
> > +		}
> > +		up(&chip->buffer_mutex);
> > +	}
> > +
> > +	return ret_size;
> 
> POSIX violation -- when there is no data available, returning a 
> non-standard error is silly
> 
So read should just return 0 if no data available?


Thanks,
Kylie