diff mbox

[tpmdd-devel,v1,12/12] tpm: TPM2 sysfs attributes

Message ID 1411549562-24242-13-git-send-email-jarkko.sakkinen@linux.intel.com
State Superseded, archived
Headers show

Commit Message

Jarkko Sakkinen Sept. 24, 2014, 9:06 a.m. UTC
Added tpm2-sysfs.c that implements sysfs attributes for a TPM2
device.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/Makefile        |   2 +-
 drivers/char/tpm/tpm-chip.c      |  10 +-
 drivers/char/tpm/tpm-interface.c |   1 +
 drivers/char/tpm/tpm2-commands.c |   2 +-
 drivers/char/tpm/tpm2-sysfs.c    | 242 +++++++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm2.h          |  31 +++++
 6 files changed, 284 insertions(+), 4 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2-sysfs.c

Comments

Jason Gunthorpe Sept. 24, 2014, 5:13 p.m. UTC | #1
On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote:
> Added tpm2-sysfs.c that implements sysfs attributes for a TPM2
> device.

You need to document in Documentation every new sysfs that is added.

The existing ones are not documented :(

> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -899,6 +899,7 @@ void tpm_remove_hardware(struct device *dev)
>  
>  	tpm_chip_unregister(chip);
>  
> +
>  	/* write it this way to be explicit (chip->dev == dev) */
>  	put_device(chip->dev);
>  }

Hunk does not belong in this patch

> diff --git a/drivers/char/tpm/tpm2-commands.c b/drivers/char/tpm/tpm2-commands.c
> index a21dfd5..6365087 100644
> +++ b/drivers/char/tpm/tpm2-commands.c
> @@ -195,7 +195,7 @@ ssize_t tpm2_get_tpm_pt(struct device *dev, u32 property_id,  u32* value,
>  	cmd.header.in = tpm2_get_tpm_pt_header;
>  	cmd.params.tpm2_get_tpm_pt_in.cap_id =
>  		cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
> -	cmd.params.tpm2_get_tpm_pt_in.property_id = property_id;
> +	cmd.params.tpm2_get_tpm_pt_in.property_id = cpu_to_be32(property_id);
>  	cmd.params.tpm2_get_tpm_pt_in.property_cnt = cpu_to_be32(1);

Hunk does not belong in this patch

> +static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{

The pcrs file never conformed to the sysfs rules, if TPM2 is getting a
whole new file set, I wouldn't mind seeing it not include the
non-conformant ones. What do you think?

> +static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{

Ditto.. The manfacturer number should probably be its own file

> +static ssize_t durations_show(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +
> +	if (chip->vendor.duration[TPM_LONG] == 0)
> +		return 0;
> +
> +	return sprintf(buf, "%d %d %d [%s]\n",
> +		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
> +		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
> +		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
> +		       chip->vendor.duration_adjusted
> +		       ? "adjusted" : "original");
> +}
> +static DEVICE_ATTR_RO(durations);

Seem useless since the durations are constant, drop it?

> +static ssize_t timeouts_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d %d %d %d [%s]\n",
> +		       jiffies_to_usecs(chip->vendor.timeout_a),
> +		       jiffies_to_usecs(chip->vendor.timeout_b),
> +		       jiffies_to_usecs(chip->vendor.timeout_c),
> +		       jiffies_to_usecs(chip->vendor.timeout_d),
> +		       chip->vendor.timeout_adjusted
> +		       ? "adjusted" : "original");
> +}
> +static DEVICE_ATTR_RO(timeouts);

Ditto

> +static ssize_t version_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	char *str = buf;
> +
> +	str += sprintf(str, "2.0\n");
> +
> +	return str - buf;
> +}
> +
> +static DEVICE_ATTR_RO(version);
> +
> +
> +static ssize_t tpm2_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", chip->tpm2);
> +}
> +static DEVICE_ATTR_RO(tpm2);

Two things for the same report? Drop one?

Also, I think some thought is needed for the char interface - some
kind of ioctl to enter TPM2 mode and EINVAL access until that is done?

Finally, this is in the wrong place in sysfs, I suspect it should be
attached to the char device node, not the platform device node? We
should at least investigate this...

> +struct tpm2_permanent {
> +	unsigned int owner_auth_set		: 1;
> +	unsigned int endorsement_auth_set	: 1;
> +	unsigned int lockout_auth_set		: 1;
> +	unsigned int reserved1			: 5;
> +	unsigned int disable_clear		: 1;
> +	unsigned int in_lockout			: 1;
> +	unsigned int tpm_generated_eps		: 1;
> +	unsigned int reserved2			: 21;
> +};
> +
> +struct tpm2_startup_clear {
> +	unsigned int ph_enable			: 1;
> +	unsigned int sh_enable			: 1;
> +	unsigned int eh_enable			: 1;
> +	unsigned int ph_enable_nv		: 1;
> +	unsigned int reserved			: 27;
> +	unsigned int orderly			: 1;
> +};

This idea is not portable, you cannot use bitfields to index bits in a
word like this. Please use constants defined with BIT(xx)

Next posting can you include a github link? Thanks

