Re: [tpmdd-devel] Re: [PATCH 1/1] tpm: update tpm sysfs file ownership - updated version

[Chris Wright <chrisw@osdl.org>]

* Kylene Hall (kjhall@us.ibm.com) wrote:
> diff -uprN linux-2.6.10/drivers/char/tpm/tpm_atmel.c linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c
> --- linux-2.6.10/drivers/char/tpm/tpm_atmel.c	2005-02-04 15:03:03.000000000 -0600
> +++ linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c	2005-02-09 14:12:30.711621784 -0600
> @@ -131,6 +131,7 @@ static struct tpm_vendor_specific tpm_at
>  	.req_complete_mask = ATML_STATUS_BUSY | ATML_STATUS_DATA_AVAIL,
>  	.req_complete_val = ATML_STATUS_DATA_AVAIL,
>  	.base = TPM_ATML_BASE,
> +	.attr = TPM_DEVICE_ATTRS,
>  	.miscdev.fops = &atmel_ops,
>  };
>  
> diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c linux-2.6.10-tpm/drivers/char/tpm/tpm.c
> --- linux-2.6.10/drivers/char/tpm/tpm.c	2005-02-04 15:03:03.000000000 -0600
> +++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c	2005-02-09 14:12:30.695624216 -0600
> @@ -213,7 +213,7 @@ static u8 pcrread[] = {
>  	0, 0, 0, 0		/* PCR index */
>  };
>  
> -static ssize_t show_pcrs(struct device *dev, char *buf)
> +ssize_t show_pcrs(struct device *dev, char *buf)

This is too generic a name for global namespace.

