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

Arjan van de Ven
2004-12-09T15:49:48Z

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.
> + /* 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 ?

> + chip->userspace_buffer =
> + kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);

that sounds like a really deceptive name to me ... since it's kernel
memory ;)

> +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 ?
> + 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 )

> +/*
> + * 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..