Jason

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Stefan Berger Sept. 24, 2014, 5:34 p.m. UTC | #2
On 09/24/2014 01:13 PM, Jason Gunthorpe wrote:
> On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote:
>> +static ssize_t durations_show(struct device *dev, struct device_attribute *attr,
>> +			      char *buf)
>> +{
>> +	struct tpm_chip *chip = dev_get_drvdata(dev);
>> +
>> +	if (chip->vendor.duration[TPM_LONG] == 0)
>> +		return 0;
>> +
>> +	return sprintf(buf, "%d %d %d [%s]\n",
>> +		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
>> +		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
>> +		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
>> +		       chip->vendor.duration_adjusted
>> +		       ? "adjusted" : "original");
>> +}
>> +static DEVICE_ATTR_RO(durations);
> Seem useless since the durations are constant, drop it?

We show them for TPM 1.2 as well, so I'd keep them fo TPM2.

>
>> +static ssize_t timeouts_show(struct device *dev, struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct tpm_chip *chip = dev_get_drvdata(dev);
>> +
>> +	return sprintf(buf, "%d %d %d %d [%s]\n",
>> +		       jiffies_to_usecs(chip->vendor.timeout_a),
>> +		       jiffies_to_usecs(chip->vendor.timeout_b),
>> +		       jiffies_to_usecs(chip->vendor.timeout_c),
>> +		       jiffies_to_usecs(chip->vendor.timeout_d),
>> +		       chip->vendor.timeout_adjusted
>> +		       ? "adjusted" : "original");
>> +}
>> +static DEVICE_ATTR_RO(timeouts);

With all the problems we had with TPM 1.2 TPM's wrong timeouts and 
showing them in sysfs, why not show them for TPM2 as well?


------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jason Gunthorpe Sept. 24, 2014, 5:59 p.m. UTC | #3
On Wed, Sep 24, 2014 at 01:34:11PM -0400, Stefan Berger wrote:
> On 09/24/2014 01:13 PM, Jason Gunthorpe wrote:
> >On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote:
> >>+static ssize_t durations_show(struct device *dev, struct device_attribute *attr,
> >>+			      char *buf)
> >>+{
> >>+	struct tpm_chip *chip = dev_get_drvdata(dev);
> >>+
> >>+	if (chip->vendor.duration[TPM_LONG] == 0)
> >>+		return 0;
> >>+
> >>+	return sprintf(buf, "%d %d %d [%s]\n",
> >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
> >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
> >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
> >>+		       chip->vendor.duration_adjusted
> >>+		       ? "adjusted" : "original");
> >>+}
> >>+static DEVICE_ATTR_RO(durations);
> >Seem useless since the durations are constant, drop it?
> 
> We show them for TPM 1.2 as well, so I'd keep them fo TPM2.

The durations are constant and hardwired in the driver for TPM2, and
the sysfs file format does not follow the one-value-per-file
rule.

So it doesn't display anything useful. In TPM2 mode all the timeouts
are constant and known, so I'd rather see it go away.

> With all the problems we had with TPM 1.2 TPM's wrong timeouts and
> showing them in sysfs, why not show them for TPM2 as well?

We had problems with devices reporting the wrong timeout from their
cap queries - that isn't possible in TPM2.

Both of these values should live in debugfs anyhow.

It would be nice to start with a fresh set of correct sysfs files for
TPM2, since essentially everything is different about the userspace
interface we can safely make a breaking change here.

Jason

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Peter Hüwe Sept. 24, 2014, 6:36 p.m. UTC | #4
Am Mittwoch, 24. September 2014, 19:13:38 schrieb Jason Gunthorpe:
> On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote:
> > Added tpm2-sysfs.c that implements sysfs attributes for a TPM2
> > device.
> 
> You need to document in Documentation every new sysfs that is added.
> 
> The existing ones are not documented :(

For 1.2 - they are.
Documentation/ABI/stable/sysfs-class-tpm
So we also now what our stable sysfs abi is :)



Peter


------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jarkko Sakkinen Sept. 24, 2014, 6:50 p.m. UTC | #5
On Wed, Sep 24, 2014 at 11:59:40AM -0600, Jason Gunthorpe wrote:
> On Wed, Sep 24, 2014 at 01:34:11PM -0400, Stefan Berger wrote:
> > On 09/24/2014 01:13 PM, Jason Gunthorpe wrote:
> > >On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote:
> > >>+static ssize_t durations_show(struct device *dev, struct device_attribute *attr,
> > >>+			      char *buf)
> > >>+{
> > >>+	struct tpm_chip *chip = dev_get_drvdata(dev);
> > >>+
> > >>+	if (chip->vendor.duration[TPM_LONG] == 0)
> > >>+		return 0;
> > >>+
> > >>+	return sprintf(buf, "%d %d %d [%s]\n",
> > >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
> > >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
> > >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
> > >>+		       chip->vendor.duration_adjusted
> > >>+		       ? "adjusted" : "original");
> > >>+}
> > >>+static DEVICE_ATTR_RO(durations);
> > >Seem useless since the durations are constant, drop it?
> > 
> > We show them for TPM 1.2 as well, so I'd keep them fo TPM2.
> 
> The durations are constant and hardwired in the driver for TPM2, and
> the sysfs file format does not follow the one-value-per-file
> rule.
> 
> So it doesn't display anything useful. In TPM2 mode all the timeouts
> are constant and known, so I'd rather see it go away.
> 
> > With all the problems we had with TPM 1.2 TPM's wrong timeouts and
> > showing them in sysfs, why not show them for TPM2 as well?
> 
> We had problems with devices reporting the wrong timeout from their
> cap queries - that isn't possible in TPM2.
> 
> Both of these values should live in debugfs anyhow.
> 
> It would be nice to start with a fresh set of correct sysfs files for
> TPM2, since essentially everything is different about the userspace
> interface we can safely make a breaking change here.