>  {
>  	u8 data[READ_PCR_RESULT_SIZE];
>  	ssize_t len;
> @@ -245,8 +245,7 @@ static ssize_t show_pcrs(struct device *
>  	}
>  	return str - buf;
>  }
> -
> -static DEVICE_ATTR(pcrs, S_IRUGO, show_pcrs, NULL);
> +EXPORT_SYMBOL_GPL(show_pcrs);
>  
>  #define  READ_PUBEK_RESULT_SIZE 314
>  static u8 readpubek[] = {
> @@ -255,7 +254,7 @@ static u8 readpubek[] = {
>  	0, 0, 0, 124,		/* TPM_ORD_ReadPubek */
>  };
>  
> -static ssize_t show_pubek(struct device *dev, char *buf)
> +ssize_t show_pubek(struct device *dev, char *buf)

same here

>  {
>  	u8 data[READ_PUBEK_RESULT_SIZE];
>  	ssize_t len;
> @@ -308,7 +307,7 @@ static ssize_t show_pubek(struct device 
>  	return str - buf;
>  }
>  
> -static DEVICE_ATTR(pubek, S_IRUGO, show_pubek, NULL);
> +EXPORT_SYMBOL_GPL(show_pubek);
>  
>  #define CAP_VER_RESULT_SIZE 18
>  static u8 cap_version[] = {
> @@ -329,7 +328,7 @@ static u8 cap_manufacturer[] = {
>  	0, 0, 1, 3
>  };
>  
> -static ssize_t show_caps(struct device *dev, char *buf)
> +ssize_t show_caps(struct device *dev, char *buf)

and here.

>  {
>  	u8 data[READ_PUBEK_RESULT_SIZE];
>  	ssize_t len;
> @@ -362,7 +361,26 @@ static ssize_t show_caps(struct device *
>  	return str - buf;
>  }
>  
> -static DEVICE_ATTR(caps, S_IRUGO, show_caps, NULL);
> +EXPORT_SYMBOL_GPL(show_caps);
> +
> +ssize_t store_cancel(struct device *dev, const char *buf,
> +			    size_t count)

and here

> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	if (chip == NULL)
> +		return 0;
> +

Do you want any extra protection besides mode bits (S_IWUSR | S_IWGRP)?
How privileged should this operation be?

> +	chip->vendor->cancel(chip);
> +
> +	down(&chip->timer_manipulation_mutex);
> +	if (timer_pending(&chip->device_timer))
> +		mod_timer(&chip->device_timer, jiffies);
> +	up(&chip->timer_manipulation_mutex);
> +
> +	return count;
> +}
> +
> +EXPORT_SYMBOL_GPL(store_cancel);
>  
>  /*
>   * Device file system interface to the TPM
> @@ -524,6 +542,7 @@ EXPORT_SYMBOL_GPL(tpm_read);
>  void tpm_remove_hardware(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	int i;
>  
>  	if (chip == NULL) {
>  		dev_err(dev, "No device data found\n");
> @@ -539,9 +558,8 @@ void tpm_remove_hardware(struct device *
>  	dev_set_drvdata(dev, NULL);
>  	misc_deregister(&chip->vendor->miscdev);
>  
> -	device_remove_file(dev, &dev_attr_pubek);
> -	device_remove_file(dev, &dev_attr_pcrs);
> -	device_remove_file(dev, &dev_attr_caps);
> +	for ( i = 0; i < TPM_NUM_ATTR; i++ ) 
> +		device_remove_file(dev, &chip->vendor->attr[i]);
>  
>  	dev_mask[chip->dev_num / 32] &= !(1 << (chip->dev_num % 32));
>  
> @@ -663,10 +681,9 @@ dev_num_search_complete:
>  
>  	list_add(&chip->list, &tpm_chip_list);
>  
> -	device_create_file(dev, &dev_attr_pubek);
> -	device_create_file(dev, &dev_attr_pcrs);
> -	device_create_file(dev, &dev_attr_caps);
> -
> +	for ( i = 0; i < TPM_NUM_ATTR; i++ ) 
> +		device_create_file(dev, &chip->vendor->attr[i]);		
> +		
>  	return 0;
>  }
>  
> diff -uprN linux-2.6.10/drivers/char/tpm/tpm.h linux-2.6.10-tpm/drivers/char/tpm/tpm.h
> --- linux-2.6.10/drivers/char/tpm/tpm.h	2005-02-04 15:03:03.000000000 -0600
> +++ linux-2.6.10-tpm/drivers/char/tpm/tpm.h	2005-02-09 14:12:30.702623152 -0600
> @@ -25,11 +25,23 @@
>  #include <linux/miscdevice.h>
>  
>  #define TPM_TIMEOUT msecs_to_jiffies(5)
> +#define TPM_NUM_ATTR 4
>  
>  /* TPM addresses */
>  #define	TPM_ADDR			0x4E
>  #define	TPM_DATA			0x4F
>  
> +extern ssize_t show_pubek(struct device *, char *);
> +extern ssize_t show_pcrs(struct device *, char *);
> +extern ssize_t show_caps(struct device *, char *);
> +extern ssize_t store_cancel(struct device *, const char *, size_t);
> +
> +#define TPM_DEVICE_ATTRS { \
> +	__ATTR(pubek, S_IRUGO, show_pubek, NULL), \
> +	__ATTR(pcrs, S_IRUGO, show_pcrs, NULL), \
> +	__ATTR(caps, S_IRUGO, show_caps, NULL), \
> +	__ATTR(cancel, S_IWUSR | S_IWGRP, NULL, store_cancel) }

This doesn't look like the right way to go.  

> +
>  struct tpm_chip;
>  
>  struct tpm_vendor_specific {
> @@ -42,6 +54,7 @@ struct tpm_vendor_specific {
>  	void (*cancel) (struct tpm_chip *);
>  	u8 (*status) (struct tpm_chip *);
>  	struct miscdevice miscdev;
> +	struct device_attribute attr[TPM_NUM_ATTR];

So every device will have the same attrs?  If so, make that whole struct
exported (not the individual show/store methods) and reference that in
each driver.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net