I'll drop timeouts and durations. It's easy to add them later if they
are needed.

> Jason

/Jarkko

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jarkko Sakkinen Sept. 24, 2014, 7:02 p.m. UTC | #6
On Wed, Sep 24, 2014 at 11:13:38AM -0600, Jason Gunthorpe wrote:
> On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote:
> > Added tpm2-sysfs.c that implements sysfs attributes for a TPM2
> > device.
> 
> You need to document in Documentation every new sysfs that is added.
> 
> The existing ones are not documented :(

This came up in Peters reply (you probably saw that).

https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-class-tpm

> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -899,6 +899,7 @@ void tpm_remove_hardware(struct device *dev)
> >  
> >  	tpm_chip_unregister(chip);
> >  
> > +
> >  	/* write it this way to be explicit (chip->dev == dev) */
> >  	put_device(chip->dev);
> >  }
> 
> Hunk does not belong in this patch

Ack (thanks).

> > diff --git a/drivers/char/tpm/tpm2-commands.c b/drivers/char/tpm/tpm2-commands.c
> > index a21dfd5..6365087 100644
> > +++ b/drivers/char/tpm/tpm2-commands.c
> > @@ -195,7 +195,7 @@ ssize_t tpm2_get_tpm_pt(struct device *dev, u32 property_id,  u32* value,
> >  	cmd.header.in = tpm2_get_tpm_pt_header;
> >  	cmd.params.tpm2_get_tpm_pt_in.cap_id =
> >  		cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
> > -	cmd.params.tpm2_get_tpm_pt_in.property_id = property_id;
> > +	cmd.params.tpm2_get_tpm_pt_in.property_id = cpu_to_be32(property_id);
> >  	cmd.params.tpm2_get_tpm_pt_in.property_cnt = cpu_to_be32(1);
> 
> Hunk does not belong in this patch

Ack (thanks).

> > +static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> 
> The pcrs file never conformed to the sysfs rules, if TPM2 is getting a
> whole new file set, I wouldn't mind seeing it not include the
> non-conformant ones. What do you think?

I think that it's better to put extra focus on these sysfs attributes in
first patch set because it's user space visible. What's wrong in the
current pcrs file?

> > +static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> 
> Ditto.. The manfacturer number should probably be its own file

Maybe here would make sense to have three files:

- manufacturer
- firmware_1
- firmware_2

More or less following the name of the TPM properties in the
specification.

> > +static ssize_t durations_show(struct device *dev, struct device_attribute *attr,
> > +			      char *buf)
> > +{
> > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +
> > +	if (chip->vendor.duration[TPM_LONG] == 0)
> > +		return 0;
> > +
> > +	return sprintf(buf, "%d %d %d [%s]\n",
> > +		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
> > +		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
> > +		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
> > +		       chip->vendor.duration_adjusted
> > +		       ? "adjusted" : "original");
> > +}
> > +static DEVICE_ATTR_RO(durations);
> 
> Seem useless since the durations are constant, drop it?
> 
> > +static ssize_t timeouts_show(struct device *dev, struct device_attribute *attr,
> > +			     char *buf)
> > +{
> > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%d %d %d %d [%s]\n",
> > +		       jiffies_to_usecs(chip->vendor.timeout_a),
> > +		       jiffies_to_usecs(chip->vendor.timeout_b),
> > +		       jiffies_to_usecs(chip->vendor.timeout_c),
> > +		       jiffies_to_usecs(chip->vendor.timeout_d),
> > +		       chip->vendor.timeout_adjusted
> > +		       ? "adjusted" : "original");
> > +}
> > +static DEVICE_ATTR_RO(timeouts);
> 
> Ditto
> 
> > +static ssize_t version_show(struct device *dev, struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	char *str = buf;
> > +
> > +	str += sprintf(str, "2.0\n");
> > +
> > +	return str - buf;
> > +}
> > +
> > +static DEVICE_ATTR_RO(version);
> > +
> > +
> > +static ssize_t tpm2_show(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> > +	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%d\n", chip->tpm2);
> > +}
> > +static DEVICE_ATTR_RO(tpm2);
> 
> Two things for the same report? Drop one?
> 
> Also, I think some thought is needed for the char interface - some
> kind of ioctl to enter TPM2 mode and EINVAL access until that is done?
> 
> Finally, this is in the wrong place in sysfs, I suspect it should be
> attached to the char device node, not the platform device node? We
> should at least investigate this...

This was forgotten. Should not be here at all. Instead we have the
variable 'version' to state specification family and level.

I did not fully understand the comment about tpm2 flag. Why driver
cannot set it when it initializes the device like with this based
on value of the STS3?

> > +struct tpm2_permanent {
> > +	unsigned int owner_auth_set		: 1;
> > +	unsigned int endorsement_auth_set	: 1;
> > +	unsigned int lockout_auth_set		: 1;
> > +	unsigned int reserved1			: 5;
> > +	unsigned int disable_clear		: 1;
> > +	unsigned int in_lockout			: 1;
> > +	unsigned int tpm_generated_eps		: 1;
> > +	unsigned int reserved2			: 21;
> > +};
> > +
> > +struct tpm2_startup_clear {
> > +	unsigned int ph_enable			: 1;
> > +	unsigned int sh_enable			: 1;
> > +	unsigned int eh_enable			: 1;
> > +	unsigned int ph_enable_nv		: 1;
> > +	unsigned int reserved			: 27;
> > +	unsigned int orderly			: 1;
> > +};
> 
> This idea is not portable, you cannot use bitfields to index bits in a
> word like this. Please use constants defined with BIT(xx)

Thanks, I'll change this.

> Next posting can you include a github link? Thanks

Yup sure. I do everything for v2 in tpm2-v1 by adding fixes on top of 
these patches. When things look good there I'll create a new branch
tpm2-v2 and prepare the next patch set.

> Jason

/Jarkko

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jason Gunthorpe Sept. 24, 2014, 8:19 p.m. UTC | #7
On Wed, Sep 24, 2014 at 10:02:34PM +0300, Jarkko Sakkinen wrote:

> > The pcrs file never conformed to the sysfs rules, if TPM2 is getting a
> > whole new file set, I wouldn't mind seeing it not include the
> > non-conformant ones. What do you think?
> 
> I think that it's better to put extra focus on these sysfs attributes in
> first patch set because it's user space visible. What's wrong in the
> current pcrs file?

Each PCR should be a distinct sysfs file, probably with a
directory. One Value Per File is the rule.

> > > +static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
> > > +			 char *buf)
> > > +{
> > 
> > Ditto.. The manfacturer number should probably be its own file
> 
> Maybe here would make sense to have three files:
> 
> - manufacturer
> - firmware_1
> - firmware_2
> 
> More or less following the name of the TPM properties in the
> specification.

Probably, maybe firmware_1/2 could be combined if they are the same
logical value? (I've always expressed it as firmware_1.firwmare_2?)

> I did not fully understand the comment about tpm2 flag. Why driver
> cannot set it when it initializes the device like with this based
> on value of the STS3?

I was talking about the /dev/ char device - a random application today
will open it and send TPM1 formed messages. Those should be refused
with EINVAL for a TPM2 chip unless the application declares via IOCTL
that it will be sending TPM2 messages.

Otherwise the API contract for the /dev/ device (write TPM1 formed
messages) is broken..

Jason

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Peter Hüwe Sept. 24, 2014, 8:35 p.m. UTC | #8
Am Mittwoch, 24. September 2014, 22:19:38 schrieb Jason Gunthorpe:
> On Wed, Sep 24, 2014 at 10:02:34PM +0300, Jarkko Sakkinen wrote:
> > > The pcrs file never conformed to the sysfs rules, if TPM2 is getting a
> > > whole new file set, I wouldn't mind seeing it not include the
> > > non-conformant ones. What do you think?
> > 
> > I think that it's better to put extra focus on these sysfs attributes in
> > first patch set because it's user space visible. What's wrong in the
> > current pcrs file?
> 
> Each PCR should be a distinct sysfs file, probably with a
> directory. One Value Per File is the rule.

That would be 24*2 files only for pcrs... 
Documentation/filesystems/sysfs.txt says:

"
Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type. "

So it would be more or less o.k. to have it in one file like we had.


Then however:
"Mixing types, expressing multiple lines of data, and doing fancy
formatting of data is heavily frowned upon. Doing these things may get
you publicly humiliated and your code rewritten without notice."


Do we really need the PCRs as sysfs files?
I know they are handy as a dev, but does any application actually use this 
directly?


Thanks,
Peter




------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Peter Hüwe Sept. 24, 2014, 8:39 p.m. UTC | #9
Am Mittwoch, 24. September 2014, 19:59:40 schrieb Jason Gunthorpe:
> On Wed, Sep 24, 2014 at 01:34:11PM -0400, Stefan Berger wrote:
> > On 09/24/2014 01:13 PM, Jason Gunthorpe wrote:
> > >On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote:
> > >>+static ssize_t durations_show(struct device *dev, struct
> > >>device_attribute *attr, +			      char *buf)
> > >>+{
> > >>+	struct tpm_chip *chip = dev_get_drvdata(dev);
> > >>+
> > >>+	if (chip->vendor.duration[TPM_LONG] == 0)
> > >>+		return 0;
> > >>+
> > >>+	return sprintf(buf, "%d %d %d [%s]\n",
> > >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
> > >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
> > >>+		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
> > >>+		       chip->vendor.duration_adjusted
> > >>+		       ? "adjusted" : "original");
> > >>+}
> > >>+static DEVICE_ATTR_RO(durations);
> > >
> > >Seem useless since the durations are constant, drop it?
> > 
> > We show them for TPM 1.2 as well, so I'd keep them fo TPM2.
> 
> The durations are constant and hardwired in the driver for TPM2, and
> the sysfs file format does not follow the one-value-per-file
> rule.
> 
> So it doesn't display anything useful. In TPM2 mode all the timeouts
> are constant and known, so I'd rather see it go away.


If it is more or less a no-op since we have set chip-
>vendor.duration[TPM_SHORT] for other code to work, we can show the values for 
TPM2.0.

However I think we don't need any extra code to show or hide the sysfs files 
for TPM2.0.

Peter

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jason Gunthorpe Sept. 24, 2014, 8:46 p.m. UTC | #10
On Wed, Sep 24, 2014 at 10:35:42PM +0200, Peter Hüwe wrote:
> Am Mittwoch, 24. September 2014, 22:19:38 schrieb Jason Gunthorpe:
> > On Wed, Sep 24, 2014 at 10:02:34PM +0300, Jarkko Sakkinen wrote:
> > > > The pcrs file never conformed to the sysfs rules, if TPM2 is getting a
> > > > whole new file set, I wouldn't mind seeing it not include the
> > > > non-conformant ones. What do you think?
> > > 
> > > I think that it's better to put extra focus on these sysfs attributes in
> > > first patch set because it's user space visible. What's wrong in the
> > > current pcrs file?
> > 
> > Each PCR should be a distinct sysfs file, probably with a
> > directory. One Value Per File is the rule.
> 
> That would be 24*2 files only for pcrs...

Some subsystems do just that..

$ ls /sys/class/infiniband/qib0/ports/1/sl2vl/
0  1  10  11  12  13  14  15  2  3  4  5  6  7  8  9

> Documentation/filesystems/sysfs.txt says:
> 
> "
> Attributes should be ASCII text files, preferably with only one value
> per file. It is noted that it may not be efficient to contain only one
> value per file, so it is socially acceptable to express an array of
> values of the same type. "
> 
> So it would be more or less o.k. to have it in one file like we had.
> 
> Then however:
> "Mixing types, expressing multiple lines of data, and doing fancy
> formatting of data is heavily frowned upon. Doing these things may get
> you publicly humiliated and your code rewritten without notice."

I think taken together that says an array of 128 bit PCR hex values
without new lines or other formatting would be OK. But the breakdown
and fancy formatting we do is not OK.

> Do we really need the PCRs as sysfs files?  I know they are handy as
> a dev, but does any application actually use this directly?

No idea, but using tpm2 to find out seems like a reasonable idea,
especially if the pcr meaning changes in some way with TPM2 ..

Jason

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jason Gunthorpe Sept. 24, 2014, 8:50 p.m. UTC | #11
On Wed, Sep 24, 2014 at 10:39:41PM +0200, Peter Hüwe wrote:
 
> If it is more or less a no-op since we have set
> chip->vendor.duration[TPM_SHORT] for other code to work, we can show
> the values for TPM2.0.

It is not a no-op, Jarkko created a whole new file for the tpm2 sysfs
interface, so the two timeout prints are duplicated code from the tpm1
sysfs interface.

Mind you, not sure if we need separate sysfs files ...

Jason

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jarkko Sakkinen Sept. 26, 2014, 5:19 p.m. UTC | #12
On Wed, Sep 24, 2014 at 02:46:27PM -0600, Jason Gunthorpe wrote:
> > That would be 24*2 files only for pcrs...
> 
> Some subsystems do just that..
> 
> $ ls /sys/class/infiniband/qib0/ports/1/sl2vl/
> 0  1  10  11  12  13  14  15  2  3  4  5  6  7  8  9

They use static structures in drivers/infiniband/hw/qib/qib_sysfs.c
and it does not looks a mess. I would prefer to create struct attribute
entries dynamically if there's clean and easy way to do that.

/Jarkko

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jarkko Sakkinen Sept. 30, 2014, 8:07 p.m. UTC | #13
On Fri, Sep 26, 2014 at 08:19:47PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 24, 2014 at 02:46:27PM -0600, Jason Gunthorpe wrote:
> > > That would be 24*2 files only for pcrs...
> > 
> > Some subsystems do just that..
> > 
> > $ ls /sys/class/infiniband/qib0/ports/1/sl2vl/
> > 0  1  10  11  12  13  14  15  2  3  4  5  6  7  8  9
> 
> They use static structures in drivers/infiniband/hw/qib/qib_sysfs.c
> and it does not looks a mess. I would prefer to create struct attribute
> entries dynamically if there's clean and easy way to do that.

I gave this a shot:

https://github.com/jsakkine/linux-tpm2/commit/dffce68ce34da265a62908dec71b2d85fc16824f

I want to initialize dynamically so that it is easy to support 
TPM_PT_PCR_COUNT later.

/Jarkko

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jason Gunthorpe Sept. 30, 2014, 8:12 p.m. UTC | #14
On Tue, Sep 30, 2014 at 11:07:21PM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 26, 2014 at 08:19:47PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 24, 2014 at 02:46:27PM -0600, Jason Gunthorpe wrote:
> > > > That would be 24*2 files only for pcrs...
> > > 
> > > Some subsystems do just that..
> > > 
> > > $ ls /sys/class/infiniband/qib0/ports/1/sl2vl/
> > > 0  1  10  11  12  13  14  15  2  3  4  5  6  7  8  9
> > 
> > They use static structures in
> > drivers/infiniband/hw/qib/qib_sysfs.c and it does not looks a
> > mess. I would prefer to create struct attribute entries
> > dynamically if there's clean and easy way to do that.
> 
> I gave this a shot:
> 
> https://github.com/jsakkine/linux-tpm2/commit/dffce68ce34da265a62908dec71b2d85fc16824f
> 
> I want to initialize dynamically so that it is easy to support
> TPM_PT_PCR_COUNT later.

You can't use a static pcr_dev_attrs, this has to be allocated in the
chip structure (because of the NULL). Otherwise looks about right
(although there are more problematic core details here, like racing of the
tpm dev create with the creation of the sysfs files)

Do you have a reason to have the pcrs in sysfs? I'd be just as happy
to see them dropped or moved to debugfs for TPM2 as well.

Jason

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Jarkko Sakkinen Oct. 2, 2014, 12:30 p.m. UTC | #15
On Tue, Sep 30, 2014 at 02:12:09PM -0600, Jason Gunthorpe wrote:
> On Tue, Sep 30, 2014 at 11:07:21PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Sep 26, 2014 at 08:19:47PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Sep 24, 2014 at 02:46:27PM -0600, Jason Gunthorpe wrote:
> > > > > That would be 24*2 files only for pcrs...
> > > > 
> > > > Some subsystems do just that..
> > > > 
> > > > $ ls /sys/class/infiniband/qib0/ports/1/sl2vl/
> > > > 0  1  10  11  12  13  14  15  2  3  4  5  6  7  8  9
> > > 
> > > They use static structures in
> > > drivers/infiniband/hw/qib/qib_sysfs.c and it does not looks a
> > > mess. I would prefer to create struct attribute entries
> > > dynamically if there's clean and easy way to do that.
> > 
> > I gave this a shot:
> > 
> > https://github.com/jsakkine/linux-tpm2/commit/dffce68ce34da265a62908dec71b2d85fc16824f
> > 
> > I want to initialize dynamically so that it is easy to support
> > TPM_PT_PCR_COUNT later.
> 
> You can't use a static pcr_dev_attrs, this has to be allocated in the
> chip structure (because of the NULL). Otherwise looks about right
> (although there are more problematic core details here, like racing of the
> tpm dev create with the creation of the sysfs files)

I improved things and pushed a commit that encapsulates PCR bank into
struct pcrs_kobj (internal struct in tpm2-sysfs.c).

This brings me to my question. In TPM2 there is not just PCR bank with
SHA-1 hashes but there are multiple PCR banks.

The way I plan to represent this is:

pcrs/<tag for banks hash algorithm>/<PCR index>

My last commit provides most of the code for implementing this although
in the initial patch set I would take a short cut and only show SHA-1
PCRs. The important thing is that it is put into pcrs/sha1 directory.

> Do you have a reason to have the pcrs in sysfs? I'd be just as happy
> to see them dropped or moved to debugfs for TPM2 as well.

Well, I at least find it useful debugging tool sometimes. And now it
is more useful because PCR values are machine readable.

I will include this to the v2 patch set. When we have a clean patch
set with all the fixes applied into tpm2-v1 branch it will be a better
time to evaluate what is mandatory and what can be postponed.

> Jason

/Jarkko

------------------------------------------------------------------------------
Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
diff mbox

Patch

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 253e823..9e71c43 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -2,7 +2,7 @@ 
 # Makefile for the kernel tpm device drivers.
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
-tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-commands.o
+tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-commands.o tpm2-sysfs.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o
 
 ifdef CONFIG_ACPI
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 128942b..91d8213 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -127,7 +127,10 @@  int tpm_chip_register(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
-	rc = tpm_sysfs_add_device(chip);
+	if (chip->tpm2)
+		rc = tpm2_sysfs_add_device(chip);
+	else
+		rc = tpm_sysfs_add_device(chip);
 	if (rc)
 		goto del_misc;
 
@@ -160,7 +163,10 @@  void tpm_chip_unregister(struct tpm_chip *chip)
 	synchronize_rcu();
 
 	tpm_dev_del_device(chip);
-	tpm_sysfs_del_device(chip);
+	if (chip->tpm2)
+		tpm2_sysfs_del_device(chip);
+	else
+		tpm_sysfs_del_device(chip);
 	tpm_remove_ppi(&chip->dev->kobj);
 
 	if (!chip->tpm2)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 7a9c096..26195db 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -899,6 +899,7 @@  void tpm_remove_hardware(struct device *dev)
 
 	tpm_chip_unregister(chip);
 
+
 	/* write it this way to be explicit (chip->dev == dev) */
 	put_device(chip->dev);
 }
diff --git a/drivers/char/tpm/tpm2-commands.c b/drivers/char/tpm/tpm2-commands.c
index a21dfd5..6365087 100644
--- a/drivers/char/tpm/tpm2-commands.c
+++ b/drivers/char/tpm/tpm2-commands.c
@@ -195,7 +195,7 @@  ssize_t tpm2_get_tpm_pt(struct device *dev, u32 property_id,  u32* value,
 	cmd.header.in = tpm2_get_tpm_pt_header;
 	cmd.params.tpm2_get_tpm_pt_in.cap_id =
 		cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
-	cmd.params.tpm2_get_tpm_pt_in.property_id = property_id;
+	cmd.params.tpm2_get_tpm_pt_in.property_id = cpu_to_be32(property_id);
 	cmd.params.tpm2_get_tpm_pt_in.property_cnt = cpu_to_be32(1);
 
 	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
diff --git a/drivers/char/tpm/tpm2-sysfs.c b/drivers/char/tpm/tpm2-sysfs.c
new file mode 100644
index 0000000..a254b2c
--- /dev/null
+++ b/drivers/char/tpm/tpm2-sysfs.c
@@ -0,0 +1,242 @@ 
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Authors:
+ * Leendert van Doorn <leendert@watson.ibm.com>
+ * Dave Safford <safford@watson.ibm.com>
+ * Reiner Sailer <sailer@watson.ibm.com>
+ * Kylene Hall <kjhall@us.ibm.com>
+ *
+ * Copyright (C) 2013 Obsidian Research Corp
+ * Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
+ *
+ * sysfs filesystem inspection interface to the TPM
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ */
+#include <linux/device.h>
+#include "tpm.h"
+
+static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	u8 digest[TPM_DIGEST_SIZE];
+	ssize_t rc;
+	int i, j;
+	char *str = buf;
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	for (i = 0; i < TPM2_PLATFORM_PCR; i++) {
+		rc = tpm2_pcr_read_dev(chip, i, digest);
+		if (rc)
+			break;
+		str += sprintf(str, "PCR-%02d: ", i);
+		for (j = 0; j < TPM_DIGEST_SIZE; j++)
+			str += sprintf(str, "%02X ", digest[j]);
+		str += sprintf(str, "\n");
+	}
+
+	return str - buf;
+}
+static DEVICE_ATTR_RO(pcrs);
+
+static ssize_t enabled_sh_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	struct tpm2_startup_clear value;
+	ssize_t rc;
+
+	rc = tpm2_get_tpm_pt(dev, TPM2_PT_STARTUP_CLEAR, (u32 *) &value,
+			     "could not retrieve STARTUP_CLEAR property");
+	if (rc)
+		return 0;
+
+	rc = sprintf(buf, "%d\n", value.sh_enable);
+	return rc;
+}
+static DEVICE_ATTR_RO(enabled_sh);
+
+static ssize_t enabled_eh_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	struct tpm2_startup_clear value;
+	ssize_t rc;
+
+	rc = tpm2_get_tpm_pt(dev, TPM2_PT_STARTUP_CLEAR, (u32 *) &value,
+			     "could not retrieve STARTUP_CLEAR property");
+	if (rc)
+		return 0;
+
+	rc = sprintf(buf, "%d\n", value.eh_enable);
+	return rc;
+}
+static DEVICE_ATTR_RO(enabled_eh);
+
+static ssize_t owned_sh_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct tpm2_permanent value;
+	ssize_t rc;
+
+	rc = tpm2_get_tpm_pt(dev, TPM2_PT_PERMANENT, (u32 *) &value,
+			     "could not retrieve PERMANENT property");
+	if (rc)
+		return 0;
+
+	rc = sprintf(buf, "%d\n", value.owner_auth_set);
+	return rc;
+}
+static DEVICE_ATTR_RO(owned_sh);
+
+static ssize_t owned_eh_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct tpm2_permanent value;
+	ssize_t rc;
+
+	rc = tpm2_get_tpm_pt(dev, TPM2_PT_PERMANENT, (u32 *) &value,
+			     "could not retrieve PERMANENT property");
+	if (rc)
+		return 0;
+
+	rc = sprintf(buf, "%d\n", value.endorsement_auth_set);
+	return rc;
+}
+static DEVICE_ATTR_RO(owned_eh);
+
+static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	u32 value1;
+	u32 value2;
+	ssize_t rc;
+	char *str = buf;
+
+	rc = tpm2_get_tpm_pt(dev, TPM2_PT_MANUFACTURER, (u32 *) &value1,
+			     "could not retrieve MANUFACTURER property");
+	if (rc)
+		return 0;
+
+	str += sprintf(str, "Manufacturer: 0x%08x\n", be32_to_cpu(value1));
+	str += sprintf(str, "TCG version: 2.0\n");
+
+	rc = tpm2_get_tpm_pt(dev, TPM2_PT_FIRMWARE_VERSION_1, (u32 *) &value1,
+			     "could not retrieve FIRMWARE_VERSION_1 property");
+	if (rc)
+		return 0;
+
+	rc = tpm2_get_tpm_pt(dev, TPM2_PT_FIRMWARE_VERSION_2, (u32 *) &value2,
+			     "could not retrieve FIRMWARE_VERSION_2 property");
+	if (rc)
+		return 0;
+
+	str += sprintf(str, "Firmware version: 0x%08x 0x%08x\n", value1, value2);
+
+	return str - buf;
+}
+static DEVICE_ATTR_RO(caps);
+
+static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	if (chip == NULL)
+		return 0;
+
+	chip->ops->cancel(chip);
+	return count;
+}
+static DEVICE_ATTR_WO(cancel);
+
+static ssize_t durations_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	if (chip->vendor.duration[TPM_LONG] == 0)
+		return 0;
+
+	return sprintf(buf, "%d %d %d [%s]\n",
+		       jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
+		       jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
+		       jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
+		       chip->vendor.duration_adjusted
+		       ? "adjusted" : "original");
+}
+static DEVICE_ATTR_RO(durations);
+
+static ssize_t timeouts_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d %d %d %d [%s]\n",
+		       jiffies_to_usecs(chip->vendor.timeout_a),
+		       jiffies_to_usecs(chip->vendor.timeout_b),
+		       jiffies_to_usecs(chip->vendor.timeout_c),
+		       jiffies_to_usecs(chip->vendor.timeout_d),
+		       chip->vendor.timeout_adjusted
+		       ? "adjusted" : "original");
+}
+static DEVICE_ATTR_RO(timeouts);
+
+static ssize_t version_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	char *str = buf;
+
+	str += sprintf(str, "2.0\n");
+
+	return str - buf;
+}
+
+static DEVICE_ATTR_RO(version);
+
+
+static ssize_t tpm2_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", chip->tpm2);
+}
+static DEVICE_ATTR_RO(tpm2);
+
+static struct attribute *tpm_dev_attrs[] = {
+	&dev_attr_pcrs.attr,
+	&dev_attr_enabled_sh.attr,
+	&dev_attr_enabled_eh.attr,
+	&dev_attr_owned_sh.attr,
+	&dev_attr_owned_eh.attr,
+	&dev_attr_caps.attr,
+	&dev_attr_cancel.attr,
+	&dev_attr_durations.attr,
+	&dev_attr_timeouts.attr,
+	&dev_attr_version.attr,
+	&dev_attr_tpm2.attr,
+	NULL,
+};
+
+static const struct attribute_group tpm_dev_group = {
+	.attrs = tpm_dev_attrs,
+};
+
+int tpm2_sysfs_add_device(struct tpm_chip *chip)
+{
+	int err;
+	err = sysfs_create_group(&chip->dev->kobj,
+				 &tpm_dev_group);
+
+	if (err)
+		dev_err(chip->dev,
+			"failed to create sysfs attributes, %d\n", err);
+	return err;
+}
+
+void tpm2_sysfs_del_device(struct tpm_chip *chip)
+{
+	sysfs_remove_group(&chip->dev->kobj, &tpm_dev_group);
+}
diff --git a/drivers/char/tpm/tpm2.h b/drivers/char/tpm/tpm2.h
index ba7c053..7a1502a 100644
--- a/drivers/char/tpm/tpm2.h
+++ b/drivers/char/tpm/tpm2.h
@@ -59,6 +59,34 @@  enum tpm2_capabilities {
 	TPM2_CAP_TPM_PROPERTIES = 6,
 };
 
+enum tpm2_tpm_properties {
+	TPM2_PT_MANUFACTURER		= 0x00000105,
+	TPM2_PT_FIRMWARE_VERSION_1	= 0x00000111,
+	TPM2_PT_FIRMWARE_VERSION_2	= 0x00000111,
+	TPM2_PT_PERMANENT		= 0x00000200,
+	TPM2_PT_STARTUP_CLEAR		= 0x00000201,
+};
+
+struct tpm2_permanent {
+	unsigned int owner_auth_set		: 1;
+	unsigned int endorsement_auth_set	: 1;
+	unsigned int lockout_auth_set		: 1;
+	unsigned int reserved1			: 5;
+	unsigned int disable_clear		: 1;
+	unsigned int in_lockout			: 1;
+	unsigned int tpm_generated_eps		: 1;
+	unsigned int reserved2			: 21;
+};
+
+struct tpm2_startup_clear {
+	unsigned int ph_enable			: 1;
+	unsigned int sh_enable			: 1;
+	unsigned int eh_enable			: 1;
+	unsigned int ph_enable_nv		: 1;
+	unsigned int reserved			: 27;
+	unsigned int orderly			: 1;
+};
+
 struct tpm_chip;
 
 #define TPM2_CC_FIRST	0x11F
@@ -74,4 +102,7 @@  int tpm2_do_selftest(struct tpm_chip *chip);
 int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max);
 void tpm2_gen_interrupt(struct tpm_chip *chip);
 
+int tpm2_sysfs_add_device(struct tpm_chip *chip);
+void tpm2_sysfs_del_device(struct tpm_chip *chip);
+
 #endif /* __DRIVERS_CHAR_TPM2_H__ */