2013-09-23 18:16:00

by Jason Gunthorpe

Subject: [PATCH 00/13] TPM cleanup

While writing two new TPM drivers I noticed that there was a big and growing
mess in drivers/char/tpm. This series of patches is the first batch of patches
containing my attempt to fix it.

This series makes several small repetitive changes to all the drivers. These
are bundled in with the commits changing the core, because the core API is
changed and everything must be kept together to retain bisect-ability.

I have decided not to tackle the lifetime issues in this series. My
first attempt was flawed.

I have presented the series in the full linear format. It is available on
my github:
https://github.com/jgunthorpe/linux/commits/tpm-devel
The branch is not stable, I will rebase and rewrite history as needed.

All patches have been compile tested on x86-64 (including vtpm, I have a hack
to make that happen) and all patches are checkpatch.pl clean.

I have tested tpm_tis (PPC32), tpm_i2c_atmel and tpm_i2c_nuvoton (ARM32) on my
hardware here.

There is very little new code here, the majority of lines is just code motion
to remove duplication.

Patches 1 -> 6, 12
- These are just simple cleanups that I discovered while doing the other
work.

Patch 7
- This is important to unify the sysfs code. I've choosen to simplify
and not make the TPM version bus-dependent.

Patch 8-9
- The drivers have this duplication of the file_operations
and sysfs code. This series pulls all of that code out of the drivers
and into the core

Patches 10->11
- This is the first step in making the TPM subsystem be like other
subsystems in the kernel.

Patches 13
- This cleans up the struct tpm_chip by hiding stuff that isn't
public

This is already a fair clean up, but there is still more to do someday:
* The lifetime issues around struct tpm_chip/etc and the way userspace
can hold /dev/tpmX open even after the driver has detached.
* struct tpm_chip and struct tpm_vendor_specific need to be merged
* The above two structs have many members that are never used by the
core code. These members need to be migrated into driver
private structures
* The resulting merged struct should live in linux/include/tpm.h
like all other device classes in the kernel.
* Several drivers need clean up to move singleton static
variables into dynamic structures
* Consolidate TCG defined constants that are duplicated in many
drivers
* Consolidate device startup (tpm_startup, tpm_get_timeoutes)
into core code

If this series is successful I may be able to do some of the above as well.

Jason Gunthorpe (13):
tpm: ibmvtpm: Use %zd formatting for size_t format arguments
tpm atmel: Call request_region with the correct base
tpm: xen-tpmfront: Fix default durations
tpm: Store devname in the tpm_chip
tpm: Use container_of to locate the tpm_chip in tpm_open
tpm: Remove redundant dev_set_drvdata
tpm: Remove tpm_show_caps_1_2
tpm: Pull everything related to /dev/tpmX into tpm-dev.c
tpm: Pull everything related to sysfs into tpm-sysfs.c
tpm: Create a tpm_class_ops structure and use it in the drivers
tpm: Use the ops structure instead of a copy in tpm_vendor_specific
tpm: st33: Remove chip->data_buffer access from this driver
tpm: Make tpm-dev allocate a per-file structure

drivers/char/tpm/Makefile | 2 +-
drivers/char/tpm/tpm-dev.c | 213 +++++++++++++++
drivers/char/tpm/tpm-sysfs.c | 318 ++++++++++++++++++++++
drivers/char/tpm/tpm.c | 524 +++---------------------------------
drivers/char/tpm/tpm.h | 86 +++---
drivers/char/tpm/tpm_atmel.c | 30 +--
drivers/char/tpm/tpm_i2c_atmel.c | 42 +--
drivers/char/tpm/tpm_i2c_infineon.c | 44 +--
drivers/char/tpm/tpm_i2c_nuvoton.c | 42 +--
drivers/char/tpm/tpm_i2c_stm_st33.c | 51 +---
drivers/char/tpm/tpm_ibmvtpm.c | 44 +--
drivers/char/tpm/tpm_infineon.c | 28 +-
drivers/char/tpm/tpm_nsc.c | 28 +-
drivers/char/tpm/tpm_spi_stm_st33.c | 50 +---
drivers/char/tpm/tpm_tis.c | 43 +--
drivers/char/tpm/xen-tpmfront.c | 57 +---
include/linux/tpm.h | 15 ++
17 files changed, 638 insertions(+), 979 deletions(-)
create mode 100644 drivers/char/tpm/tpm-dev.c
create mode 100644 drivers/char/tpm/tpm-sysfs.c

--
1.8.1.2

2013-09-23 18:16:17

by Jason Gunthorpe

Subject: [PATCH 02/13] tpm atmel: Call request_region with the correct base

Commit e0dd03caf20d040a0a86b6bd74028ec9bda545f5
changed the code path here so that ateml_get_base_addr
no longer directly altered the tpm_vendor_specific
structure, and instead placed the base address on the stack.

The commit missed updating the request_region call, which
would have resulted in request_region being called with 0
as the base address.

I don't know if request_region(0, ..) will fail, if so the
driver has been broken since 2006 and we should remove it
from the tree as it has no users.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm_atmel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
index 99d6820..c9a528d 100644
--- a/drivers/char/tpm/tpm_atmel.c
+++ b/drivers/char/tpm/tpm_atmel.c
@@ -202,7 +202,7 @@ static int __init init_atmel(void)

have_region =
(atmel_request_region
- (tpm_atmel.base, region_size, "tpm_atmel0") == NULL) ? 0 : 1;
+ (base, region_size, "tpm_atmel0") == NULL) ? 0 : 1;

pdev = platform_device_register_simple("tpm_atmel", -1, NULL, 0);
if (IS_ERR(pdev)) {
--
1.8.1.2

2013-09-23 18:16:16

by Jason Gunthorpe

Subject: [PATCH 04/13] tpm: Store devname in the tpm_chip

Just put the memory directly in the chip structure, rather than
in a 2nd dedicated kmalloc.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm.c | 17 ++++++-----------
drivers/char/tpm/tpm.h | 1 +
2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index e3c974a..71eb8c7 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1463,7 +1463,6 @@ void tpm_dev_vendor_release(struct tpm_chip *chip)
chip->vendor.release(chip->dev);

clear_bit(chip->dev_num, dev_mask);
- kfree(chip->vendor.miscdev.name);
}
EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);

@@ -1496,17 +1495,13 @@ EXPORT_SYMBOL_GPL(tpm_dev_release);
struct tpm_chip *tpm_register_hardware(struct device *dev,
const struct tpm_vendor_specific *entry)
{
-#define DEVNAME_SIZE 7
-
- char *devname;
struct tpm_chip *chip;

/* Driver specific per-device data */
chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- devname = kmalloc(DEVNAME_SIZE, GFP_KERNEL);

- if (chip == NULL || devname == NULL)
- goto out_free;
+ if (chip == NULL)
+ return NULL;

mutex_init(&chip->buffer_mutex);
mutex_init(&chip->tpm_mutex);
@@ -1531,8 +1526,9 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,

set_bit(chip->dev_num, dev_mask);

- scnprintf(devname, DEVNAME_SIZE, "%s%d", "tpm", chip->dev_num);
- chip->vendor.miscdev.name = devname;
+ scnprintf(chip->devname, sizeof(chip->devname), "%s%d", "tpm",
+ chip->dev_num);
+ chip->vendor.miscdev.name = chip->devname;

chip->vendor.miscdev.parent = dev;
chip->dev = get_device(dev);
@@ -1558,7 +1554,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,
goto put_device;
}

- chip->bios_dir = tpm_bios_log_setup(devname);
+ chip->bios_dir = tpm_bios_log_setup(chip->devname);

/* Make chip available */
spin_lock(&driver_lock);
@@ -1571,7 +1567,6 @@ put_device:
put_device(chip->dev);
out_free:
kfree(chip);
- kfree(devname);
return NULL;
}
EXPORT_SYMBOL_GPL(tpm_register_hardware);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7bfc17..0df18b5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -122,6 +122,7 @@ struct tpm_chip {
struct device *dev; /* Device stuff */

int dev_num; /* /dev/tpm# */
+ char devname[7];
unsigned long is_open; /* only one allowed */
int time_expired;

--
1.8.1.2

2013-09-23 18:16:14

by Jason Gunthorpe

Subject: [PATCH 06/13] tpm: Remove redundant dev_set_drvdata

TPM drivers should not call dev_set_drvdata (or aliases), only the core
code is allowed to call dev_set_drvdata, and it does it during
tpm_register_hardware.

These extra sets are harmless, but are an anti-pattern that many drivers
have copied.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm_i2c_infineon.c | 2 --
drivers/char/tpm/tpm_i2c_stm_st33.c | 2 --
drivers/char/tpm/tpm_spi_stm_st33.c | 1 -
drivers/char/tpm/xen-tpmfront.c | 2 --
4 files changed, 7 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index b8735de..e33d8e5 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -685,7 +685,6 @@ out_vendor:
chip->dev->release = NULL;
chip->release = NULL;
tpm_dev.client = NULL;
- dev_set_drvdata(chip->dev, chip);
out_err:
return rc;
}
@@ -766,7 +765,6 @@ static int tpm_tis_i2c_remove(struct i2c_client *client)
chip->dev->release = NULL;
chip->release = NULL;
tpm_dev.client = NULL;
- dev_set_drvdata(chip->dev, chip);

return 0;
}
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 5bb8e2d..1c68d93 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -746,8 +746,6 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)

tpm_get_timeouts(chip);

- i2c_set_clientdata(client, chip);
-
dev_info(chip->dev, "TPM I2C Initialized\n");
return 0;
_irq_set:
diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
index 8d3e8e2..f5e3cd6 100644
--- a/drivers/char/tpm/tpm_spi_stm_st33.c
+++ b/drivers/char/tpm/tpm_spi_stm_st33.c
@@ -779,7 +779,6 @@ tpm_st33_spi_probe(struct spi_device *dev)
tpm_get_timeouts(chip);

/* attach chip datas to client */
- spi_set_drvdata(dev, chip);
platform_data->bchipf = false;

pr_info("TPM SPI Initialized\n");
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 6f2fe2b..7711928 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -379,8 +379,6 @@ static int tpmfront_probe(struct xenbus_device *dev,

tpm_get_timeouts(priv->chip);

- dev_set_drvdata(&dev->dev, priv->chip);
-
return rv;
}

--
1.8.1.2

2013-09-23 18:16:13

by Jason Gunthorpe

Subject: [PATCH 05/13] tpm: Use container_of to locate the tpm_chip in tpm_open

misc_open sets the file->private_date to the misc_dev when calling
open. We can use container_of to go from the misc_dev back to the
tpm_chip.

Future clean ups will move tpm_open into a new file and this change
means we do not have to export the tpm_chip list.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm.c | 21 ++++-----------------
1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 71eb8c7..c3ab508 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1170,38 +1170,25 @@ EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
*/
int tpm_open(struct inode *inode, struct file *file)
{
- int minor = iminor(inode);
- struct tpm_chip *chip = NULL, *pos;
-
- rcu_read_lock();
- list_for_each_entry_rcu(pos, &tpm_chip_list, list) {
- if (pos->vendor.miscdev.minor == minor) {
- chip = pos;
- get_device(chip->dev);
- break;
- }
- }
- rcu_read_unlock();
-
- if (!chip)
- return -ENODEV;
+ struct miscdevice *misc = file->private_data;
+ struct tpm_chip *chip = container_of(misc, struct tpm_chip,
+ vendor.miscdev);

if (test_and_set_bit(0, &chip->is_open)) {
dev_dbg(chip->dev, "Another process owns this TPM\n");
- put_device(chip->dev);
return -EBUSY;
}

chip->data_buffer = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
if (chip->data_buffer == NULL) {
clear_bit(0, &chip->is_open);
- put_device(chip->dev);
return -ENOMEM;
}

atomic_set(&chip->data_pending, 0);

file->private_data = chip;
+ get_device(chip->dev);
return 0;
}
EXPORT_SYMBOL_GPL(tpm_open);
--
1.8.1.2

2013-09-23 18:16:11

by Jason Gunthorpe

Subject: [PATCH 01/13] tpm: ibmvtpm: Use %zd formatting for size_t format arguments

This suppresses compile warnings on 32 bit builds.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm_ibmvtpm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 56b07c3..838f043 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -98,7 +98,7 @@ static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)

if (count < len) {
dev_err(ibmvtpm->dev,
- "Invalid size in recv: count=%ld, crq_size=%d\n",
+ "Invalid size in recv: count=%zd, crq_size=%d\n",
count, len);
return -EIO;
}
@@ -136,7 +136,7 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)

if (count > ibmvtpm->rtce_size) {
dev_err(ibmvtpm->dev,
- "Invalid size in send: count=%ld, rtce_size=%d\n",
+ "Invalid size in send: count=%zd, rtce_size=%d\n",
count, ibmvtpm->rtce_size);
return -EIO;
}
--
1.8.1.2

2013-09-23 18:18:07

by Jason Gunthorpe

Subject: [PATCH 03/13] tpm: xen-tpmfront: Fix default durations

All the default durations were being set to 10 minutes which is
way too long for the timeouts. Normal values for the longest
duration are around 5 mins, and short duration ar around .5s.

Further, these are just the default, tpm_get_timeouts will set
them to values from the TPM (or throw an error).

Just remove them.

Cc: Daniel De Graaf <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/xen-tpmfront.c | 7 -------
1 file changed, 7 deletions(-)

diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 7a7929b..6f2fe2b 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -210,8 +210,6 @@ static struct attribute_group vtpm_attr_grp = {
.attrs = vtpm_attrs,
};

-#define TPM_LONG_TIMEOUT (10 * 60 * HZ)
-
static const struct tpm_vendor_specific tpm_vtpm = {
.status = vtpm_status,
.recv = vtpm_recv,
@@ -224,11 +222,6 @@ static const struct tpm_vendor_specific tpm_vtpm = {
.miscdev = {
.fops = &vtpm_ops,
},
- .duration = {
- TPM_LONG_TIMEOUT,
- TPM_LONG_TIMEOUT,
- TPM_LONG_TIMEOUT,
- },
};

static irqreturn_t tpmif_interrupt(int dummy, void *dev_id)
--
1.8.1.2

2013-09-23 18:18:51

by Jason Gunthorpe

Subject: [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

CLASS-sysfs.c is a common idiom for linux subsystems.

This pulls all the sysfs attribute functions and related code
into tpm-sysfs.c. To support this change some constants are moved
from tpm.c to tpm.h and __tpm_pcr_read is made non-static and is
called tpm_pcr_read_dev.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/Makefile | 2 +-
drivers/char/tpm/tpm-sysfs.c | 318 ++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm.c | 279 +------------------------------
drivers/char/tpm/tpm.h | 54 +++---
drivers/char/tpm/tpm_atmel.c | 16 --
drivers/char/tpm/tpm_i2c_atmel.c | 30 ----
drivers/char/tpm/tpm_i2c_infineon.c | 30 ----
drivers/char/tpm/tpm_i2c_nuvoton.c | 30 ----
drivers/char/tpm/tpm_i2c_stm_st33.c | 25 ---
drivers/char/tpm/tpm_ibmvtpm.c | 28 ----
drivers/char/tpm/tpm_infineon.c | 16 --
drivers/char/tpm/tpm_nsc.c | 16 --
drivers/char/tpm/tpm_spi_stm_st33.c | 25 ---
drivers/char/tpm/tpm_tis.c | 30 ----
drivers/char/tpm/xen-tpmfront.c | 34 ----
15 files changed, 355 insertions(+), 578 deletions(-)
create mode 100644 drivers/char/tpm/tpm-sysfs.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 5a73665..2c876a9 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -1,7 +1,7 @@
#
# Makefile for the kernel tpm device drivers.
#
-obj-$(CONFIG_TCG_TPM) += tpm.o tpm-dev.o
+obj-$(CONFIG_TCG_TPM) += tpm.o tpm-dev.o tpm-sysfs.o
ifdef CONFIG_ACPI
obj-$(CONFIG_TCG_TPM) += tpm_bios.o
tpm_bios-objs += tpm_eventlog.o tpm_acpi.o tpm_ppi.o
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
new file mode 100644
index 0000000..3bcfed0
--- /dev/null
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -0,0 +1,318 @@
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Authors:
+ * Leendert van Doorn <[email protected]>
+ * Dave Safford <[email protected]>
+ * Reiner Sailer <[email protected]>
+ * Kylene Hall <[email protected]>
+ *
+ * Copyright (C) 2013 Obsidian Reearch Corp
+ * Jason Gunthorpe <[email protected]>
+ *
+ * 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"
+
+/* XXX for now this helper is duplicated in tpm.c */
+static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
+ int len, const char *desc)
+{
+ int err;
+
+ len = tpm_transmit(chip, (u8 *) cmd, len);
+ if (len < 0)
+ return len;
+ else if (len < TPM_HEADER_SIZE)
+ return -EFAULT;
+
+ err = be32_to_cpu(cmd->header.out.return_code);
+ if (err != 0 && desc)
+ dev_err(chip->dev, "A TPM error (%d) occurred %s\n", err, desc);
+
+ return err;
+}
+
+#define READ_PUBEK_RESULT_SIZE 314
+#define TPM_ORD_READPUBEK cpu_to_be32(124)
+static struct tpm_input_header tpm_readpubek_header = {
+ .tag = TPM_TAG_RQU_COMMAND,
+ .length = cpu_to_be32(30),
+ .ordinal = TPM_ORD_READPUBEK
+};
+static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ u8 *data;
+ struct tpm_cmd_t tpm_cmd;
+ ssize_t err;
+ int i, rc;
+ char *str = buf;
+
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+
+ tpm_cmd.header.in = tpm_readpubek_header;
+ err = transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
+ "attempting to read the PUBEK");
+ if (err)
+ goto out;
+
+ /*
+ ignore header 10 bytes
+ algorithm 32 bits (1 == RSA )
+ encscheme 16 bits
+ sigscheme 16 bits
+ parameters (RSA 12->bytes: keybit, #primes, expbit)
+ keylenbytes 32 bits
+ 256 byte modulus
+ ignore checksum 20 bytes
+ */
+ data = tpm_cmd.params.readpubek_out_buffer;
+ str +=
+ sprintf(str,
+ "Algorithm: %02X %02X %02X %02X\n"
+ "Encscheme: %02X %02X\n"
+ "Sigscheme: %02X %02X\n"
+ "Parameters: %02X %02X %02X %02X "
+ "%02X %02X %02X %02X "
+ "%02X %02X %02X %02X\n"
+ "Modulus length: %d\n"
+ "Modulus:\n",
+ data[0], data[1], data[2], data[3],
+ data[4], data[5],
+ data[6], data[7],
+ data[12], data[13], data[14], data[15],
+ data[16], data[17], data[18], data[19],
+ data[20], data[21], data[22], data[23],
+ be32_to_cpu(*((__be32 *) (data + 24))));
+
+ for (i = 0; i < 256; i++) {
+ str += sprintf(str, "%02X ", data[i + 28]);
+ if ((i + 1) % 16 == 0)
+ str += sprintf(str, "\n");
+ }
+out:
+ rc = str - buf;
+ return rc;
+}
+static DEVICE_ATTR_RO(pubek);
+
+static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ cap_t cap;
+ u8 digest[TPM_DIGEST_SIZE];
+ ssize_t rc;
+ int i, j, num_pcrs;
+ char *str = buf;
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+
+ rc = tpm_getcap(dev, TPM_CAP_PROP_PCR, &cap,
+ "attempting to determine the number of PCRS");
+ if (rc)
+ return 0;
+
+ num_pcrs = be32_to_cpu(cap.num_pcrs);
+ for (i = 0; i < num_pcrs; i++) {
+ rc = tpm_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_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ cap_t cap;
+ ssize_t rc;
+
+ rc = tpm_getcap(dev, TPM_CAP_FLAG_PERM, &cap,
+ "attempting to determine the permanent enabled state");
+ if (rc)
+ return 0;
+
+ rc = sprintf(buf, "%d\n", !cap.perm_flags.disable);
+ return rc;
+}
+static DEVICE_ATTR_RO(enabled);
+
+ssize_t active_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ cap_t cap;
+ ssize_t rc;
+
+ rc = tpm_getcap(dev, TPM_CAP_FLAG_PERM, &cap,
+ "attempting to determine the permanent active state");
+ if (rc)
+ return 0;
+
+ rc = sprintf(buf, "%d\n", !cap.perm_flags.deactivated);
+ return rc;
+}
+static DEVICE_ATTR_RO(active);
+
+static ssize_t owned_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ cap_t cap;
+ ssize_t rc;
+
+ rc = tpm_getcap(dev, TPM_CAP_PROP_OWNER, &cap,
+ "attempting to determine the owner state");
+ if (rc)
+ return 0;
+
+ rc = sprintf(buf, "%d\n", cap.owned);
+ return rc;
+}
+static DEVICE_ATTR_RO(owned);
+
+static ssize_t temp_deactivated_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ cap_t cap;
+ ssize_t rc;
+
+ rc = tpm_getcap(dev, TPM_CAP_FLAG_VOL, &cap,
+ "attempting to determine the temporary state");
+ if (rc)
+ return 0;
+
+ rc = sprintf(buf, "%d\n", cap.stclear_flags.deactivated);
+ return rc;
+}
+static DEVICE_ATTR_RO(temp_deactivated);
+
+static ssize_t caps_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ cap_t cap;
+ ssize_t rc;
+ char *str = buf;
+
+ rc = tpm_getcap(dev, TPM_CAP_PROP_MANUFACTURER, &cap,
+ "attempting to determine the manufacturer");
+ if (rc)
+ return 0;
+ str += sprintf(str, "Manufacturer: 0x%x\n",
+ be32_to_cpu(cap.manufacturer_id));
+
+ /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
+ rc = tpm_getcap(dev, CAP_VERSION_1_2, &cap,
+ "attempting to determine the 1.2 version");
+ if (!rc) {
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ cap.tpm_version_1_2.Major,
+ cap.tpm_version_1_2.Minor,
+ cap.tpm_version_1_2.revMajor,
+ cap.tpm_version_1_2.revMinor);
+ } else {
+ /* Otherwise just use TPM_STRUCT_VER */
+ rc = tpm_getcap(dev, CAP_VERSION_1_1, &cap,
+ "attempting to determine the 1.1 version");
+ if (rc)
+ return 0;
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ cap.tpm_version.Major,
+ cap.tpm_version.Minor,
+ cap.tpm_version.revMajor,
+ cap.tpm_version.revMinor);
+ }
+
+ 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->vendor.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 struct attribute *tpm_dev_attrs[] = {
+ &dev_attr_pubek.attr,
+ &dev_attr_pcrs.attr,
+ &dev_attr_enabled.attr,
+ &dev_attr_active.attr,
+ &dev_attr_owned.attr,
+ &dev_attr_temp_deactivated.attr,
+ &dev_attr_caps.attr,
+ &dev_attr_cancel.attr,
+ &dev_attr_durations.attr,
+ &dev_attr_timeouts.attr,
+ NULL,
+};
+
+static const struct attribute_group tpm_dev_group = {
+ .attrs = tpm_dev_attrs,
+};
+
+int tpm_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 tpm_sysfs_del_device(struct tpm_chip *chip)
+{
+ sysfs_remove_group(&chip->dev->kobj, &tpm_dev_group);
+}
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 63f1c45..39758f8 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -32,13 +32,6 @@
#include "tpm.h"
#include "tpm_eventlog.h"

-enum tpm_duration {
- TPM_SHORT = 0,
- TPM_MEDIUM = 1,
- TPM_LONG = 2,
- TPM_UNDEFINED,
-};
-
#define TPM_MAX_ORDINAL 243
#define TSC_MAX_ORDINAL 12
#define TPM_PROTECTED_COMMAND 0x00
@@ -404,24 +397,6 @@ out:
#define TPM_DIGEST_SIZE 20
#define TPM_RET_CODE_IDX 6

-enum tpm_capabilities {
- TPM_CAP_FLAG = cpu_to_be32(4),
- TPM_CAP_PROP = cpu_to_be32(5),
- CAP_VERSION_1_1 = cpu_to_be32(0x06),
- CAP_VERSION_1_2 = cpu_to_be32(0x1A)
-};
-
-enum tpm_sub_capabilities {
- TPM_CAP_PROP_PCR = cpu_to_be32(0x101),
- TPM_CAP_PROP_MANUFACTURER = cpu_to_be32(0x103),
- TPM_CAP_FLAG_PERM = cpu_to_be32(0x108),
- TPM_CAP_FLAG_VOL = cpu_to_be32(0x109),
- TPM_CAP_PROP_OWNER = cpu_to_be32(0x111),
- TPM_CAP_PROP_TIS_TIMEOUT = cpu_to_be32(0x115),
- TPM_CAP_PROP_TIS_DURATION = cpu_to_be32(0x120),
-
-};
-
static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
int len, const char *desc)
{
@@ -441,7 +416,6 @@ static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
}

#define TPM_INTERNAL_RESULT_SIZE 200
-#define TPM_TAG_RQU_COMMAND cpu_to_be16(193)
#define TPM_ORD_GET_CAP cpu_to_be32(101)
#define TPM_ORD_GET_RANDOM cpu_to_be32(70)

@@ -641,70 +615,6 @@ static int tpm_continue_selftest(struct tpm_chip *chip)
return rc;
}

-ssize_t tpm_show_enabled(struct device * dev, struct device_attribute * attr,
- char *buf)
-{
- cap_t cap;
- ssize_t rc;
-
- rc = tpm_getcap(dev, TPM_CAP_FLAG_PERM, &cap,
- "attempting to determine the permanent enabled state");
- if (rc)
- return 0;
-
- rc = sprintf(buf, "%d\n", !cap.perm_flags.disable);
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_show_enabled);
-
-ssize_t tpm_show_active(struct device * dev, struct device_attribute * attr,
- char *buf)
-{
- cap_t cap;
- ssize_t rc;
-
- rc = tpm_getcap(dev, TPM_CAP_FLAG_PERM, &cap,
- "attempting to determine the permanent active state");
- if (rc)
- return 0;
-
- rc = sprintf(buf, "%d\n", !cap.perm_flags.deactivated);
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_show_active);
-
-ssize_t tpm_show_owned(struct device * dev, struct device_attribute * attr,
- char *buf)
-{
- cap_t cap;
- ssize_t rc;
-
- rc = tpm_getcap(dev, TPM_CAP_PROP_OWNER, &cap,
- "attempting to determine the owner state");
- if (rc)
- return 0;
-
- rc = sprintf(buf, "%d\n", cap.owned);
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_show_owned);
-
-ssize_t tpm_show_temp_deactivated(struct device * dev,
- struct device_attribute * attr, char *buf)
-{
- cap_t cap;
- ssize_t rc;
-
- rc = tpm_getcap(dev, TPM_CAP_FLAG_VOL, &cap,
- "attempting to determine the temporary state");
- if (rc)
- return 0;
-
- rc = sprintf(buf, "%d\n", cap.stclear_flags.deactivated);
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_show_temp_deactivated);
-
/*
* tpm_chip_find_get - return tpm_chip for given chip number
*/
@@ -734,7 +644,7 @@ static struct tpm_input_header pcrread_header = {
.ordinal = TPM_ORDINAL_PCRREAD
};

-static int __tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
{
int rc;
struct tpm_cmd_t cmd;
@@ -769,7 +679,7 @@ int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf)
chip = tpm_chip_find_get(chip_num);
if (chip == NULL)
return -ENODEV;
- rc = __tpm_pcr_read(chip, pcr_idx, res_buf);
+ rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
tpm_chip_put(chip);
return rc;
}
@@ -894,187 +804,6 @@ int tpm_send(u32 chip_num, void *cmd, size_t buflen)
}
EXPORT_SYMBOL_GPL(tpm_send);

-ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- cap_t cap;
- u8 digest[TPM_DIGEST_SIZE];
- ssize_t rc;
- int i, j, num_pcrs;
- char *str = buf;
- struct tpm_chip *chip = dev_get_drvdata(dev);
-
- rc = tpm_getcap(dev, TPM_CAP_PROP_PCR, &cap,
- "attempting to determine the number of PCRS");
- if (rc)
- return 0;
-
- num_pcrs = be32_to_cpu(cap.num_pcrs);
- for (i = 0; i < num_pcrs; i++) {
- rc = __tpm_pcr_read(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;
-}
-EXPORT_SYMBOL_GPL(tpm_show_pcrs);
-
-#define READ_PUBEK_RESULT_SIZE 314
-#define TPM_ORD_READPUBEK cpu_to_be32(124)
-static struct tpm_input_header tpm_readpubek_header = {
- .tag = TPM_TAG_RQU_COMMAND,
- .length = cpu_to_be32(30),
- .ordinal = TPM_ORD_READPUBEK
-};
-
-ssize_t tpm_show_pubek(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- u8 *data;
- struct tpm_cmd_t tpm_cmd;
- ssize_t err;
- int i, rc;
- char *str = buf;
-
- struct tpm_chip *chip = dev_get_drvdata(dev);
-
- tpm_cmd.header.in = tpm_readpubek_header;
- err = transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
- "attempting to read the PUBEK");
- if (err)
- goto out;
-
- /*
- ignore header 10 bytes
- algorithm 32 bits (1 == RSA )
- encscheme 16 bits
- sigscheme 16 bits
- parameters (RSA 12->bytes: keybit, #primes, expbit)
- keylenbytes 32 bits
- 256 byte modulus
- ignore checksum 20 bytes
- */
- data = tpm_cmd.params.readpubek_out_buffer;
- str +=
- sprintf(str,
- "Algorithm: %02X %02X %02X %02X\n"
- "Encscheme: %02X %02X\n"
- "Sigscheme: %02X %02X\n"
- "Parameters: %02X %02X %02X %02X "
- "%02X %02X %02X %02X "
- "%02X %02X %02X %02X\n"
- "Modulus length: %d\n"
- "Modulus:\n",
- data[0], data[1], data[2], data[3],
- data[4], data[5],
- data[6], data[7],
- data[12], data[13], data[14], data[15],
- data[16], data[17], data[18], data[19],
- data[20], data[21], data[22], data[23],
- be32_to_cpu(*((__be32 *) (data + 24))));
-
- for (i = 0; i < 256; i++) {
- str += sprintf(str, "%02X ", data[i + 28]);
- if ((i + 1) % 16 == 0)
- str += sprintf(str, "\n");
- }
-out:
- rc = str - buf;
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_show_pubek);
-
-
-ssize_t tpm_show_caps(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- cap_t cap;
- ssize_t rc;
- char *str = buf;
-
- rc = tpm_getcap(dev, TPM_CAP_PROP_MANUFACTURER, &cap,
- "attempting to determine the manufacturer");
- if (rc)
- return 0;
- str += sprintf(str, "Manufacturer: 0x%x\n",
- be32_to_cpu(cap.manufacturer_id));
-
- /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
- rc = tpm_getcap(dev, CAP_VERSION_1_2, &cap,
- "attempting to determine the 1.2 version");
- if (!rc) {
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version_1_2.Major,
- cap.tpm_version_1_2.Minor,
- cap.tpm_version_1_2.revMajor,
- cap.tpm_version_1_2.revMinor);
- } else {
- /* Otherwise just use TPM_STRUCT_VER */
- rc = tpm_getcap(dev, CAP_VERSION_1_1, &cap,
- "attempting to determine the 1.1 version");
- if (rc)
- return 0;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version.Major,
- cap.tpm_version.Minor,
- cap.tpm_version.revMajor,
- cap.tpm_version.revMinor);
- }
-
- return str - buf;
-}
-EXPORT_SYMBOL_GPL(tpm_show_caps);
-
-ssize_t tpm_show_durations(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");
-}
-EXPORT_SYMBOL_GPL(tpm_show_durations);
-
-ssize_t tpm_show_timeouts(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");
-}
-EXPORT_SYMBOL_GPL(tpm_show_timeouts);
-
-ssize_t tpm_store_cancel(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->vendor.cancel(chip);
- return count;
-}
-EXPORT_SYMBOL_GPL(tpm_store_cancel);
-
static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, bool check_cancel,
bool *canceled)
{
@@ -1150,7 +879,7 @@ void tpm_remove_hardware(struct device *dev)
synchronize_rcu();

tpm_dev_del_device(chip);
- sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
+ tpm_sysfs_del_device(chip);
tpm_remove_ppi(&dev->kobj);
tpm_bios_log_teardown(chip->bios_dir);

@@ -1367,7 +1096,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,
if (tpm_dev_add_device(chip))
goto put_device;

- if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group))
+ if (tpm_sysfs_add_device(chip))
goto del_misc;

if (tpm_add_ppi(&dev->kobj))
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 496228c..14ba162 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -46,6 +46,14 @@ enum tpm_addr {
TPM_ADDR = 0x4E,
};

+/* Indexes the duration array */
+enum tpm_duration {
+ TPM_SHORT = 0,
+ TPM_MEDIUM = 1,
+ TPM_LONG = 2,
+ TPM_UNDEFINED,
+};
+
#define TPM_WARN_RETRY 0x800
#define TPM_WARN_DOING_SELFTEST 0x802
#define TPM_ERR_DEACTIVATED 0x6
@@ -53,27 +61,6 @@ enum tpm_addr {
#define TPM_ERR_INVALID_POSTINIT 38

#define TPM_HEADER_SIZE 10
-extern ssize_t tpm_show_pubek(struct device *, struct device_attribute *attr,
- char *);
-extern ssize_t tpm_show_pcrs(struct device *, struct device_attribute *attr,
- char *);
-extern ssize_t tpm_show_caps(struct device *, struct device_attribute *attr,
- char *);
-extern ssize_t tpm_store_cancel(struct device *, struct device_attribute *attr,
- const char *, size_t);
-extern ssize_t tpm_show_enabled(struct device *, struct device_attribute *attr,
- char *);
-extern ssize_t tpm_show_active(struct device *, struct device_attribute *attr,
- char *);
-extern ssize_t tpm_show_owned(struct device *, struct device_attribute *attr,
- char *);
-extern ssize_t tpm_show_temp_deactivated(struct device *,
- struct device_attribute *attr, char *);
-extern ssize_t tpm_show_durations(struct device *,
- struct device_attribute *attr, char *);
-extern ssize_t tpm_show_timeouts(struct device *,
- struct device_attribute *attr, char *);
-
struct tpm_chip;

struct tpm_vendor_specific {
@@ -95,7 +82,6 @@ struct tpm_vendor_specific {
u8 (*status) (struct tpm_chip *);
void (*release) (struct device *);
struct miscdevice miscdev;
- struct attribute_group *attr_group;
struct list_head list;
int locality;
unsigned long timeout_a, timeout_b, timeout_c, timeout_d; /* jiffies */
@@ -171,6 +157,8 @@ struct tpm_output_header {
__be32 return_code;
} __packed;

+#define TPM_TAG_RQU_COMMAND cpu_to_be16(193)
+
struct stclear_flags_t {
__be16 tag;
u8 deactivated;
@@ -244,6 +232,24 @@ typedef union {
struct duration_t duration;
} cap_t;

+enum tpm_capabilities {
+ TPM_CAP_FLAG = cpu_to_be32(4),
+ TPM_CAP_PROP = cpu_to_be32(5),
+ CAP_VERSION_1_1 = cpu_to_be32(0x06),
+ CAP_VERSION_1_2 = cpu_to_be32(0x1A)
+};
+
+enum tpm_sub_capabilities {
+ TPM_CAP_PROP_PCR = cpu_to_be32(0x101),
+ TPM_CAP_PROP_MANUFACTURER = cpu_to_be32(0x103),
+ TPM_CAP_FLAG_PERM = cpu_to_be32(0x108),
+ TPM_CAP_FLAG_VOL = cpu_to_be32(0x109),
+ TPM_CAP_PROP_OWNER = cpu_to_be32(0x111),
+ TPM_CAP_PROP_TIS_TIMEOUT = cpu_to_be32(0x115),
+ TPM_CAP_PROP_TIS_DURATION = cpu_to_be32(0x120),
+
+};
+
struct tpm_getcap_params_in {
__be32 cap;
__be32 subcap_size;
@@ -340,6 +346,10 @@ extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,

int tpm_dev_add_device(struct tpm_chip *chip);
void tpm_dev_del_device(struct tpm_chip *chip);
+int tpm_sysfs_add_device(struct tpm_chip *chip);
+void tpm_sysfs_del_device(struct tpm_chip *chip);
+
+int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);

#ifdef CONFIG_ACPI
extern int tpm_add_ppi(struct kobject *);
diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
index 9692e2f..7e665d0 100644
--- a/drivers/char/tpm/tpm_atmel.c
+++ b/drivers/char/tpm/tpm_atmel.c
@@ -121,21 +121,6 @@ static bool tpm_atml_req_canceled(struct tpm_chip *chip, u8 status)
return (status == ATML_STATUS_READY);
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR |S_IWGRP, NULL, tpm_store_cancel);
-
-static struct attribute* atmel_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- NULL,
-};
-
-static struct attribute_group atmel_attr_grp = { .attrs = atmel_attrs };
-
static const struct tpm_vendor_specific tpm_atmel = {
.recv = tpm_atml_recv,
.send = tpm_atml_send,
@@ -144,7 +129,6 @@ static const struct tpm_vendor_specific tpm_atmel = {
.req_complete_mask = ATML_STATUS_BUSY | ATML_STATUS_DATA_AVAIL,
.req_complete_val = ATML_STATUS_DATA_AVAIL,
.req_canceled = tpm_atml_req_canceled,
- .attr_group = &atmel_attr_grp,
};

static struct platform_device *pdev;
diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
index f9cee5f..8476a05 100644
--- a/drivers/char/tpm/tpm_i2c_atmel.c
+++ b/drivers/char/tpm/tpm_i2c_atmel.c
@@ -135,35 +135,6 @@ static u8 i2c_atmel_read_status(struct tpm_chip *chip)
return ATMEL_STS_OK;
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
-static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
-
-static struct attribute *i2c_atmel_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- &dev_attr_durations.attr,
- &dev_attr_timeouts.attr,
- NULL,
-};
-
-static struct attribute_group i2c_atmel_attr_grp = {
- .attrs = i2c_atmel_attrs
-};
-
static bool i2c_atmel_req_canceled(struct tpm_chip *chip, u8 status)
{
return 0;
@@ -177,7 +148,6 @@ static const struct tpm_vendor_specific i2c_atmel = {
.req_complete_mask = ATMEL_STS_OK,
.req_complete_val = ATMEL_STS_OK,
.req_canceled = i2c_atmel_req_canceled,
- .attr_group = &i2c_atmel_attr_grp,
};

static int i2c_atmel_probe(struct i2c_client *client,
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index c1ba7fa..ac1218f 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -566,35 +566,6 @@ static bool tpm_tis_i2c_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
-static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
-
-static struct attribute *tis_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- &dev_attr_durations.attr,
- &dev_attr_timeouts.attr,
- NULL,
-};
-
-static struct attribute_group tis_attr_grp = {
- .attrs = tis_attrs
-};
-
static struct tpm_vendor_specific tpm_tis_i2c = {
.status = tpm_tis_i2c_status,
.recv = tpm_tis_i2c_recv,
@@ -603,7 +574,6 @@ static struct tpm_vendor_specific tpm_tis_i2c = {
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_i2c_req_canceled,
- .attr_group = &tis_attr_grp,
};

static int tpm_tis_i2c_init(struct device *dev)
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index 3766fe7..5cb9670 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -456,35 +456,6 @@ static bool i2c_nuvoton_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
-static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
-
-static struct attribute *i2c_nuvoton_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- &dev_attr_durations.attr,
- &dev_attr_timeouts.attr,
- NULL,
-};
-
-static struct attribute_group i2c_nuvoton_attr_grp = {
- .attrs = i2c_nuvoton_attrs
-};
-
static const struct tpm_vendor_specific tpm_i2c = {
.status = i2c_nuvoton_read_status,
.recv = i2c_nuvoton_recv,
@@ -493,7 +464,6 @@ static const struct tpm_vendor_specific tpm_i2c = {
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = i2c_nuvoton_req_canceled,
- .attr_group = &i2c_nuvoton_attr_grp,
};

/* The only purpose for the handler is to signal to any waiting threads that
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 2328c5e..588e763 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -569,30 +569,6 @@ static bool tpm_st33_i2c_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-
-static struct attribute *stm_tpm_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr, NULL,
-};
-
-static struct attribute_group stm_tpm_attr_grp = {
- .attrs = stm_tpm_attrs
-};
-
static struct tpm_vendor_specific st_i2c_tpm = {
.send = tpm_stm_i2c_send,
.recv = tpm_stm_i2c_recv,
@@ -601,7 +577,6 @@ static struct tpm_vendor_specific st_i2c_tpm = {
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_st33_i2c_req_canceled,
- .attr_group = &stm_tpm_attr_grp,
};

static int interrupts;
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 88a0121..6217d1d 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -403,33 +403,6 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
return (status == 0);
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
- NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
-static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
-
-static struct attribute *ibmvtpm_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- &dev_attr_durations.attr,
- &dev_attr_timeouts.attr, NULL,
-};
-
-static struct attribute_group ibmvtpm_attr_grp = { .attrs = ibmvtpm_attrs };
-
static const struct tpm_vendor_specific tpm_ibmvtpm = {
.recv = tpm_ibmvtpm_recv,
.send = tpm_ibmvtpm_send,
@@ -438,7 +411,6 @@ static const struct tpm_vendor_specific tpm_ibmvtpm = {
.req_complete_mask = 0,
.req_complete_val = 0,
.req_canceled = tpm_ibmvtpm_req_canceled,
- .attr_group = &ibmvtpm_attr_grp,
};

static const struct dev_pm_ops tpm_ibmvtpm_pm_ops = {
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index c75c10c..9525be5 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -371,21 +371,6 @@ static u8 tpm_inf_status(struct tpm_chip *chip)
return tpm_data_in(STAT);
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-
-static struct attribute *inf_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- NULL,
-};
-
-static struct attribute_group inf_attr_grp = {.attrs = inf_attrs };
-
static const struct tpm_vendor_specific tpm_inf = {
.recv = tpm_inf_recv,
.send = tpm_inf_send,
@@ -393,7 +378,6 @@ static const struct tpm_vendor_specific tpm_inf = {
.status = tpm_inf_status,
.req_complete_mask = 0,
.req_complete_val = 0,
- .attr_group = &inf_attr_grp,
};

static const struct pnp_device_id tpm_inf_pnp_tbl[] = {
diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
index a4acac9..ad980be 100644
--- a/drivers/char/tpm/tpm_nsc.c
+++ b/drivers/char/tpm/tpm_nsc.c
@@ -232,21 +232,6 @@ static bool tpm_nsc_req_canceled(struct tpm_chip *chip, u8 status)
return (status == NSC_STATUS_RDY);
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR|S_IWGRP, NULL, tpm_store_cancel);
-
-static struct attribute * nsc_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- NULL,
-};
-
-static struct attribute_group nsc_attr_grp = { .attrs = nsc_attrs };
-
static const struct tpm_vendor_specific tpm_nsc = {
.recv = tpm_nsc_recv,
.send = tpm_nsc_send,
@@ -255,7 +240,6 @@ static const struct tpm_vendor_specific tpm_nsc = {
.req_complete_mask = NSC_STATUS_OBF,
.req_complete_val = NSC_STATUS_OBF,
.req_canceled = tpm_nsc_req_canceled,
- .attr_group = &nsc_attr_grp,
};

static struct platform_device *pdev = NULL;
diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
index 99854e8..af46f0d 100644
--- a/drivers/char/tpm/tpm_spi_stm_st33.c
+++ b/drivers/char/tpm/tpm_spi_stm_st33.c
@@ -591,36 +591,11 @@ out:
return size;
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-
-static struct attribute *stm_tpm_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr, NULL,
-};
-
-static struct attribute_group stm_tpm_attr_grp = {
- .attrs = stm_tpm_attrs
-};
-
static struct tpm_vendor_specific st_spi_tpm = {
.send = tpm_stm_spi_send,
.recv = tpm_stm_spi_recv,
.cancel = tpm_stm_spi_cancel,
.status = tpm_stm_spi_status,
- .attr_group = &stm_tpm_attr_grp,
};

static int evaluate_latency(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 46f57f5..a362ede 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -432,35 +432,6 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
}
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
- NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
-static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
-
-static struct attribute *tis_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- &dev_attr_durations.attr,
- &dev_attr_timeouts.attr, NULL,
-};
-
-static struct attribute_group tis_attr_grp = {
- .attrs = tis_attrs
-};
-
static struct tpm_vendor_specific tpm_tis = {
.status = tpm_tis_status,
.recv = tpm_tis_recv,
@@ -469,7 +440,6 @@ static struct tpm_vendor_specific tpm_tis = {
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_req_canceled,
- .attr_group = &tis_attr_grp,
};

static irqreturn_t tis_int_probe(int irq, void *dev_id)
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 12a4ab2..7892557 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -168,39 +168,6 @@ ssize_t tpm_store_locality(struct device *dev, struct device_attribute *attr,
return len;
}

-static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
-static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
-static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
-static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
-static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
-static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
- NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
-static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
-static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
-static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
-static DEVICE_ATTR(locality, S_IRUGO | S_IWUSR, tpm_show_locality,
- tpm_store_locality);
-
-static struct attribute *vtpm_attrs[] = {
- &dev_attr_pubek.attr,
- &dev_attr_pcrs.attr,
- &dev_attr_enabled.attr,
- &dev_attr_active.attr,
- &dev_attr_owned.attr,
- &dev_attr_temp_deactivated.attr,
- &dev_attr_caps.attr,
- &dev_attr_cancel.attr,
- &dev_attr_durations.attr,
- &dev_attr_timeouts.attr,
- &dev_attr_locality.attr,
- NULL,
-};
-
-static struct attribute_group vtpm_attr_grp = {
- .attrs = vtpm_attrs,
-};
-
static const struct tpm_vendor_specific tpm_vtpm = {
.status = vtpm_status,
.recv = vtpm_recv,
@@ -209,7 +176,6 @@ static const struct tpm_vendor_specific tpm_vtpm = {
.req_complete_mask = VTPM_STATUS_IDLE | VTPM_STATUS_RESULT,
.req_complete_val = VTPM_STATUS_IDLE | VTPM_STATUS_RESULT,
.req_canceled = vtpm_req_canceled,
- .attr_group = &vtpm_attr_grp,
};

static irqreturn_t tpmif_interrupt(int dummy, void *dev_id)
--
1.8.1.2

2013-09-23 18:19:24

by Jason Gunthorpe

Subject: [PATCH 11/13] tpm: Use the ops structure instead of a copy in tpm_vendor_specific

This builds on the last commit to use the ops structure in the core
and reduce the size of tpm_vendor_specific.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm-sysfs.c | 2 +-
drivers/char/tpm/tpm.c | 35 +++++++++++++----------------------
drivers/char/tpm/tpm.h | 9 +--------
drivers/char/tpm/tpm_i2c_stm_st33.c | 4 ++--
drivers/char/tpm/tpm_spi_stm_st33.c | 4 ++--
5 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 3bcfed0..d2a8bca 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -245,7 +245,7 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
if (chip == NULL)
return 0;

- chip->vendor.cancel(chip);
+ chip->ops->cancel(chip);
return count;
}
static DEVICE_ATTR_WO(cancel);
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 60b04df..eef2edd 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -353,7 +353,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,

mutex_lock(&chip->tpm_mutex);

- if ((rc = chip->vendor.send(chip, (u8 *) buf, count)) < 0) {
+ rc = chip->ops->send(chip, (u8 *) buf, count);
+ if (rc < 0) {
dev_err(chip->dev,
"tpm_transmit: tpm_send: error %zd\n", rc);
goto out;
@@ -364,12 +365,12 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,

stop = jiffies + tpm_calc_ordinal_duration(chip, ordinal);
do {
- u8 status = chip->vendor.status(chip);
- if ((status & chip->vendor.req_complete_mask) ==
- chip->vendor.req_complete_val)
+ u8 status = chip->ops->status(chip);
+ if ((status & chip->ops->req_complete_mask) ==
+ chip->ops->req_complete_val)
goto out_recv;

- if (chip->vendor.req_canceled(chip, status)) {
+ if (chip->ops->req_canceled(chip, status)) {
dev_err(chip->dev, "Operation Canceled\n");
rc = -ECANCELED;
goto out;
@@ -379,13 +380,13 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
rmb();
} while (time_before(jiffies, stop));

- chip->vendor.cancel(chip);
+ chip->ops->cancel(chip);
dev_err(chip->dev, "Operation Timed out\n");
rc = -ETIME;
goto out;

out_recv:
- rc = chip->vendor.recv(chip, (u8 *) buf, bufsiz);
+ rc = chip->ops->recv(chip, (u8 *) buf, bufsiz);
if (rc < 0)
dev_err(chip->dev,
"tpm_transmit: tpm_recv: error %zd\n", rc);
@@ -807,12 +808,12 @@ EXPORT_SYMBOL_GPL(tpm_send);
static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, bool check_cancel,
bool *canceled)
{
- u8 status = chip->vendor.status(chip);
+ u8 status = chip->ops->status(chip);

*canceled = false;
if ((status & mask) == mask)
return true;
- if (check_cancel && chip->vendor.req_canceled(chip, status)) {
+ if (check_cancel && chip->ops->req_canceled(chip, status)) {
*canceled = true;
return true;
}
@@ -828,7 +829,7 @@ int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
bool canceled = false;

/* check current status */
- status = chip->vendor.status(chip);
+ status = chip->ops->status(chip);
if ((status & mask) == mask)
return 0;

@@ -855,7 +856,7 @@ again:
} else {
do {
msleep(TPM_TIMEOUT);
- status = chip->vendor.status(chip);
+ status = chip->ops->status(chip);
if ((status & mask) == mask)
return 0;
} while (time_before(jiffies, stop));
@@ -1027,9 +1028,6 @@ void tpm_dev_vendor_release(struct tpm_chip *chip)
if (!chip)
return;

- if (chip->vendor.release)
- chip->vendor.release(chip->dev);
-
clear_bit(chip->dev_num, dev_mask);
}
EXPORT_SYMBOL_GPL(tpm_dev_vendor_release);
@@ -1074,14 +1072,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,
mutex_init(&chip->tpm_mutex);
INIT_LIST_HEAD(&chip->list);

- chip->vendor.req_complete_mask = ops->req_complete_mask;
- chip->vendor.req_complete_val = ops->req_complete_val;
- chip->vendor.req_canceled = ops->req_canceled;
- chip->vendor.recv = ops->recv;
- chip->vendor.send = ops->send;
- chip->vendor.cancel = ops->cancel;
- chip->vendor.status = ops->status;
-
+ chip->ops = ops;
chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);

if (chip->dev_num >= TPM_NUM_DEVICES) {
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a56af2c..7b0a46e 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -64,9 +64,6 @@ enum tpm_duration {
struct tpm_chip;

struct tpm_vendor_specific {
- u8 req_complete_mask;
- u8 req_complete_val;
- bool (*req_canceled)(struct tpm_chip *chip, u8 status);
void __iomem *iobase; /* ioremapped address */
unsigned long base; /* TPM base address */

@@ -76,11 +73,6 @@ struct tpm_vendor_specific {
int region_size;
int have_region;

- int (*recv) (struct tpm_chip *, u8 *, size_t);
- int (*send) (struct tpm_chip *, u8 *, size_t);
- void (*cancel) (struct tpm_chip *);
- u8 (*status) (struct tpm_chip *);
- void (*release) (struct device *);
struct miscdevice miscdev;
struct list_head list;
int locality;
@@ -104,6 +96,7 @@ struct tpm_vendor_specific {

struct tpm_chip {
struct device *dev; /* Device stuff */
+ const struct tpm_class_ops *ops;

int dev_num; /* /dev/tpm# */
char devname[7];
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 0ef5d94..cd8c3e3 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -559,7 +559,7 @@ static int tpm_stm_i2c_recv(struct tpm_chip *chip, unsigned char *buf,
}

out:
- chip->vendor.cancel(chip);
+ chip->ops->cancel(chip);
release_locality(chip);
return size;
}
@@ -808,7 +808,7 @@ static int tpm_st33_i2c_pm_resume(struct device *dev)
if (power_mgt) {
gpio_set_value(pin_infos->io_lpcpd, 1);
ret = wait_for_serirq_timeout(chip,
- (chip->vendor.status(chip) &
+ (chip->ops->status(chip) &
TPM_STS_VALID) == TPM_STS_VALID,
chip->vendor.timeout_b);
} else {
diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
index 619545f..22f6e83 100644
--- a/drivers/char/tpm/tpm_spi_stm_st33.c
+++ b/drivers/char/tpm/tpm_spi_stm_st33.c
@@ -586,7 +586,7 @@ static int tpm_stm_spi_recv(struct tpm_chip *chip, unsigned char *buf,
}

out:
- chip->vendor.cancel(chip);
+ chip->ops->cancel(chip);
release_locality(chip);
return size;
}
@@ -843,7 +843,7 @@ static int tpm_st33_spi_pm_resume(struct device *dev)
if (power_mgt) {
gpio_set_value(pin_infos->io_lpcpd, 1);
ret = wait_for_serirq_timeout(chip,
- (chip->vendor.status(chip) &
+ (chip->ops->status(chip) &
TPM_STS_VALID) == TPM_STS_VALID,
chip->vendor.timeout_b);
} else {
--
1.8.1.2

2013-09-23 18:19:37

by Jason Gunthorpe

Subject: [PATCH 10/13] tpm: Create a tpm_class_ops structure and use it in the drivers

This replaces the static initialization of a tpm_vendor_specific
structure in the drivers with the standard Linux idiom of providing
a const structure of function pointers.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm.c | 10 ++++++++--
drivers/char/tpm/tpm.h | 6 +++---
drivers/char/tpm/tpm_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_infineon.c | 2 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
drivers/char/tpm/tpm_i2c_stm_st33.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 2 +-
drivers/char/tpm/tpm_infineon.c | 2 +-
drivers/char/tpm/tpm_nsc.c | 2 +-
drivers/char/tpm/tpm_spi_stm_st33.c | 2 +-
drivers/char/tpm/tpm_tis.c | 2 +-
drivers/char/tpm/xen-tpmfront.c | 2 +-
include/linux/tpm.h | 15 +++++++++++++++
14 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 39758f8..60b04df 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1061,7 +1061,7 @@ EXPORT_SYMBOL_GPL(tpm_dev_release);
* pci_disable_device
*/
struct tpm_chip *tpm_register_hardware(struct device *dev,
- const struct tpm_vendor_specific *entry)
+ const struct tpm_class_ops *ops)
{
struct tpm_chip *chip;

@@ -1074,7 +1074,13 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,
mutex_init(&chip->tpm_mutex);
INIT_LIST_HEAD(&chip->list);

- memcpy(&chip->vendor, entry, sizeof(struct tpm_vendor_specific));
+ chip->vendor.req_complete_mask = ops->req_complete_mask;
+ chip->vendor.req_complete_val = ops->req_complete_val;
+ chip->vendor.req_canceled = ops->req_canceled;
+ chip->vendor.recv = ops->recv;
+ chip->vendor.send = ops->send;
+ chip->vendor.cancel = ops->cancel;
+ chip->vendor.status = ops->status;

chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 14ba162..a56af2c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -64,8 +64,8 @@ enum tpm_duration {
struct tpm_chip;

struct tpm_vendor_specific {
- const u8 req_complete_mask;
- const u8 req_complete_val;
+ u8 req_complete_mask;
+ u8 req_complete_val;
bool (*req_canceled)(struct tpm_chip *chip, u8 status);
void __iomem *iobase; /* ioremapped address */
unsigned long base; /* TPM base address */
@@ -336,7 +336,7 @@ extern void tpm_gen_interrupt(struct tpm_chip *);
extern int tpm_do_selftest(struct tpm_chip *);
extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
extern struct tpm_chip* tpm_register_hardware(struct device *,
- const struct tpm_vendor_specific *);
+ const struct tpm_class_ops *ops);
extern void tpm_dev_vendor_release(struct tpm_chip *);
extern void tpm_remove_hardware(struct device *);
extern int tpm_pm_suspend(struct device *);
diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
index 7e665d0..6069d13 100644
--- a/drivers/char/tpm/tpm_atmel.c
+++ b/drivers/char/tpm/tpm_atmel.c
@@ -121,7 +121,7 @@ static bool tpm_atml_req_canceled(struct tpm_chip *chip, u8 status)
return (status == ATML_STATUS_READY);
}

-static const struct tpm_vendor_specific tpm_atmel = {
+static const struct tpm_class_ops tpm_atmel = {
.recv = tpm_atml_recv,
.send = tpm_atml_send,
.cancel = tpm_atml_cancel,
diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
index 8476a05..ecdd32a 100644
--- a/drivers/char/tpm/tpm_i2c_atmel.c
+++ b/drivers/char/tpm/tpm_i2c_atmel.c
@@ -140,7 +140,7 @@ static bool i2c_atmel_req_canceled(struct tpm_chip *chip, u8 status)
return 0;
}

-static const struct tpm_vendor_specific i2c_atmel = {
+static const struct tpm_class_ops i2c_atmel = {
.status = i2c_atmel_read_status,
.recv = i2c_atmel_recv,
.send = i2c_atmel_send,
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index ac1218f..52b9b2b 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -566,7 +566,7 @@ static bool tpm_tis_i2c_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static struct tpm_vendor_specific tpm_tis_i2c = {
+static const struct tpm_class_ops tpm_tis_i2c = {
.status = tpm_tis_i2c_status,
.recv = tpm_tis_i2c_recv,
.send = tpm_tis_i2c_send,
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index 5cb9670..ea60aa3 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -456,7 +456,7 @@ static bool i2c_nuvoton_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static const struct tpm_vendor_specific tpm_i2c = {
+static const struct tpm_class_ops tpm_i2c = {
.status = i2c_nuvoton_read_status,
.recv = i2c_nuvoton_recv,
.send = i2c_nuvoton_send,
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 588e763..0ef5d94 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -569,7 +569,7 @@ static bool tpm_st33_i2c_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static struct tpm_vendor_specific st_i2c_tpm = {
+static const struct tpm_class_ops st_i2c_tpm = {
.send = tpm_stm_i2c_send,
.recv = tpm_stm_i2c_recv,
.cancel = tpm_stm_i2c_cancel,
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 6217d1d..e915747 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -403,7 +403,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
return (status == 0);
}

-static const struct tpm_vendor_specific tpm_ibmvtpm = {
+static const struct tpm_class_ops tpm_ibmvtpm = {
.recv = tpm_ibmvtpm_recv,
.send = tpm_ibmvtpm_send,
.cancel = tpm_ibmvtpm_cancel,
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index 9525be5..dc0a255 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -371,7 +371,7 @@ static u8 tpm_inf_status(struct tpm_chip *chip)
return tpm_data_in(STAT);
}

-static const struct tpm_vendor_specific tpm_inf = {
+static const struct tpm_class_ops tpm_inf = {
.recv = tpm_inf_recv,
.send = tpm_inf_send,
.cancel = tpm_inf_cancel,
diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
index ad980be..3179ec9 100644
--- a/drivers/char/tpm/tpm_nsc.c
+++ b/drivers/char/tpm/tpm_nsc.c
@@ -232,7 +232,7 @@ static bool tpm_nsc_req_canceled(struct tpm_chip *chip, u8 status)
return (status == NSC_STATUS_RDY);
}

-static const struct tpm_vendor_specific tpm_nsc = {
+static const struct tpm_class_ops tpm_nsc = {
.recv = tpm_nsc_recv,
.send = tpm_nsc_send,
.cancel = tpm_nsc_cancel,
diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
index af46f0d..619545f 100644
--- a/drivers/char/tpm/tpm_spi_stm_st33.c
+++ b/drivers/char/tpm/tpm_spi_stm_st33.c
@@ -591,7 +591,7 @@ out:
return size;
}

-static struct tpm_vendor_specific st_spi_tpm = {
+static const struct tpm_class_ops st_spi_tpm = {
.send = tpm_stm_spi_send,
.recv = tpm_stm_spi_recv,
.cancel = tpm_stm_spi_cancel,
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index a362ede..8094b08 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -432,7 +432,7 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
}
}

-static struct tpm_vendor_specific tpm_tis = {
+static const struct tpm_class_ops tpm_tis = {
.status = tpm_tis_status,
.recv = tpm_tis_recv,
.send = tpm_tis_send,
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 7892557..60d5420 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -168,7 +168,7 @@ ssize_t tpm_store_locality(struct device *dev, struct device_attribute *attr,
return len;
}

-static const struct tpm_vendor_specific tpm_vtpm = {
+static const struct tpm_class_ops tpm_vtpm = {
.status = vtpm_status,
.recv = vtpm_recv,
.send = vtpm_send,
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 9a9051b..a14bf63 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -29,6 +29,21 @@
*/
#define TPM_ANY_NUM 0xFFFF

+struct tpm_chip;
+
+struct tpm_class_ops {
+ const u8 req_complete_mask;
+ const u8 req_complete_val;
+ bool (*req_canceled)(struct tpm_chip *chip, u8 status);
+ int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
+ int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
+ void (*cancel) (struct tpm_chip *chip);
+ u8 (*status) (struct tpm_chip *chip);
+};
+
+struct tpm_chip *tpmm_alloc_dev(struct device *dev,
+ const struct tpm_class_ops *ops);
+
#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)

extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
--
1.8.1.2

2013-09-23 18:20:08

by Jason Gunthorpe

Subject: [PATCH 12/13] tpm: st33: Remove chip->data_buffer access from this driver

For some reason this driver thinks that chip->data_buffer needs
to be set before it can call tpm_pm_*. This is not true. data_buffer
is used only by /dev/tpmX, which is why it is managed exclusively
by the fops functions.

Cc: Mathias Leblanc <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm_i2c_stm_st33.c | 8 --------
drivers/char/tpm/tpm_spi_stm_st33.c | 8 --------
2 files changed, 16 deletions(-)

diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index cd8c3e3..cb39876 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -770,24 +770,18 @@ static int tpm_st33_i2c_remove(struct i2c_client *client)
#ifdef CONFIG_PM_SLEEP
/*
* tpm_st33_i2c_pm_suspend suspend the TPM device
- * Added: Work around when suspend and no tpm application is running, suspend
- * may fail because chip->data_buffer is not set (only set in tpm_open in Linux
- * TPM core)
* @param: client, the i2c_client drescription (TPM I2C description).
* @param: mesg, the power management message.
* @return: 0 in case of success.
*/
static int tpm_st33_i2c_pm_suspend(struct device *dev)
{
- struct tpm_chip *chip = dev_get_drvdata(dev);
struct st33zp24_platform_data *pin_infos = dev->platform_data;
int ret = 0;

if (power_mgt) {
gpio_set_value(pin_infos->io_lpcpd, 0);
} else {
- if (chip->data_buffer == NULL)
- chip->data_buffer = pin_infos->tpm_i2c_buffer[0];
ret = tpm_pm_suspend(dev);
}
return ret;
@@ -812,8 +806,6 @@ static int tpm_st33_i2c_pm_resume(struct device *dev)
TPM_STS_VALID) == TPM_STS_VALID,
chip->vendor.timeout_b);
} else {
- if (chip->data_buffer == NULL)
- chip->data_buffer = pin_infos->tpm_i2c_buffer[0];
ret = tpm_pm_resume(dev);
if (!ret)
tpm_do_selftest(chip);
diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
index 22f6e83..81c61f6 100644
--- a/drivers/char/tpm/tpm_spi_stm_st33.c
+++ b/drivers/char/tpm/tpm_spi_stm_st33.c
@@ -808,22 +808,16 @@ static int tpm_st33_spi_remove(struct spi_device *client)
#ifdef CONFIG_PM_SLEEP
/*
* tpm_st33_spi_pm_suspend suspend the TPM device
- * Added: Work around when suspend and no tpm application is running, suspend
- * may fail because chip->data_buffer is not set (only set in tpm_open in Linux
- * TPM core)
* @return: 0 in case of success.
*/
static int tpm_st33_spi_pm_suspend(struct device *dev)
{
- struct tpm_chip *chip = dev_get_drvdata(dev);
struct st33zp24_platform_data *pin_infos = dev->platform_data;
int ret = 0;

if (power_mgt)
gpio_set_value(pin_infos->io_lpcpd, 0);
else {
- if (chip->data_buffer == NULL)
- chip->data_buffer = pin_infos->tpm_spi_buffer[0];
ret = tpm_pm_suspend(dev);
}
return ret;
@@ -847,8 +841,6 @@ static int tpm_st33_spi_pm_resume(struct device *dev)
TPM_STS_VALID) == TPM_STS_VALID,
chip->vendor.timeout_b);
} else {
- if (chip->data_buffer == NULL)
- chip->data_buffer = pin_infos->tpm_spi_buffer[0];
ret = tpm_pm_resume(dev);
if (!ret)
tpm_do_selftest(chip);
--
1.8.1.2

2013-09-23 18:20:29

by Jason Gunthorpe

Subject: [PATCH 13/13] tpm: Make tpm-dev allocate a per-file structure

This consolidates everything that is only used within tpm-dev.c
into tpm-dev.c and out of the publicly visible struct tpm_chip.

The per-file allocation lays the ground work for someday fixing the
strange forced O_EXCL behaviour of the current code.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm-dev.c | 100 ++++++++++++++++++++++++++-------------------
drivers/char/tpm/tpm.h | 7 ----
2 files changed, 57 insertions(+), 50 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 8d94e97..f492eb6 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -22,21 +22,34 @@
#include <linux/uaccess.h>
#include "tpm.h"

+struct file_priv {
+ struct tpm_chip *chip;
+
+ /* Data passed to and from the tpm via the read/write calls */
+ atomic_t data_pending;
+ struct mutex buffer_mutex;
+
+ struct timer_list user_read_timer; /* user needs to claim result */
+ struct work_struct work;
+
+ u8 data_buffer[TPM_BUFSIZE];
+};
+
static void user_reader_timeout(unsigned long ptr)
{
- struct tpm_chip *chip = (struct tpm_chip *) ptr;
+ struct file_priv *priv = (struct file_priv *)ptr;

- schedule_work(&chip->work);
+ schedule_work(&priv->work);
}

static void timeout_work(struct work_struct *work)
{
- struct tpm_chip *chip = container_of(work, struct tpm_chip, work);
+ struct file_priv *priv = container_of(work, struct file_priv, work);

- mutex_lock(&chip->buffer_mutex);
- atomic_set(&chip->data_pending, 0);
- memset(chip->data_buffer, 0, TPM_BUFSIZE);
- mutex_unlock(&chip->buffer_mutex);
+ mutex_lock(&priv->buffer_mutex);
+ atomic_set(&priv->data_pending, 0);
+ memset(priv->data_buffer, 0, sizeof(priv->data_buffer));
+ mutex_unlock(&priv->buffer_mutex);
}

static int tpm_open(struct inode *inode, struct file *file)
@@ -44,6 +57,7 @@ static int tpm_open(struct inode *inode, struct file *file)
struct miscdevice *misc = file->private_data;
struct tpm_chip *chip = container_of(misc, struct tpm_chip,
vendor.miscdev);
+ struct file_priv *priv;

/* It's assured that the chip will be opened just once,
* by the check of is_open variable, which is protected
@@ -53,15 +67,20 @@ static int tpm_open(struct inode *inode, struct file *file)
return -EBUSY;
}

- chip->data_buffer = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
- if (chip->data_buffer == NULL) {
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (priv == NULL) {
clear_bit(0, &chip->is_open);
return -ENOMEM;
}

- atomic_set(&chip->data_pending, 0);
+ priv->chip = chip;
+ atomic_set(&priv->data_pending, 0);
+ mutex_init(&priv->buffer_mutex);
+ setup_timer(&priv->user_read_timer, user_reader_timeout,
+ (unsigned long)priv);
+ INIT_WORK(&priv->work, timeout_work);

- file->private_data = chip;
+ file->private_data = priv;
get_device(chip->dev);
return 0;
}
@@ -69,28 +88,28 @@ static int tpm_open(struct inode *inode, struct file *file)
static ssize_t tpm_read(struct file *file, char __user *buf,
size_t size, loff_t *off)
{
- struct tpm_chip *chip = file->private_data;
+ struct file_priv *priv = file->private_data;
ssize_t ret_size;
int rc;

- del_singleshot_timer_sync(&chip->user_read_timer);
- flush_work(&chip->work);
- ret_size = atomic_read(&chip->data_pending);
+ del_singleshot_timer_sync(&priv->user_read_timer);
+ flush_work(&priv->work);
+ ret_size = atomic_read(&priv->data_pending);
if (ret_size > 0) { /* relay data */
ssize_t orig_ret_size = ret_size;
if (size < ret_size)
ret_size = size;

- mutex_lock(&chip->buffer_mutex);
- rc = copy_to_user(buf, chip->data_buffer, ret_size);
- memset(chip->data_buffer, 0, orig_ret_size);
+ mutex_lock(&priv->buffer_mutex);
+ rc = copy_to_user(buf, priv->data_buffer, ret_size);
+ memset(priv->data_buffer, 0, orig_ret_size);
if (rc)
ret_size = -EFAULT;

- mutex_unlock(&chip->buffer_mutex);
+ mutex_unlock(&priv->buffer_mutex);
}

- atomic_set(&chip->data_pending, 0);
+ atomic_set(&priv->data_pending, 0);

return ret_size;
}
@@ -98,7 +117,7 @@ static ssize_t tpm_read(struct file *file, char __user *buf,
static ssize_t tpm_write(struct file *file, const char __user *buf,
size_t size, loff_t *off)
{
- struct tpm_chip *chip = file->private_data;
+ struct file_priv *priv = file->private_data;
size_t in_size = size;
ssize_t out_size;

@@ -106,32 +125,33 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
either via tpm_read or a user_read_timer timeout.
This also prevents splitted buffered writes from blocking here.
*/
- if (atomic_read(&chip->data_pending) != 0)
+ if (atomic_read(&priv->data_pending) != 0)
return -EBUSY;

if (in_size > TPM_BUFSIZE)
return -E2BIG;

- mutex_lock(&chip->buffer_mutex);
+ mutex_lock(&priv->buffer_mutex);

if (copy_from_user
- (chip->data_buffer, (void __user *) buf, in_size)) {
- mutex_unlock(&chip->buffer_mutex);
+ (priv->data_buffer, (void __user *) buf, in_size)) {
+ mutex_unlock(&priv->buffer_mutex);
return -EFAULT;
}

/* atomic tpm command send and result receive */
- out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);
+ out_size = tpm_transmit(priv->chip, priv->data_buffer,
+ sizeof(priv->data_buffer));
if (out_size < 0) {
- mutex_unlock(&chip->buffer_mutex);
+ mutex_unlock(&priv->buffer_mutex);
return out_size;
}

- atomic_set(&chip->data_pending, out_size);
- mutex_unlock(&chip->buffer_mutex);
+ atomic_set(&priv->data_pending, out_size);
+ mutex_unlock(&priv->buffer_mutex);

/* Set a timeout by which the reader must come claim the result */
- mod_timer(&chip->user_read_timer, jiffies + (60 * HZ));
+ mod_timer(&priv->user_read_timer, jiffies + (60 * HZ));

return in_size;
}
@@ -141,15 +161,15 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
*/
static int tpm_release(struct inode *inode, struct file *file)
{
- struct tpm_chip *chip = file->private_data;
+ struct file_priv *priv = file->private_data;

- del_singleshot_timer_sync(&chip->user_read_timer);
- flush_work(&chip->work);
+ del_singleshot_timer_sync(&priv->user_read_timer);
+ flush_work(&priv->work);
file->private_data = NULL;
- atomic_set(&chip->data_pending, 0);
- kzfree(chip->data_buffer);
- clear_bit(0, &chip->is_open);
- put_device(chip->dev);
+ atomic_set(&priv->data_pending, 0);
+ clear_bit(0, &priv->chip->is_open);
+ put_device(priv->chip->dev);
+ kfree(priv);
return 0;
}

@@ -166,12 +186,6 @@ int tpm_dev_add_device(struct tpm_chip *chip)
{
int rc;

- mutex_init(&chip->buffer_mutex);
- INIT_WORK(&chip->work, timeout_work);
-
- setup_timer(&chip->user_read_timer, user_reader_timeout,
- (unsigned long)chip);
-
chip->vendor.miscdev.fops = &tpm_fops;
if (chip->dev_num == 0)
chip->vendor.miscdev.minor = TPM_MINOR;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7b0a46e..e4d0888 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -103,13 +103,6 @@ struct tpm_chip {
unsigned long is_open; /* only one allowed */
int time_expired;

- /* Data passed to and from the tpm via the read/write calls */
- u8 *data_buffer;
- atomic_t data_pending;
- struct mutex buffer_mutex;
-
- struct timer_list user_read_timer; /* user needs to claim result */
- struct work_struct work;
struct mutex tpm_mutex; /* tpm is processing */

struct tpm_vendor_specific vendor;
--
1.8.1.2

2013-09-23 18:21:23

by Jason Gunthorpe

Subject: [PATCH 07/13] tpm: Remove tpm_show_caps_1_2

The version of the TPM should not depend on the bus it is connected
through. 1.1, 1.2 and soon 2.0 TPMS will be all be able to use the
same bus interfaces.

Make tpm_show_caps try the 1.2 capability first. If that fails then
fall back to the 1.1 capability. This effectively auto-detects what
interface the TPM supports at run-time.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm.c | 56 +++++++++++++++----------------------
drivers/char/tpm/tpm.h | 2 --
drivers/char/tpm/tpm_i2c_atmel.c | 2 +-
drivers/char/tpm/tpm_i2c_infineon.c | 2 +-
drivers/char/tpm/tpm_i2c_nuvoton.c | 2 +-
drivers/char/tpm/tpm_i2c_stm_st33.c | 2 +-
drivers/char/tpm/tpm_ibmvtpm.c | 2 +-
drivers/char/tpm/tpm_spi_stm_st33.c | 2 +-
drivers/char/tpm/tpm_tis.c | 2 +-
9 files changed, 30 insertions(+), 42 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index c3ab508..72f0c68 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1020,43 +1020,33 @@ ssize_t tpm_show_caps(struct device *dev, struct device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
be32_to_cpu(cap.manufacturer_id));

- rc = tpm_getcap(dev, CAP_VERSION_1_1, &cap,
- "attempting to determine the 1.1 version");
- if (rc)
- return 0;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version.Major, cap.tpm_version.Minor,
- cap.tpm_version.revMajor, cap.tpm_version.revMinor);
- return str - buf;
-}
-EXPORT_SYMBOL_GPL(tpm_show_caps);
-
-ssize_t tpm_show_caps_1_2(struct device * dev,
- struct device_attribute * attr, char *buf)
-{
- cap_t cap;
- ssize_t rc;
- char *str = buf;
-
- rc = tpm_getcap(dev, TPM_CAP_PROP_MANUFACTURER, &cap,
- "attempting to determine the manufacturer");
- if (rc)
- return 0;
- str += sprintf(str, "Manufacturer: 0x%x\n",
- be32_to_cpu(cap.manufacturer_id));
+ /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
rc = tpm_getcap(dev, CAP_VERSION_1_2, &cap,
"attempting to determine the 1.2 version");
- if (rc)
- return 0;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version_1_2.Major, cap.tpm_version_1_2.Minor,
- cap.tpm_version_1_2.revMajor,
- cap.tpm_version_1_2.revMinor);
+ if (!rc) {
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ cap.tpm_version_1_2.Major,
+ cap.tpm_version_1_2.Minor,
+ cap.tpm_version_1_2.revMajor,
+ cap.tpm_version_1_2.revMinor);
+ } else {
+ /* Otherwise just use TPM_STRUCT_VER */
+ rc = tpm_getcap(dev, CAP_VERSION_1_1, &cap,
+ "attempting to determine the 1.1 version");
+ if (rc)
+ return 0;
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ cap.tpm_version.Major,
+ cap.tpm_version.Minor,
+ cap.tpm_version.revMajor,
+ cap.tpm_version.revMinor);
+ }
+
return str - buf;
}
-EXPORT_SYMBOL_GPL(tpm_show_caps_1_2);
+EXPORT_SYMBOL_GPL(tpm_show_caps);

ssize_t tpm_show_durations(struct device *dev, struct device_attribute *attr,
char *buf)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 0df18b5..f328478 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -59,8 +59,6 @@ extern ssize_t tpm_show_pcrs(struct device *, struct device_attribute *attr,
char *);
extern ssize_t tpm_show_caps(struct device *, struct device_attribute *attr,
char *);
-extern ssize_t tpm_show_caps_1_2(struct device *, struct device_attribute *attr,
- char *);
extern ssize_t tpm_store_cancel(struct device *, struct device_attribute *attr,
const char *, size_t);
extern ssize_t tpm_show_enabled(struct device *, struct device_attribute *attr,
diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
index 79877f0..e986d01 100644
--- a/drivers/char/tpm/tpm_i2c_atmel.c
+++ b/drivers/char/tpm/tpm_i2c_atmel.c
@@ -150,7 +150,7 @@ static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index e33d8e5..fefd2aa 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -581,7 +581,7 @@ static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index 4f74e7d..6276fea 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -471,7 +471,7 @@ static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index 1c68d93..b069afb 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -584,7 +584,7 @@ static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);

static struct attribute *stm_tpm_attrs[] = {
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 838f043..2783a42 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -419,7 +419,7 @@ static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
index f5e3cd6..7b8899d 100644
--- a/drivers/char/tpm/tpm_spi_stm_st33.c
+++ b/drivers/char/tpm/tpm_spi_stm_st33.c
@@ -606,7 +606,7 @@ static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated, NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);

static struct attribute *stm_tpm_attrs[] = {
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 5796d01..1b74459 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -448,7 +448,7 @@ static DEVICE_ATTR(active, S_IRUGO, tpm_show_active, NULL);
static DEVICE_ATTR(owned, S_IRUGO, tpm_show_owned, NULL);
static DEVICE_ATTR(temp_deactivated, S_IRUGO, tpm_show_temp_deactivated,
NULL);
-static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
+static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
--
1.8.1.2

2013-09-23 18:21:21

by Jason Gunthorpe

Subject: [PATCH 08/13] tpm: Pull everything related to /dev/tpmX into tpm-dev.c

CLASS-dev.c is a common idiom for Linux subsystems

This pulls all the code related to the miscdev into tpm-dev.c and makes it
static. The identical file_operation structs in the drivers are purged and the
tpm common code unconditionally creates the miscdev.

Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/Makefile | 2 +-
drivers/char/tpm/tpm-dev.c | 199 ++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm.c | 178 ++------------------------------
drivers/char/tpm/tpm.h | 11 +-
drivers/char/tpm/tpm_atmel.c | 10 --
drivers/char/tpm/tpm_i2c_atmel.c | 10 --
drivers/char/tpm/tpm_i2c_infineon.c | 10 --
drivers/char/tpm/tpm_i2c_nuvoton.c | 10 --
drivers/char/tpm/tpm_i2c_stm_st33.c | 10 --
drivers/char/tpm/tpm_ibmvtpm.c | 10 --
drivers/char/tpm/tpm_infineon.c | 10 --
drivers/char/tpm/tpm_nsc.c | 10 --
drivers/char/tpm/tpm_spi_stm_st33.c | 10 --
drivers/char/tpm/tpm_tis.c | 11 --
drivers/char/tpm/xen-tpmfront.c | 12 ---
15 files changed, 216 insertions(+), 287 deletions(-)
create mode 100644 drivers/char/tpm/tpm-dev.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 55dfe87..5a73665 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -1,7 +1,7 @@
#
# Makefile for the kernel tpm device drivers.
#
-obj-$(CONFIG_TCG_TPM) += tpm.o
+obj-$(CONFIG_TCG_TPM) += tpm.o tpm-dev.o
ifdef CONFIG_ACPI
obj-$(CONFIG_TCG_TPM) += tpm_bios.o
tpm_bios-objs += tpm_eventlog.o tpm_acpi.o tpm_ppi.o
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
new file mode 100644
index 0000000..8d94e97
--- /dev/null
+++ b/drivers/char/tpm/tpm-dev.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Authors:
+ * Leendert van Doorn <[email protected]>
+ * Dave Safford <[email protected]>
+ * Reiner Sailer <[email protected]>
+ * Kylene Hall <[email protected]>
+ *
+ * Copyright (C) 2013 Obsidian Reearch Corp
+ * Jason Gunthorpe <[email protected]>
+ *
+ * Device file system 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/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include "tpm.h"
+
+static void user_reader_timeout(unsigned long ptr)
+{
+ struct tpm_chip *chip = (struct tpm_chip *) ptr;
+
+ schedule_work(&chip->work);
+}
+
+static void timeout_work(struct work_struct *work)
+{
+ struct tpm_chip *chip = container_of(work, struct tpm_chip, work);
+
+ mutex_lock(&chip->buffer_mutex);
+ atomic_set(&chip->data_pending, 0);
+ memset(chip->data_buffer, 0, TPM_BUFSIZE);
+ mutex_unlock(&chip->buffer_mutex);
+}
+
+static int tpm_open(struct inode *inode, struct file *file)
+{
+ struct miscdevice *misc = file->private_data;
+ struct tpm_chip *chip = container_of(misc, struct tpm_chip,
+ vendor.miscdev);
+
+ /* It's assured that the chip will be opened just once,
+ * by the check of is_open variable, which is protected
+ * by driver_lock. */
+ if (test_and_set_bit(0, &chip->is_open)) {
+ dev_dbg(chip->dev, "Another process owns this TPM\n");
+ return -EBUSY;
+ }
+
+ chip->data_buffer = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
+ if (chip->data_buffer == NULL) {
+ clear_bit(0, &chip->is_open);
+ return -ENOMEM;
+ }
+
+ atomic_set(&chip->data_pending, 0);
+
+ file->private_data = chip;
+ get_device(chip->dev);
+ return 0;
+}
+
+static ssize_t tpm_read(struct file *file, char __user *buf,
+ size_t size, loff_t *off)
+{
+ struct tpm_chip *chip = file->private_data;
+ ssize_t ret_size;
+ int rc;
+
+ del_singleshot_timer_sync(&chip->user_read_timer);
+ flush_work(&chip->work);
+ ret_size = atomic_read(&chip->data_pending);
+ if (ret_size > 0) { /* relay data */
+ ssize_t orig_ret_size = ret_size;
+ if (size < ret_size)
+ ret_size = size;
+
+ mutex_lock(&chip->buffer_mutex);
+ rc = copy_to_user(buf, chip->data_buffer, ret_size);
+ memset(chip->data_buffer, 0, orig_ret_size);
+ if (rc)
+ ret_size = -EFAULT;
+
+ mutex_unlock(&chip->buffer_mutex);
+ }
+
+ atomic_set(&chip->data_pending, 0);
+
+ return ret_size;
+}
+
+static ssize_t tpm_write(struct file *file, const char __user *buf,
+ size_t size, loff_t *off)
+{
+ struct tpm_chip *chip = file->private_data;
+ size_t in_size = size;
+ ssize_t out_size;
+
+ /* cannot perform a write until the read has cleared
+ either via tpm_read or a user_read_timer timeout.
+ This also prevents splitted buffered writes from blocking here.
+ */
+ if (atomic_read(&chip->data_pending) != 0)
+ return -EBUSY;
+
+ if (in_size > TPM_BUFSIZE)
+ return -E2BIG;
+
+ mutex_lock(&chip->buffer_mutex);
+
+ if (copy_from_user
+ (chip->data_buffer, (void __user *) buf, in_size)) {
+ mutex_unlock(&chip->buffer_mutex);
+ return -EFAULT;
+ }
+
+ /* atomic tpm command send and result receive */
+ out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);
+ if (out_size < 0) {
+ mutex_unlock(&chip->buffer_mutex);
+ return out_size;
+ }
+
+ atomic_set(&chip->data_pending, out_size);
+ mutex_unlock(&chip->buffer_mutex);
+
+ /* Set a timeout by which the reader must come claim the result */
+ mod_timer(&chip->user_read_timer, jiffies + (60 * HZ));
+
+ return in_size;
+}
+
+/*
+ * Called on file close
+ */
+static int tpm_release(struct inode *inode, struct file *file)
+{
+ struct tpm_chip *chip = file->private_data;
+
+ del_singleshot_timer_sync(&chip->user_read_timer);
+ flush_work(&chip->work);
+ file->private_data = NULL;
+ atomic_set(&chip->data_pending, 0);
+ kzfree(chip->data_buffer);
+ clear_bit(0, &chip->is_open);
+ put_device(chip->dev);
+ return 0;
+}
+
+static const struct file_operations tpm_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .open = tpm_open,
+ .read = tpm_read,
+ .write = tpm_write,
+ .release = tpm_release,
+};
+
+int tpm_dev_add_device(struct tpm_chip *chip)
+{
+ int rc;
+
+ mutex_init(&chip->buffer_mutex);
+ INIT_WORK(&chip->work, timeout_work);
+
+ setup_timer(&chip->user_read_timer, user_reader_timeout,
+ (unsigned long)chip);
+
+ chip->vendor.miscdev.fops = &tpm_fops;
+ if (chip->dev_num == 0)
+ chip->vendor.miscdev.minor = TPM_MINOR;
+ else
+ chip->vendor.miscdev.minor = MISC_DYNAMIC_MINOR;
+
+ chip->vendor.miscdev.name = chip->devname;
+ chip->vendor.miscdev.parent = chip->dev;
+
+ rc = misc_register(&chip->vendor.miscdev);
+ if (rc) {
+ chip->vendor.miscdev.name = NULL;
+ dev_warn(chip->dev,
+ "unable to misc_register %s, minor %d err=%d\n",
+ chip->vendor.miscdev.name,
+ chip->vendor.miscdev.minor, rc);
+ }
+ return rc;
+}
+
+void tpm_dev_del_device(struct tpm_chip *chip)
+{
+ if (chip->vendor.miscdev.name)
+ misc_deregister(&chip->vendor.miscdev);
+}
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 72f0c68..63f1c45 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -312,23 +312,6 @@ static const u8 tpm_ordinal_duration[TPM_MAX_ORDINAL] = {
TPM_MEDIUM,
};

-static void user_reader_timeout(unsigned long ptr)
-{
- struct tpm_chip *chip = (struct tpm_chip *) ptr;
-
- schedule_work(&chip->work);
-}
-
-static void timeout_work(struct work_struct *work)
-{
- struct tpm_chip *chip = container_of(work, struct tpm_chip, work);
-
- mutex_lock(&chip->buffer_mutex);
- atomic_set(&chip->data_pending, 0);
- memset(chip->data_buffer, 0, TPM_BUFSIZE);
- mutex_unlock(&chip->buffer_mutex);
-}
-
/*
* Returns max number of jiffies to wait
*/
@@ -355,8 +338,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
/*
* Internal kernel interface to transmit TPM commands
*/
-static ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
- size_t bufsiz)
+ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
+ size_t bufsiz)
{
ssize_t rc;
u32 count, ordinal;
@@ -1151,127 +1134,6 @@ again:
return -ETIME;
}
EXPORT_SYMBOL_GPL(wait_for_tpm_stat);
-/*
- * Device file system interface to the TPM
- *
- * It's assured that the chip will be opened just once,
- * by the check of is_open variable, which is protected
- * by driver_lock.
- */
-int tpm_open(struct inode *inode, struct file *file)
-{
- struct miscdevice *misc = file->private_data;
- struct tpm_chip *chip = container_of(misc, struct tpm_chip,
- vendor.miscdev);
-
- if (test_and_set_bit(0, &chip->is_open)) {
- dev_dbg(chip->dev, "Another process owns this TPM\n");
- return -EBUSY;
- }
-
- chip->data_buffer = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
- if (chip->data_buffer == NULL) {
- clear_bit(0, &chip->is_open);
- return -ENOMEM;
- }
-
- atomic_set(&chip->data_pending, 0);
-
- file->private_data = chip;
- get_device(chip->dev);
- return 0;
-}
-EXPORT_SYMBOL_GPL(tpm_open);
-
-/*
- * Called on file close
- */
-int tpm_release(struct inode *inode, struct file *file)
-{
- struct tpm_chip *chip = file->private_data;
-
- del_singleshot_timer_sync(&chip->user_read_timer);
- flush_work(&chip->work);
- file->private_data = NULL;
- atomic_set(&chip->data_pending, 0);
- kzfree(chip->data_buffer);
- clear_bit(0, &chip->is_open);
- put_device(chip->dev);
- return 0;
-}
-EXPORT_SYMBOL_GPL(tpm_release);
-
-ssize_t tpm_write(struct file *file, const char __user *buf,
- size_t size, loff_t *off)
-{
- struct tpm_chip *chip = file->private_data;
- size_t in_size = size;
- ssize_t out_size;
-
- /* cannot perform a write until the read has cleared
- either via tpm_read or a user_read_timer timeout.
- This also prevents splitted buffered writes from blocking here.
- */
- if (atomic_read(&chip->data_pending) != 0)
- return -EBUSY;
-
- if (in_size > TPM_BUFSIZE)
- return -E2BIG;
-
- mutex_lock(&chip->buffer_mutex);
-
- if (copy_from_user
- (chip->data_buffer, (void __user *) buf, in_size)) {
- mutex_unlock(&chip->buffer_mutex);
- return -EFAULT;
- }
-
- /* atomic tpm command send and result receive */
- out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);
- if (out_size < 0) {
- mutex_unlock(&chip->buffer_mutex);
- return out_size;
- }
-
- atomic_set(&chip->data_pending, out_size);
- mutex_unlock(&chip->buffer_mutex);
-
- /* Set a timeout by which the reader must come claim the result */
- mod_timer(&chip->user_read_timer, jiffies + (60 * HZ));
-
- return in_size;
-}
-EXPORT_SYMBOL_GPL(tpm_write);
-
-ssize_t tpm_read(struct file *file, char __user *buf,
- size_t size, loff_t *off)
-{
- struct tpm_chip *chip = file->private_data;
- ssize_t ret_size;
- int rc;
-
- del_singleshot_timer_sync(&chip->user_read_timer);
- flush_work(&chip->work);
- ret_size = atomic_read(&chip->data_pending);
- if (ret_size > 0) { /* relay data */
- ssize_t orig_ret_size = ret_size;
- if (size < ret_size)
- ret_size = size;
-
- mutex_lock(&chip->buffer_mutex);
- rc = copy_to_user(buf, chip->data_buffer, ret_size);
- memset(chip->data_buffer, 0, orig_ret_size);
- if (rc)
- ret_size = -EFAULT;
-
- mutex_unlock(&chip->buffer_mutex);
- }
-
- atomic_set(&chip->data_pending, 0);
-
- return ret_size;
-}
-EXPORT_SYMBOL_GPL(tpm_read);

void tpm_remove_hardware(struct device *dev)
{
@@ -1287,7 +1149,7 @@ void tpm_remove_hardware(struct device *dev)
spin_unlock(&driver_lock);
synchronize_rcu();

- misc_deregister(&chip->vendor.miscdev);
+ tpm_dev_del_device(chip);
sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
tpm_remove_ppi(&dev->kobj);
tpm_bios_log_teardown(chip->bios_dir);
@@ -1480,15 +1342,9 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,
if (chip == NULL)
return NULL;

- mutex_init(&chip->buffer_mutex);
mutex_init(&chip->tpm_mutex);
INIT_LIST_HEAD(&chip->list);

- INIT_WORK(&chip->work, timeout_work);
-
- setup_timer(&chip->user_read_timer, user_reader_timeout,
- (unsigned long)chip);
-
memcpy(&chip->vendor, entry, sizeof(struct tpm_vendor_specific));

chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
@@ -1496,40 +1352,26 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,
if (chip->dev_num >= TPM_NUM_DEVICES) {
dev_err(dev, "No available tpm device numbers\n");
goto out_free;
- } else if (chip->dev_num == 0)
- chip->vendor.miscdev.minor = TPM_MINOR;
- else
- chip->vendor.miscdev.minor = MISC_DYNAMIC_MINOR;
+ }

set_bit(chip->dev_num, dev_mask);

scnprintf(chip->devname, sizeof(chip->devname), "%s%d", "tpm",
chip->dev_num);
- chip->vendor.miscdev.name = chip->devname;

- chip->vendor.miscdev.parent = dev;
chip->dev = get_device(dev);
chip->release = dev->release;
dev->release = tpm_dev_release;
dev_set_drvdata(dev, chip);

- if (misc_register(&chip->vendor.miscdev)) {
- dev_err(chip->dev,
- "unable to misc_register %s, minor %d\n",
- chip->vendor.miscdev.name,
- chip->vendor.miscdev.minor);
+ if (tpm_dev_add_device(chip))
goto put_device;
- }

- if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
- misc_deregister(&chip->vendor.miscdev);
- goto put_device;
- }
+ if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group))
+ goto del_misc;

- if (tpm_add_ppi(&dev->kobj)) {
- misc_deregister(&chip->vendor.miscdev);
- goto put_device;
- }
+ if (tpm_add_ppi(&dev->kobj))
+ goto del_misc;

chip->bios_dir = tpm_bios_log_setup(chip->devname);

@@ -1540,6 +1382,8 @@ struct tpm_chip *tpm_register_hardware(struct device *dev,

return chip;

+del_misc:
+ tpm_dev_del_device(chip);
put_device:
put_device(chip->dev);
out_free:
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f328478..496228c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -323,25 +323,24 @@ struct tpm_cmd_t {

ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *);

+ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
+ size_t bufsiz);
extern int tpm_get_timeouts(struct tpm_chip *);
extern void tpm_gen_interrupt(struct tpm_chip *);
extern int tpm_do_selftest(struct tpm_chip *);
extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
extern struct tpm_chip* tpm_register_hardware(struct device *,
const struct tpm_vendor_specific *);
-extern int tpm_open(struct inode *, struct file *);
-extern int tpm_release(struct inode *, struct file *);
-extern void tpm_dev_release(struct device *dev);
extern void tpm_dev_vendor_release(struct tpm_chip *);
-extern ssize_t tpm_write(struct file *, const char __user *, size_t,
- loff_t *);
-extern ssize_t tpm_read(struct file *, char __user *, size_t, loff_t *);
extern void tpm_remove_hardware(struct device *);
extern int tpm_pm_suspend(struct device *);
extern int tpm_pm_resume(struct device *);
extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,
wait_queue_head_t *, bool);

+int tpm_dev_add_device(struct tpm_chip *chip);
+void tpm_dev_del_device(struct tpm_chip *chip);
+
#ifdef CONFIG_ACPI
extern int tpm_add_ppi(struct kobject *);
extern void tpm_remove_ppi(struct kobject *);
diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
index c9a528d..9692e2f 100644
--- a/drivers/char/tpm/tpm_atmel.c
+++ b/drivers/char/tpm/tpm_atmel.c
@@ -121,15 +121,6 @@ static bool tpm_atml_req_canceled(struct tpm_chip *chip, u8 status)
return (status == ATML_STATUS_READY);
}

-static const struct file_operations atmel_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
@@ -154,7 +145,6 @@ static const struct tpm_vendor_specific tpm_atmel = {
.req_complete_val = ATML_STATUS_DATA_AVAIL,
.req_canceled = tpm_atml_req_canceled,
.attr_group = &atmel_attr_grp,
- .miscdev = { .fops = &atmel_ops, },
};

static struct platform_device *pdev;
diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
index e986d01..f9cee5f 100644
--- a/drivers/char/tpm/tpm_i2c_atmel.c
+++ b/drivers/char/tpm/tpm_i2c_atmel.c
@@ -135,15 +135,6 @@ static u8 i2c_atmel_read_status(struct tpm_chip *chip)
return ATMEL_STS_OK;
}

-static const struct file_operations i2c_atmel_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -187,7 +178,6 @@ static const struct tpm_vendor_specific i2c_atmel = {
.req_complete_val = ATMEL_STS_OK,
.req_canceled = i2c_atmel_req_canceled,
.attr_group = &i2c_atmel_attr_grp,
- .miscdev.fops = &i2c_atmel_ops,
};

static int i2c_atmel_probe(struct i2c_client *client,
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index fefd2aa..c1ba7fa 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -566,15 +566,6 @@ static bool tpm_tis_i2c_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static const struct file_operations tis_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -613,7 +604,6 @@ static struct tpm_vendor_specific tpm_tis_i2c = {
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_i2c_req_canceled,
.attr_group = &tis_attr_grp,
- .miscdev.fops = &tis_ops,
};

static int tpm_tis_i2c_init(struct device *dev)
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index 6276fea..3766fe7 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -456,15 +456,6 @@ static bool i2c_nuvoton_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static const struct file_operations i2c_nuvoton_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -503,7 +494,6 @@ static const struct tpm_vendor_specific tpm_i2c = {
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = i2c_nuvoton_req_canceled,
.attr_group = &i2c_nuvoton_attr_grp,
- .miscdev.fops = &i2c_nuvoton_ops,
};

/* The only purpose for the handler is to signal to any waiting threads that
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c
index b069afb..2328c5e 100644
--- a/drivers/char/tpm/tpm_i2c_stm_st33.c
+++ b/drivers/char/tpm/tpm_i2c_stm_st33.c
@@ -569,15 +569,6 @@ static bool tpm_st33_i2c_req_canceled(struct tpm_chip *chip, u8 status)
return (status == TPM_STS_COMMAND_READY);
}

-static const struct file_operations tpm_st33_i2c_fops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .read = tpm_read,
- .write = tpm_write,
- .open = tpm_open,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -611,7 +602,6 @@ static struct tpm_vendor_specific st_i2c_tpm = {
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_st33_i2c_req_canceled,
.attr_group = &stm_tpm_attr_grp,
- .miscdev = {.fops = &tpm_st33_i2c_fops,},
};

static int interrupts;
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 2783a42..88a0121 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -403,15 +403,6 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
return (status == 0);
}

-static const struct file_operations ibmvtpm_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -448,7 +439,6 @@ static const struct tpm_vendor_specific tpm_ibmvtpm = {
.req_complete_val = 0,
.req_canceled = tpm_ibmvtpm_req_canceled,
.attr_group = &ibmvtpm_attr_grp,
- .miscdev = { .fops = &ibmvtpm_ops, },
};

static const struct dev_pm_ops tpm_ibmvtpm_pm_ops = {
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index 2b480c2..c75c10c 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -386,15 +386,6 @@ static struct attribute *inf_attrs[] = {

static struct attribute_group inf_attr_grp = {.attrs = inf_attrs };

-static const struct file_operations inf_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static const struct tpm_vendor_specific tpm_inf = {
.recv = tpm_inf_recv,
.send = tpm_inf_send,
@@ -403,7 +394,6 @@ static const struct tpm_vendor_specific tpm_inf = {
.req_complete_mask = 0,
.req_complete_val = 0,
.attr_group = &inf_attr_grp,
- .miscdev = {.fops = &inf_ops,},
};

static const struct pnp_device_id tpm_inf_pnp_tbl[] = {
diff --git a/drivers/char/tpm/tpm_nsc.c b/drivers/char/tpm/tpm_nsc.c
index 770c46f..a4acac9 100644
--- a/drivers/char/tpm/tpm_nsc.c
+++ b/drivers/char/tpm/tpm_nsc.c
@@ -232,15 +232,6 @@ static bool tpm_nsc_req_canceled(struct tpm_chip *chip, u8 status)
return (status == NSC_STATUS_RDY);
}

-static const struct file_operations nsc_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps, NULL);
@@ -265,7 +256,6 @@ static const struct tpm_vendor_specific tpm_nsc = {
.req_complete_val = NSC_STATUS_OBF,
.req_canceled = tpm_nsc_req_canceled,
.attr_group = &nsc_attr_grp,
- .miscdev = { .fops = &nsc_ops, },
};

static struct platform_device *pdev = NULL;
diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
index 7b8899d..99854e8 100644
--- a/drivers/char/tpm/tpm_spi_stm_st33.c
+++ b/drivers/char/tpm/tpm_spi_stm_st33.c
@@ -591,15 +591,6 @@ out:
return size;
}

-static const struct file_operations tpm_st33_spi_fops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .read = tpm_read,
- .write = tpm_write,
- .open = tpm_open,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -630,7 +621,6 @@ static struct tpm_vendor_specific st_spi_tpm = {
.cancel = tpm_stm_spi_cancel,
.status = tpm_stm_spi_status,
.attr_group = &stm_tpm_attr_grp,
- .miscdev = {.fops = &tpm_st33_spi_fops,},
};

static int evaluate_latency(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 1b74459..46f57f5 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -432,15 +432,6 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
}
}

-static const struct file_operations tis_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -479,8 +470,6 @@ static struct tpm_vendor_specific tpm_tis = {
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_req_canceled,
.attr_group = &tis_attr_grp,
- .miscdev = {
- .fops = &tis_ops,},
};

static irqreturn_t tis_int_probe(int irq, void *dev_id)
diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
index 7711928..12a4ab2 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -168,15 +168,6 @@ ssize_t tpm_store_locality(struct device *dev, struct device_attribute *attr,
return len;
}

-static const struct file_operations vtpm_ops = {
- .owner = THIS_MODULE,
- .llseek = no_llseek,
- .open = tpm_open,
- .read = tpm_read,
- .write = tpm_write,
- .release = tpm_release,
-};
-
static DEVICE_ATTR(pubek, S_IRUGO, tpm_show_pubek, NULL);
static DEVICE_ATTR(pcrs, S_IRUGO, tpm_show_pcrs, NULL);
static DEVICE_ATTR(enabled, S_IRUGO, tpm_show_enabled, NULL);
@@ -219,9 +210,6 @@ static const struct tpm_vendor_specific tpm_vtpm = {
.req_complete_val = VTPM_STATUS_IDLE | VTPM_STATUS_RESULT,
.req_canceled = vtpm_req_canceled,
.attr_group = &vtpm_attr_grp,
- .miscdev = {
- .fops = &vtpm_ops,
- },
};

static irqreturn_t tpmif_interrupt(int dummy, void *dev_id)
--
1.8.1.2

2013-09-23 18:52:46

by Konrad Rzeszutek Wilk

Subject: Re: [PATCH 03/13] tpm: xen-tpmfront: Fix default durations

On Mon, Sep 23, 2013 at 12:14:33PM -0600, Jason Gunthorpe wrote:
> All the default durations were being set to 10 minutes which is
> way too long for the timeouts. Normal values for the longest
> duration are around 5 mins, and short duration ar around .5s.
>
> Further, these are just the default, tpm_get_timeouts will set
> them to values from the TPM (or throw an error).
>
> Just remove them.

Sounds sensible.

Daniel?

>
> Cc: Daniel De Graaf <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> ---
> drivers/char/tpm/xen-tpmfront.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
> index 7a7929b..6f2fe2b 100644
> --- a/drivers/char/tpm/xen-tpmfront.c
> +++ b/drivers/char/tpm/xen-tpmfront.c
> @@ -210,8 +210,6 @@ static struct attribute_group vtpm_attr_grp = {
> .attrs = vtpm_attrs,
> };
>
> -#define TPM_LONG_TIMEOUT (10 * 60 * HZ)
> -
> static const struct tpm_vendor_specific tpm_vtpm = {
> .status = vtpm_status,
> .recv = vtpm_recv,
> @@ -224,11 +222,6 @@ static const struct tpm_vendor_specific tpm_vtpm = {
> .miscdev = {
> .fops = &vtpm_ops,
> },
> - .duration = {
> - TPM_LONG_TIMEOUT,
> - TPM_LONG_TIMEOUT,
> - TPM_LONG_TIMEOUT,
> - },
> };
>
> static irqreturn_t tpmif_interrupt(int dummy, void *dev_id)
> --
> 1.8.1.2
>

2013-09-23 19:10:33

by Daniel De Graaf

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On 09/23/2013 02:14 PM, Jason Gunthorpe wrote:
> CLASS-sysfs.c is a common idiom for linux subsystems.
>
> This pulls all the sysfs attribute functions and related code
> into tpm-sysfs.c. To support this change some constants are moved
> from tpm.c to tpm.h and __tpm_pcr_read is made non-static and is
> called tpm_pcr_read_dev.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>
[...]
> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
> index 12a4ab2..7892557 100644
> --- a/drivers/char/tpm/xen-tpmfront.c
> +++ b/drivers/char/tpm/xen-tpmfront.c
[...]
> -static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
> -static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
> -static DEVICE_ATTR(locality, S_IRUGO | S_IWUSR, tpm_show_locality,
> - tpm_store_locality);

This patch drops the "locality" sysfs attribute from xen-tpmfront. Since
that attribute is currently only implemented for the xen TPM driver, it
is best to leave it there for now (and its show/store functions could
also be made static, an oversight I just noticed now). If this attribute
is later made available on other TPM drivers, it may need to contain
device-specific logic, but such an implementation is well outside the
scope of this series.

--
Daniel De Graaf
National Security Agency

2013-09-23 19:12:56

by Daniel De Graaf

Subject: Re: [PATCH 03/13] tpm: xen-tpmfront: Fix default durations

On 09/23/2013 02:51 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 23, 2013 at 12:14:33PM -0600, Jason Gunthorpe wrote:
>> All the default durations were being set to 10 minutes which is
>> way too long for the timeouts. Normal values for the longest
>> duration are around 5 mins, and short duration ar around .5s.
>>
>> Further, these are just the default, tpm_get_timeouts will set
>> them to values from the TPM (or throw an error).
>>
>> Just remove them.
>
> Sounds sensible.
>
> Daniel?

Also sounds good to me. I believe this was just a remnant of the old
xen-tpmfront code that didn't get removed once the timeouts were being
read from the backend.

>>
>> Cc: Daniel De Graaf <[email protected]>
>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>> Signed-off-by: Jason Gunthorpe <[email protected]>
>> ---
>> drivers/char/tpm/xen-tpmfront.c | 7 -------
>> 1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
>> index 7a7929b..6f2fe2b 100644
>> --- a/drivers/char/tpm/xen-tpmfront.c
>> +++ b/drivers/char/tpm/xen-tpmfront.c
>> @@ -210,8 +210,6 @@ static struct attribute_group vtpm_attr_grp = {
>> .attrs = vtpm_attrs,
>> };
>>
>> -#define TPM_LONG_TIMEOUT (10 * 60 * HZ)
>> -
>> static const struct tpm_vendor_specific tpm_vtpm = {
>> .status = vtpm_status,
>> .recv = vtpm_recv,
>> @@ -224,11 +222,6 @@ static const struct tpm_vendor_specific tpm_vtpm = {
>> .miscdev = {
>> .fops = &vtpm_ops,
>> },
>> - .duration = {
>> - TPM_LONG_TIMEOUT,
>> - TPM_LONG_TIMEOUT,
>> - TPM_LONG_TIMEOUT,
>> - },
>> };
>>
>> static irqreturn_t tpmif_interrupt(int dummy, void *dev_id)
>> --
>> 1.8.1.2
>>
>
>


--
Daniel De Graaf
National Security Agency

2013-09-23 19:36:44

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Mon, Sep 23, 2013 at 02:54:21PM -0400, Daniel De Graaf wrote:
> On 09/23/2013 02:14 PM, Jason Gunthorpe wrote:
> >CLASS-sysfs.c is a common idiom for linux subsystems.
> >
> >This pulls all the sysfs attribute functions and related code
> >into tpm-sysfs.c. To support this change some constants are moved
> >from tpm.c to tpm.h and __tpm_pcr_read is made non-static and is
> >called tpm_pcr_read_dev.
> >
> >Signed-off-by: Jason Gunthorpe <[email protected]>
> [...]
> >diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
> >index 12a4ab2..7892557 100644
> >+++ b/drivers/char/tpm/xen-tpmfront.c
> [...]
> >-static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
> >-static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
> >-static DEVICE_ATTR(locality, S_IRUGO | S_IWUSR, tpm_show_locality,
> >- tpm_store_locality);
>
> This patch drops the "locality" sysfs attribute from xen-tpmfront. Since
> that attribute is currently only implemented for the xen TPM driver, it
> is best to leave it there for now (and its show/store functions could
> also be made static, an oversight I just noticed now). If this attribute
> is later made available on other TPM drivers, it may need to contain
> device-specific logic, but such an implementation is well outside the
> scope of this series.

Okay, I see what you are talking about, the compiler didn't warn
because of the missing static.

This really is a core functionality. Lots of other drivers support
locality, but none have dared actually expose the functionality. IHMO,
it is a mistake to just jam a locality attribute in one driver.

Sorry, I would have said something when the driver was posted if it
was obvious this was hiding in there :|

It looks like this driver was introduced in the 3.12 merge window, we
could drop the attribute, and try to merge a core supported locality
API in 3.13? What do you think?

But, if you say it is needed, it is easy enough to adjust this patch
series.

Thanks,
Jason

2013-09-23 20:21:41

by Daniel De Graaf

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On 09/23/2013 03:36 PM, Jason Gunthorpe wrote:
> On Mon, Sep 23, 2013 at 02:54:21PM -0400, Daniel De Graaf wrote:
>> On 09/23/2013 02:14 PM, Jason Gunthorpe wrote:
>>> CLASS-sysfs.c is a common idiom for linux subsystems.
>>>
>>> This pulls all the sysfs attribute functions and related code
>>> into tpm-sysfs.c. To support this change some constants are moved
>> >from tpm.c to tpm.h and __tpm_pcr_read is made non-static and is
>>> called tpm_pcr_read_dev.
>>>
>>> Signed-off-by: Jason Gunthorpe <[email protected]>
>> [...]
>>> diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c
>>> index 12a4ab2..7892557 100644
>>> +++ b/drivers/char/tpm/xen-tpmfront.c
>> [...]
>>> -static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
>>> -static DEVICE_ATTR(timeouts, S_IRUGO, tpm_show_timeouts, NULL);
>>> -static DEVICE_ATTR(locality, S_IRUGO | S_IWUSR, tpm_show_locality,
>>> - tpm_store_locality);
>>
>> This patch drops the "locality" sysfs attribute from xen-tpmfront. Since
>> that attribute is currently only implemented for the xen TPM driver, it
>> is best to leave it there for now (and its show/store functions could
>> also be made static, an oversight I just noticed now). If this attribute
>> is later made available on other TPM drivers, it may need to contain
>> device-specific logic, but such an implementation is well outside the
>> scope of this series.
>
> Okay, I see what you are talking about, the compiler didn't warn
> because of the missing static.
>
> This really is a core functionality. Lots of other drivers support
> locality, but none have dared actually expose the functionality. IHMO,
> it is a mistake to just jam a locality attribute in one driver.
>
> Sorry, I would have said something when the driver was posted if it
> was obvious this was hiding in there :|

That's fine; it wasn't really advertised in the description, and was
mostly added in order to demonstrate the locality security label binding
in Xen's vtpm-stubdom.

> It looks like this driver was introduced in the 3.12 merge window, we
> could drop the attribute, and try to merge a core supported locality
> API in 3.13? What do you think?
>
> But, if you say it is needed, it is easy enough to adjust this patch
> series.
>
> Thanks,
> Jason

If it's replaced with an alternative, I don't think the sysfs attribute
will need to remain. I am not aware of any clients that currently use
this attribute. The sysfs attribute could remain as the common interface
for changing locality, unless you think an ioctl on /dev/tpm0 or
something else would be preferable (the attribute was just the simplest
way to implement locality switching at the time).

Do you already have an idea on what the core-supported API's interface
would be? Making the current locality a part of the TPM device state
would suffice for TIS and xen-tpmfront with minimal changes, but I
haven't checked the other drivers.

--
Daniel De Graaf
National Security Agency

2013-09-23 20:42:43

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Mon, Sep 23, 2013 at 04:20:51PM -0400, Daniel De Graaf wrote:

> That's fine; it wasn't really advertised in the description, and was
> mostly added in order to demonstrate the locality security label binding
> in Xen's vtpm-stubdom.

Ok, lets take it out for now then? I'll send a patch.

> >It looks like this driver was introduced in the 3.12 merge window, we
> >could drop the attribute, and try to merge a core supported locality
> >API in 3.13? What do you think?
> >
> >But, if you say it is needed, it is easy enough to adjust this patch
> >series.

> If it's replaced with an alternative, I don't think the sysfs attribute
> will need to remain. I am not aware of any clients that currently use
> this attribute. The sysfs attribute could remain as the common interface
> for changing locality, unless you think an ioctl on /dev/tpm0 or
> something else would be preferable (the attribute was just the simplest
> way to implement locality switching at the time).

Off the very top of my head:

I suspect that a good API would be a sysfs attribute
'default_locality' and an IOCTL to change localities. The
default_locality would only take effect when the /dev/tpmX is opened,
so fiddling with sysfs doesn't break active users.

The struct ops I've added would have a function to change localities,
some of the generic TPM functions should be revised to have a locality
argument.

Some thought is needed to determine what locality in-kernel users
should be using. I suspect userspace and kernel space should not be
forced to the same locality.

Should user space be restricted to a subset of localities?

What use models do you see with the security label binding mechanism
you have on the hypervisor side?

Jason

2013-09-23 21:27:15

by Joel Schopp

Subject: Re: [tpmdd-devel] [PATCH 00/13] TPM cleanup

> Jason Gunthorpe (13):
> tpm: ibmvtpm: Use %zd formatting for size_t format arguments
> tpm atmel: Call request_region with the correct base
> tpm: xen-tpmfront: Fix default durations
> tpm: Store devname in the tpm_chip
> tpm: Use container_of to locate the tpm_chip in tpm_open
> tpm: Remove redundant dev_set_drvdata
> tpm: Remove tpm_show_caps_1_2
> tpm: Pull everything related to /dev/tpmX into tpm-dev.c
> tpm: Pull everything related to sysfs into tpm-sysfs.c
> tpm: Create a tpm_class_ops structure and use it in the drivers
> tpm: Use the ops structure instead of a copy in tpm_vendor_specific
> tpm: st33: Remove chip->data_buffer access from this driver
> tpm: Make tpm-dev allocate a per-file structure
>
> drivers/char/tpm/Makefile | 2 +-
> drivers/char/tpm/tpm-dev.c | 213 +++++++++++++++
> drivers/char/tpm/tpm-sysfs.c | 318 ++++++++++++++++++++++
> drivers/char/tpm/tpm.c | 524 +++---------------------------------
> drivers/char/tpm/tpm.h | 86 +++---
> drivers/char/tpm/tpm_atmel.c | 30 +--
> drivers/char/tpm/tpm_i2c_atmel.c | 42 +--
> drivers/char/tpm/tpm_i2c_infineon.c | 44 +--
> drivers/char/tpm/tpm_i2c_nuvoton.c | 42 +--
> drivers/char/tpm/tpm_i2c_stm_st33.c | 51 +---
> drivers/char/tpm/tpm_ibmvtpm.c | 44 +--
> drivers/char/tpm/tpm_infineon.c | 28 +-
> drivers/char/tpm/tpm_nsc.c | 28 +-
> drivers/char/tpm/tpm_spi_stm_st33.c | 50 +---
> drivers/char/tpm/tpm_tis.c | 43 +--
> drivers/char/tpm/xen-tpmfront.c | 57 +---
> include/linux/tpm.h | 15 ++
> 17 files changed, 638 insertions(+), 979 deletions(-)
> create mode 100644 drivers/char/tpm/tpm-dev.c
> create mode 100644 drivers/char/tpm/tpm-sysfs.c
>

For what it's worth I have nothing to say except the cleanups look sane
to me.
Reviewed-by: Joel Schopp <[email protected]>

2013-09-23 22:01:55

by Daniel De Graaf

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On 09/23/2013 04:42 PM, Jason Gunthorpe wrote:
> On Mon, Sep 23, 2013 at 04:20:51PM -0400, Daniel De Graaf wrote:
>
>> That's fine; it wasn't really advertised in the description, and was
>> mostly added in order to demonstrate the locality security label binding
>> in Xen's vtpm-stubdom.
>
> Ok, lets take it out for now then? I'll send a patch.
>
>>> It looks like this driver was introduced in the 3.12 merge window, we
>>> could drop the attribute, and try to merge a core supported locality
>>> API in 3.13? What do you think?
>>>
>>> But, if you say it is needed, it is easy enough to adjust this patch
>>> series.
>
>> If it's replaced with an alternative, I don't think the sysfs attribute
>> will need to remain. I am not aware of any clients that currently use
>> this attribute. The sysfs attribute could remain as the common interface
>> for changing locality, unless you think an ioctl on /dev/tpm0 or
>> something else would be preferable (the attribute was just the simplest
>> way to implement locality switching at the time).
>
> Off the very top of my head:
>
> I suspect that a good API would be a sysfs attribute
> 'default_locality' and an IOCTL to change localities. The
> default_locality would only take effect when the /dev/tpmX is opened,
> so fiddling with sysfs doesn't break active users.
>
> The struct ops I've added would have a function to change localities,
> some of the generic TPM functions should be revised to have a locality
> argument.
>
> Some thought is needed to determine what locality in-kernel users
> should be using. I suspect userspace and kernel space should not be
> forced to the same locality.
>
> Should user space be restricted to a subset of localities?

In a PC client TPM, normal OS code (as opposed to firmware or microcode)
is already restricted to locality 0-2. It may make sense to restrict
locality 2 to the kernel, which would allow an in-kernel TPM seal
command to be able to bind data so that userspace cannot unseal it.
However, only allowing localities 0 and 1 to userspace may be too
restrictive if userspace also wishes to use locality for separation,
since locality 1 does not have the ability to reset any PCRs that
locality 0 cannot also reset.

The kernel could reserve only locality 1 for its own use, giving it the
ability to seal data but not interfering with the ability to reset PCRs.
This would be my preference, although it is less intuitive to allow code
of lower privilege (userspace) to control the higher numbered locality
2.

Perhaps a .config option would be useful to allow the system designer to
choose what, if any, locality to reserve for kernel use?

> What use models do you see with the security label binding mechanism
> you have on the hypervisor side?
>
> Jason
>

Currently, Xen's virtual TPM restricts certain localities to certain
domains as identified by the domain's security label. For example, a
guest VM may only be allowed to use locality 0-2 (as on real hardware),
while the device model domain associated with the guest is allowed
locality 0-4, and a monitoring/introspection domain must use another
vTPM-only locality (perhaps 5; the TCG can define additional localities
for use in vTPMs).

--
Daniel De Graaf
National Security Agency

2013-09-23 22:23:36

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Mon, Sep 23, 2013 at 06:00:46PM -0400, Daniel De Graaf wrote:

> In a PC client TPM, normal OS code (as opposed to firmware or microcode)
> is already restricted to locality 0-2. It may make sense to restrict
> locality 2 to the kernel, which would allow an in-kernel TPM seal
> command to be able to bind data so that userspace cannot unseal it.
> However, only allowing localities 0 and 1 to userspace may be too
> restrictive if userspace also wishes to use locality for separation,
> since locality 1 does not have the ability to reset any PCRs that
> locality 0 cannot also reset.
> The kernel could reserve only locality 1 for its own use, giving it the
> ability to seal data but not interfering with the ability to reset PCRs.
> This would be my preference, although it is less intuitive to allow code
> of lower privilege (userspace) to control the higher numbered locality
> 2.

This matches my vague understanding (we don't use localities here)

>> Perhaps a .config option would be useful to allow the system designer to
>> choose what, if any, locality to reserve for kernel use?

A runtime sysfs seems reasonable..

Would:
user_default_locality
kernel_default_locality
user_allowed_localities (bitmask)
supported_localities (bitmask)
a GET_LOCALITY/SET_LOCALITY IOCTL to change localities of an open'd
/dev/tpmX

Do the job?

At first glance anyhow. I wonder what in-kernel users would be
impacted by localities..

Any thoughts on root vs not-root? Would middelware want to use
localities?

Do you know anyone on the userspace SW side who could look at this?

Jason

2013-09-24 14:29:31

by Daniel De Graaf

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On 09/23/2013 06:23 PM, Jason Gunthorpe wrote:
> On Mon, Sep 23, 2013 at 06:00:46PM -0400, Daniel De Graaf wrote:
>
>> In a PC client TPM, normal OS code (as opposed to firmware or microcode)
>> is already restricted to locality 0-2. It may make sense to restrict
>> locality 2 to the kernel, which would allow an in-kernel TPM seal
>> command to be able to bind data so that userspace cannot unseal it.
>> However, only allowing localities 0 and 1 to userspace may be too
>> restrictive if userspace also wishes to use locality for separation,
>> since locality 1 does not have the ability to reset any PCRs that
>> locality 0 cannot also reset.
>> The kernel could reserve only locality 1 for its own use, giving it the
>> ability to seal data but not interfering with the ability to reset PCRs.
>> This would be my preference, although it is less intuitive to allow code
>> of lower privilege (userspace) to control the higher numbered locality
>> 2.
>
> This matches my vague understanding (we don't use localities here)
>
>>> Perhaps a .config option would be useful to allow the system designer to
>>> choose what, if any, locality to reserve for kernel use?
>
> A runtime sysfs seems reasonable..

Allowing a userspace application to change which locality is kernel- and
userspace-only will eliminate the primary benefit of having a locality
restricted to the kernel. With the kernel-only locality selected at
compile (or possibly kernel command line) time, a reboot with different
measurements would be required for userspace to gain access to the
locality used to seal a secret intended for use by the kernel alone -
and the secret would presumably be sealed to those original
measurements.

> Would:
> user_default_locality
> kernel_default_locality
> user_allowed_localities (bitmask)
> supported_localities (bitmask)
> a GET_LOCALITY/SET_LOCALITY IOCTL to change localities of an open'd
> /dev/tpmX
>
> Do the job?

At least "supported_localities" should be generated by the driver if it
is generated at all. There are a few different proposals for handling
localities over 4 in virtual TPMs; one is that locality numbers between
32-255 would be permitted and 5-31 made non-addressable. While this
would work with a bitmask, I'm not sure that is the best solution.

Perhaps:
default_locality - default to CONFIG_USER_DEFAULT_LOCALITY
sysfs node permissions 0644
kernel_locality - default to #CONFIG_KERNEL_DEFAULT_LOCALITY
0444 if CONFIG_KERNEL_ONLY_LOCALITY=y
0644 if CONFIG_KERNEL_ONLY_LOCALITY=n
ioctl TPM_{GET,SET}_LOCALITY on an open /dev/tpmX

If CONFIG_KERNEL_ONLY_LOCALITY=y, the userspace locality is not
permitted to be equal to kernel_locality (but may take any other valid
value). Drivers may reject locality values that they consider invalid
(the default should be to only allow 0-4, which is all that is defined
in the spec) or may fail on attempted use of the TPM by passing down an
error from the hardware - I would expect the latter to be the case on
attempts to use locality 4 in the tpm_tis driver.

> At first glance anyhow. I wonder what in-kernel users would be
> impacted by localities..

The only one I see immediately is seal/unseal (security/keys/trusted.c).
The invocation of the seal command would need to be changed to seal the
trusted keys to the kernel-only locality in order to take advantage of
the increased protection provided by a kernel-only locality.

IMA could potentially be impacted by the locality selection if it were
configured to use a locality-restricted PCR; however, the default (10) is
not restricted and there is generally no need to use a locality-restricted
PCR for this.

> Any thoughts on root vs not-root? Would middelware want to use
> localities?

I think permissions on the /dev/tpmX node suffices for this distinction.
The TCS daemon would need to be trusted to separate multiple user-space
localities since it will be keeping /dev/tpmX open anyway.

> Do you know anyone on the userspace SW side who could look at this?
>
> Jason

I should be able to find someone.

--
Daniel De Graaf
National Security Agency

2013-09-30 18:10:16

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Tue, Sep 24, 2013 at 10:28:41AM -0400, Daniel De Graaf wrote:
> On 09/23/2013 06:23 PM, Jason Gunthorpe wrote:
> >On Mon, Sep 23, 2013 at 06:00:46PM -0400, Daniel De Graaf wrote:
> >
> >>In a PC client TPM, normal OS code (as opposed to firmware or microcode)
> >>is already restricted to locality 0-2. It may make sense to restrict
> >>locality 2 to the kernel, which would allow an in-kernel TPM seal
> >>command to be able to bind data so that userspace cannot unseal it.
> >>However, only allowing localities 0 and 1 to userspace may be too
> >>restrictive if userspace also wishes to use locality for separation,
> >>since locality 1 does not have the ability to reset any PCRs that
> >>locality 0 cannot also reset.
> >>The kernel could reserve only locality 1 for its own use, giving it the
> >>ability to seal data but not interfering with the ability to reset PCRs.
> >>This would be my preference, although it is less intuitive to allow code
> >>of lower privilege (userspace) to control the higher numbered locality
> >>2.
> >
> >This matches my vague understanding (we don't use localities here)
> >
> >>>Perhaps a .config option would be useful to allow the system designer to
> >>>choose what, if any, locality to reserve for kernel use?
> >
> >A runtime sysfs seems reasonable..
>
> Allowing a userspace application to change which locality is kernel- and
> userspace-only will eliminate the primary benefit of having a locality
> restricted to the kernel.

Right, I was sort of thinking these sysfs files would be write-once or
trapped doored to prevent going backwards (like how secure level
worked). Ideally the write would be in the initramfs, which is part of
the PCR computation, so it should have the same properties as CONFIG_?

I think using CONFIG_ options would make this feature unavaiable to
distro kernel users...

> At least "supported_localities" should be generated by the driver if it
> is generated at all. There are a few different proposals for handling
> localities over 4 in virtual TPMs; one is that locality numbers between
> 32-255 would be permitted and 5-31 made non-addressable. While this
> would work with a bitmask, I'm not sure that is the best solution.

Hmm, a 256 bit wide mask isn't impossible, not sure what other
options are good. Do you think userspace needs to know what localities
are valid or is an ioctl scan sufficient?

> Perhaps:
> default_locality - default to CONFIG_USER_DEFAULT_LOCALITY
> sysfs node permissions 0644
> kernel_locality - default to #CONFIG_KERNEL_DEFAULT_LOCALITY
> 0444 if CONFIG_KERNEL_ONLY_LOCALITY=y
> 0644 if CONFIG_KERNEL_ONLY_LOCALITY=n
> ioctl TPM_{GET,SET}_LOCALITY on an open /dev/tpmX
>
> If CONFIG_KERNEL_ONLY_LOCALITY=y, the userspace locality is not
> permitted to be equal to kernel_locality (but may take any other valid
> value). Drivers may reject locality values that they consider invalid
> (the default should be to only allow 0-4, which is all that is defined
> in the spec) or may fail on attempted use of the TPM by passing down an
> error from the hardware - I would expect the latter to be the case on
> attempts to use locality 4 in the tpm_tis driver.

Seems resonable, CONFIG_KERNEL_ONLY_LOCALITY could be
CONFIG_TPM_ONE_TIME_LOCALITY (eg you get to set kernel_locality only
once)

> The only one I see immediately is seal/unseal (security/keys/trusted.c).
> The invocation of the seal command would need to be changed to seal the
> trusted keys to the kernel-only locality in order to take advantage of
> the increased protection provided by a kernel-only locality.

Right

> >Do you know anyone on the userspace SW side who could look at this?

> I should be able to find someone.

Okay, let me know. I'd like to get a few more clean ups done before
mucking with the sysfs, but the way forward for locality looks pretty
clear..

Thanks,
Jason

2013-09-30 20:37:45

by Daniel De Graaf

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On 09/30/2013 02:10 PM, Jason Gunthorpe wrote:
> On Tue, Sep 24, 2013 at 10:28:41AM -0400, Daniel De Graaf wrote:
>> On 09/23/2013 06:23 PM, Jason Gunthorpe wrote:
>>> On Mon, Sep 23, 2013 at 06:00:46PM -0400, Daniel De Graaf wrote:
>>>
>>>> In a PC client TPM, normal OS code (as opposed to firmware or microcode)
>>>> is already restricted to locality 0-2. It may make sense to restrict
>>>> locality 2 to the kernel, which would allow an in-kernel TPM seal
>>>> command to be able to bind data so that userspace cannot unseal it.
>>>> However, only allowing localities 0 and 1 to userspace may be too
>>>> restrictive if userspace also wishes to use locality for separation,
>>>> since locality 1 does not have the ability to reset any PCRs that
>>>> locality 0 cannot also reset.
>>>> The kernel could reserve only locality 1 for its own use, giving it the
>>>> ability to seal data but not interfering with the ability to reset PCRs.
>>>> This would be my preference, although it is less intuitive to allow code
>>>> of lower privilege (userspace) to control the higher numbered locality
>>>> 2.
>>>
>>> This matches my vague understanding (we don't use localities here)
>>>
>>>>> Perhaps a .config option would be useful to allow the system designer to
>>>>> choose what, if any, locality to reserve for kernel use?
>>>
>>> A runtime sysfs seems reasonable..
>>
>> Allowing a userspace application to change which locality is kernel- and
>> userspace-only will eliminate the primary benefit of having a locality
>> restricted to the kernel.
>
> Right, I was sort of thinking these sysfs files would be write-once or
> trapped doored to prevent going backwards (like how secure level
> worked). Ideally the write would be in the initramfs, which is part of
> the PCR computation, so it should have the same properties as CONFIG_?
>
> I think using CONFIG_ options would make this feature unavaiable to
> distro kernel users...

This just moves the problem - now you need a custom initrd instead of
a custom kernel. Other TPM options like IMA's PCR selection also must
be changed at CONFIG_ time, although that seems to be more justified
since IMA in TCB mode is not usable on any distro kernel that makes
the TPM driver a module (i.e. most or all of them).

>> At least "supported_localities" should be generated by the driver if it
>> is generated at all. There are a few different proposals for handling
>> localities over 4 in virtual TPMs; one is that locality numbers between
>> 32-255 would be permitted and 5-31 made non-addressable. While this
>> would work with a bitmask, I'm not sure that is the best solution.
>
> Hmm, a 256 bit wide mask isn't impossible, not sure what other
> options are good. Do you think userspace needs to know what localities
> are valid or is an ioctl scan sufficient?

I don't think userspace needs to know this directly. The system designer
will need know in order to create any kind of policy about what locality
is being used (even if that's just setting the default), but they would
also need to be aware of the hardware or virtual environment and could
always enumerate them manually as a last resort.

There is also the fact that the driver may not be able to tell if a
locality is available without doing some kind of test command. The Xen
TPM interface doesn't expose what localities are available, for example,
and the TIS interface may need to test to see if locality 3 and 4 are
actually blocked by the chipset - at least 3 might be available on some
systems (the spec leaves this "implementation dependent").

>> Perhaps:
>> default_locality - default to CONFIG_USER_DEFAULT_LOCALITY
>> sysfs node permissions 0644
>> kernel_locality - default to #CONFIG_KERNEL_DEFAULT_LOCALITY
>> 0444 if CONFIG_KERNEL_ONLY_LOCALITY=y
>> 0644 if CONFIG_KERNEL_ONLY_LOCALITY=n
>> ioctl TPM_{GET,SET}_LOCALITY on an open /dev/tpmX
>>
>> If CONFIG_KERNEL_ONLY_LOCALITY=y, the userspace locality is not
>> permitted to be equal to kernel_locality (but may take any other valid
>> value). Drivers may reject locality values that they consider invalid
>> (the default should be to only allow 0-4, which is all that is defined
>> in the spec) or may fail on attempted use of the TPM by passing down an
>> error from the hardware - I would expect the latter to be the case on
>> attempts to use locality 4 in the tpm_tis driver.
>
> Seems resonable, CONFIG_KERNEL_ONLY_LOCALITY could be
> CONFIG_TPM_ONE_TIME_LOCALITY (eg you get to set kernel_locality only
> once)

Hmm, how much trouble would it be to make this a menu selection? Even
with the one-time-set option, you still need a default set either in
the code or by CONFIG_ so that the TPM is not unavailable before the
sysfs write. The options would be:

- CONFIG_TPM_KERNEL_DEFAULT_LOCALITY = [int]
- CONFIG_TPM_KERNEL_LOCALITY_FIXED - no changes from userspace
- CONFIG_TPM_KERNEL_LOCALITY_ONESHOT - only one change possible
- CONFIG_TPM_KERNEL_LOCALITY_ANY - may be changed freely

The userspace locality is not allowed to use the kernel locality if
the mode is either FIXED or ONESHOT, but may share locality if ANY
is used.

Or, for more flexibility (I actually like this one better):

- CONFIG_TPM_KERNEL_DEFAULT_LOCALITY = [int]
- CONFIG_TPM_KERNEL_LOCALITY_FIXED = [bool]

And sysfs contains:
- kernel_locality [0644, int; 0444 if FIXED=y or when locked(?)]
- lock_kernel_locality [write-once; only exists if FIXED=n]

Where kernel_locality may be changed until a write is made to
local_kernel_locality, at which time the value of kernel_locality
becomes read-only and no longer available via /dev/tpmX.

>> The only one I see immediately is seal/unseal (security/keys/trusted.c).
>> The invocation of the seal command would need to be changed to seal the
>> trusted keys to the kernel-only locality in order to take advantage of
>> the increased protection provided by a kernel-only locality.
>
> Right

Actually, only the invocation needs to be changed - the PCR selection
is passed in from userspace, which will just need to use PCR_INFO_LONG
with the proper locality mask.

>>> Do you know anyone on the userspace SW side who could look at this?
>
>> I should be able to find someone.
>
> Okay, let me know. I'd like to get a few more clean ups done before
> mucking with the sysfs, but the way forward for locality looks pretty
> clear..
>
> Thanks,
> Jason

So far, nobody I have talked to has offered any strong opinions on
what locality should be used or how it should be set. I think finding
a developer of trousers may be the most useful to talk about how the
ioctl portion of this would need to be set up - if someone is actually
needed.

--
Daniel De Graaf
National Security Agency

2013-09-30 21:20:13

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Mon, Sep 30, 2013 at 04:36:27PM -0400, Daniel De Graaf wrote:

> >I think using CONFIG_ options would make this feature unavaiable to
> >distro kernel users...
>
> This just moves the problem - now you need a custom initrd instead of
> a custom kernel. Other TPM options like IMA's PCR selection also must
> be changed at CONFIG_ time, although that seems to be more justified
> since IMA in TCB mode is not usable on any distro kernel that makes
> the TPM driver a module (i.e. most or all of them).

A 'custom' initrd is something a distro can automate. Eg a distro's
initrd generation script could read /etc/tpm.cfg and generate an
initrd with the module load and correct sysfs writes. This is more
accessible than recompiling the kernel.

My comments would apply to IMA as well, it should work with standard
distros, meaning the initrd must be able to set it up. So, load the
module in the initrd, setup localities, select the PCR, then enable
IMA.

The bootloader should measure the kernel and initrd together.

IMHO, distros are not making it easy to enable TPM features, and
requiring a kernel recompile is not helping :)

> There is also the fact that the driver may not be able to tell if a
> locality is available without doing some kind of test command. The
> Xen

Make sense.

> Or, for more flexibility (I actually like this one better):
>
> - CONFIG_TPM_KERNEL_DEFAULT_LOCALITY = [int]
> - CONFIG_TPM_KERNEL_LOCALITY_FIXED = [bool]
>
> And sysfs contains:
> - kernel_locality [0644, int; 0444 if FIXED=y or when locked(?)]
> - lock_kernel_locality [write-once; only exists if FIXED=n]

Yes, this looks simple and sane.

But if there isn't really a need to have a hardwired kernel, the
defaults can be DEFAULT_LOCALITY=0, LOCALITY_FIXED=n and we can
recommend distros rely on the initrd.

> So far, nobody I have talked to has offered any strong opinions on
> what locality should be used or how it should be set. I think finding
> a developer of trousers may be the most useful to talk about how the
> ioctl portion of this would need to be set up - if someone is actually
> needed.

It would be nice to have a user! As I said, we don't use it here.

Jason

2013-09-30 22:10:00

by Joel Schopp

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

<snip>

> There is also the fact that the driver may not be able to tell if a
> locality is available without doing some kind of test command. The Xen
> TPM interface doesn't expose what localities are available, for example,
> and the TIS interface may need to test to see if locality 3 and 4 are
> actually blocked by the chipset - at least 3 might be available on some
> systems (the spec leaves this "implementation dependent").
>
>>> Perhaps:
>>> default_locality - default to CONFIG_USER_DEFAULT_LOCALITY
>>> sysfs node permissions 0644
>>> kernel_locality - default to #CONFIG_KERNEL_DEFAULT_LOCALITY
>>> 0444 if CONFIG_KERNEL_ONLY_LOCALITY=y
>>> 0644 if CONFIG_KERNEL_ONLY_LOCALITY=n
>>> ioctl TPM_{GET,SET}_LOCALITY on an open /dev/tpmX
>>>
>>> If CONFIG_KERNEL_ONLY_LOCALITY=y, the userspace locality is not
>>> permitted to be equal to kernel_locality (but may take any other valid
>>> value). Drivers may reject locality values that they consider invalid
>>> (the default should be to only allow 0-4, which is all that is defined
>>> in the spec) or may fail on attempted use of the TPM by passing down an
>>> error from the hardware - I would expect the latter to be the case on
>>> attempts to use locality 4 in the tpm_tis driver.
>>
>> Seems resonable, CONFIG_KERNEL_ONLY_LOCALITY could be
>> CONFIG_TPM_ONE_TIME_LOCALITY (eg you get to set kernel_locality only
>> once)
>
> Hmm, how much trouble would it be to make this a menu selection? Even
> with the one-time-set option, you still need a default set either in
> the code or by CONFIG_ so that the TPM is not unavailable before the
> sysfs write. The options would be:
>
> - CONFIG_TPM_KERNEL_DEFAULT_LOCALITY = [int]
> - CONFIG_TPM_KERNEL_LOCALITY_FIXED - no changes from userspace
> - CONFIG_TPM_KERNEL_LOCALITY_ONESHOT - only one change possible
> - CONFIG_TPM_KERNEL_LOCALITY_ANY - may be changed freely
>
> The userspace locality is not allowed to use the kernel locality if
> the mode is either FIXED or ONESHOT, but may share locality if ANY
> is used.
>
> Or, for more flexibility (I actually like this one better):
>
> - CONFIG_TPM_KERNEL_DEFAULT_LOCALITY = [int]
> - CONFIG_TPM_KERNEL_LOCALITY_FIXED = [bool]

This seems best of the options discussed to me.

>
> And sysfs contains:
> - kernel_locality [0644, int; 0444 if FIXED=y or when locked(?)]
> - lock_kernel_locality [write-once; only exists if FIXED=n]
>
> Where kernel_locality may be changed until a write is made to
> local_kernel_locality, at which time the value of kernel_locality
> becomes read-only and no longer available via /dev/tpmX.
>
>>> The only one I see immediately is seal/unseal (security/keys/trusted.c).
>>> The invocation of the seal command would need to be changed to seal the
>>> trusted keys to the kernel-only locality in order to take advantage of
>>> the increased protection provided by a kernel-only locality.
>>
>> Right
>
> Actually, only the invocation needs to be changed - the PCR selection
> is passed in from userspace, which will just need to use PCR_INFO_LONG
> with the proper locality mask.
>
>>>> Do you know anyone on the userspace SW side who could look at this?
>>
>>> I should be able to find someone.
>>
>> Okay, let me know. I'd like to get a few more clean ups done before
>> mucking with the sysfs, but the way forward for locality looks pretty
>> clear..
>>
>> Thanks,
>> Jason
>
> So far, nobody I have talked to has offered any strong opinions on
> what locality should be used or how it should be set. I think finding
> a developer of trousers may be the most useful to talk about how the
> ioctl portion of this would need to be set up - if someone is actually
> needed.
>

I am a TrouSerS developer and am ccing Richard, another TrouSerS
developer, and ccing the trousers-tech list. It would be good if you
could elaborate on the question and context for those not following the
entire thread, myself included.

2013-10-01 21:57:17

by Peter Huewe

Subject: Re: [PATCH 01/13] tpm: ibmvtpm: Use %zd formatting for size_t format arguments

Am Montag, 23. September 2013, 20:14:31 schrieb Jason Gunthorpe:
> This suppresses compile warnings on 32 bit builds.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>


Reviewed-by: Peter Huewe <[email protected]>
Signed-off-by: Peter Huewe <[email protected]>

Staged here
https://github.com/PeterHuewe/linux-tpmdd for-james

2013-10-01 22:21:28

by Jason Gunthorpe

Subject: Re: [PATCH 07/13] tpm: Remove tpm_show_caps_1_2

On Wed, Oct 02, 2013 at 12:09:22AM +0200, Peter H?we wrote:

> Since the tpm_spi_stm_st33, tpm_i2c_nuvoton and tpm_i2c_atmel drivers
> are not yet merged and were heavily improved by you anyway, please
> include this improvement directly in the new drivers.

Okay, it is easy enough to invert the patch order and put the drivers
last. I am not submitting the st33 driver, but when things are done I
will send Mathias a patch with the changes from this series that are
needed and he can deal with re-submitting his driver.

FWIW, I needed the drivers first to keep my world sane when testing
this thing, so the patches are all as-tested from my perspective.

Jason

2013-10-01 22:36:55

by Peter Huewe

Subject: Re: [tpmdd-devel] [PATCH 07/13] tpm: Remove tpm_show_caps_1_2

Hi Jason,

Am Mittwoch, 2. Oktober 2013, 00:21:13 schrieb Jason Gunthorpe:
> On Wed, Oct 02, 2013 at 12:09:22AM +0200, Peter H?we wrote:
> > Since the tpm_spi_stm_st33, tpm_i2c_nuvoton and tpm_i2c_atmel drivers
> > are not yet merged and were heavily improved by you anyway, please
> > include this improvement directly in the new drivers.
>
> Okay, it is easy enough to invert the patch order and put the drivers
> last. I am not submitting the st33 driver, but when things are done I
> will send Mathias a patch with the changes from this series that are
> needed and he can deal with re-submitting his driver.
>
> FWIW, I needed the drivers first to keep my world sane when testing
> this thing, so the patches are all as-tested from my perspective.

nevermind and don't worry - the way you submitted the patches is perfectly
fine for me.
You can keep the patches seperate and in case of doubt I can simply squash
them in the end.

Thanks,
Peter

2013-10-01 22:51:01

by Peter Huewe

Subject: Re: [PATCH 08/13] tpm: Pull everything related to /dev/tpmX into tpm-dev.c

Am Montag, 23. September 2013, 20:14:38 schrieb Jason Gunthorpe:
> CLASS-dev.c is a common idiom for Linux subsystems
>
> This pulls all the code related to the miscdev into tpm-dev.c and makes it
> static. The identical file_operation structs in the drivers are purged and
> the tpm common code unconditionally creates the miscdev.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> ---
> drivers/char/tpm/Makefile | 2 +-
> drivers/char/tpm/tpm-dev.c | 199

When compiling the tpm drivers as modules I get
ERROR: "tpm_sysfs_del_device" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "tpm_dev_add_device" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "tpm_dev_del_device" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "tpm_sysfs_add_device" [drivers/char/tpm/tpm.ko] undefined!
ERROR: "tpm_transmit" [drivers/char/tpm/tpm-sysfs.ko] undefined!
ERROR: "tpm_pcr_read_dev" [drivers/char/tpm/tpm-sysfs.ko] undefined!
ERROR: "tpm_getcap" [drivers/char/tpm/tpm-sysfs.ko] undefined!
ERROR: "tpm_transmit" [drivers/char/tpm/tpm-dev.ko] undefined!


I added a suitable patch with the appropriate EXPORT_SYMBOL_GPL declarations
to my testing branch. (also see next message)


Thanks,
Peter

2013-10-01 22:57:48

by Jason Gunthorpe

Subject: Re: [PATCH 08/13] tpm: Pull everything related to /dev/tpmX into tpm-dev.c

On Wed, Oct 02, 2013 at 12:52:40AM +0200, Peter H?we wrote:
> Am Montag, 23. September 2013, 20:14:38 schrieb Jason Gunthorpe:
> > CLASS-dev.c is a common idiom for Linux subsystems
> >
> > This pulls all the code related to the miscdev into tpm-dev.c and makes it
> > static. The identical file_operation structs in the drivers are purged and
> > the tpm common code unconditionally creates the miscdev.
> >
> > Signed-off-by: Jason Gunthorpe <[email protected]>
> > drivers/char/tpm/Makefile | 2 +-
> > drivers/char/tpm/tpm-dev.c | 199
>
> When compiling the tpm drivers as modules I get
> ERROR: "tpm_sysfs_del_device" [drivers/char/tpm/tpm.ko] undefined!
> ERROR: "tpm_dev_add_device" [drivers/char/tpm/tpm.ko] undefined!
> ERROR: "tpm_dev_del_device" [drivers/char/tpm/tpm.ko] undefined!
> ERROR: "tpm_sysfs_add_device" [drivers/char/tpm/tpm.ko] undefined!
> ERROR: "tpm_transmit" [drivers/char/tpm/tpm-sysfs.ko] undefined!
> ERROR: "tpm_pcr_read_dev" [drivers/char/tpm/tpm-sysfs.ko] undefined!
> ERROR: "tpm_getcap" [drivers/char/tpm/tpm-sysfs.ko] undefined!
> ERROR: "tpm_transmit" [drivers/char/tpm/tpm-dev.ko] undefined!

Oh, I am glad you can test modules..

I botched the makefile changes for the new .c files.

I believe it should be like this:

obj-$(CONFIG_TCG_TPM) += tpm-core.o
tpm-core-y := tpm.o tpm-dev.o tpm-sysfs.o

> I added a suitable patch with the appropriate EXPORT_SYMBOL_GPL declarations
> to my testing branch. (also see next message)

EXPORT_SYMBOL_GPL is not correct, these are in-module references, not
cross module references, and I've deliberately not exported them to
prevent drivers from trying to use them inappropriately.

Let me know if you want me to respin things.

Regards,
Jason

2013-10-01 23:12:40

by Peter Huewe

Subject: Re: [PATCH 08/13] tpm: Pull everything related to /dev/tpmX into tpm-dev.c

Am Mittwoch, 2. Oktober 2013, 00:57:33 schrieb Jason Gunthorpe:
> > When compiling the tpm drivers as modules I get
> > ERROR: "tpm_sysfs_del_device" [drivers/char/tpm/tpm.ko] undefined!
> > ERROR: "tpm_dev_add_device" [drivers/char/tpm/tpm.ko] undefined!
> > ERROR: "tpm_dev_del_device" [drivers/char/tpm/tpm.ko] undefined!
> > ERROR: "tpm_sysfs_add_device" [drivers/char/tpm/tpm.ko] undefined!
> > ERROR: "tpm_transmit" [drivers/char/tpm/tpm-sysfs.ko] undefined!
> > ERROR: "tpm_pcr_read_dev" [drivers/char/tpm/tpm-sysfs.ko] undefined!
> > ERROR: "tpm_getcap" [drivers/char/tpm/tpm-sysfs.ko] undefined!
> > ERROR: "tpm_transmit" [drivers/char/tpm/tpm-dev.ko] undefined!
>
> Oh, I am glad you can test modules..
>
> I botched the makefile changes for the new .c files.
>
> I believe it should be like this:
>
> obj-$(CONFIG_TCG_TPM) += tpm-core.o
> tpm-core-y := tpm.o tpm-dev.o tpm-sysfs.o
>
> > I added a suitable patch with the appropriate EXPORT_SYMBOL_GPL
> > declarations to my testing branch. (also see next message)
>
> EXPORT_SYMBOL_GPL is not correct, these are in-module references, not
> cross module references, and I've deliberately not exported them to
> prevent drivers from trying to use them inappropriately.
Of course you're right - I just wanted to get it compile as fast as possible
;)

The makefile patch did fix it.

Staged here
https://github.com/PeterHuewe/linux-tpmdd/tree/testing-and-review
(along with a small fix for the tpm_i2c_atmel driver ;)


Thanks,
Peter

2013-10-01 23:23:42

by Jason Gunthorpe

Subject: Re: [PATCH 08/13] tpm: Pull everything related to /dev/tpmX into tpm-dev.c

On Wed, Oct 02, 2013 at 01:14:18AM +0200, Peter H?we wrote:

> The makefile patch did fix it.

Great, feel free squash into the broken commit, or I can respin
things.

> (along with a small fix for the tpm_i2c_atmel driver ;)

Hum, I wonder why my compilers didn't whine, x86-64 should have
complained I think...

Anyhow, the %zd is the error here, expected_len should ideally be a
u32 as it comes from be32_to_cpu(hdr->length)

Regards,
Jason

2013-10-02 19:36:55

by Ashley Lai

Subject: Re: [tpmdd-devel] [PATCH 01/13] tpm: ibmvtpm: Use %zd formatting for size_t format arguments

On Mon, 2013-09-23 at 12:14 -0600, Jason Gunthorpe wrote:
> This suppresses compile warnings on 32 bit builds.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>

Looks good to me.

Acked-by: Ashley Lai <[email protected]>

Thanks,
--Ashley Lai

2013-10-03 05:05:30

by Jason Gunthorpe

Subject: Re: [PATCH 08/13] tpm: Pull everything related to /dev/tpmX into tpm-dev.c

On Wed, Oct 02, 2013 at 01:14:18AM +0200, Peter H?we wrote:

> > I botched the makefile changes for the new .c files.
> >
> > I believe it should be like this:
> >
> > obj-$(CONFIG_TCG_TPM) += tpm-core.o
> > tpm-core-y := tpm.o tpm-dev.o tpm-sysfs.o
> >
> > > I added a suitable patch with the appropriate EXPORT_SYMBOL_GPL
> > > declarations to my testing branch. (also see next message)
> >
> > EXPORT_SYMBOL_GPL is not correct, these are in-module references, not
> > cross module references, and I've deliberately not exported them to
> > prevent drivers from trying to use them inappropriately.

> Of course you're right - I just wanted to get it compile as fast as
> possible ;)

It looks like my solution changes the tpm module name to tpm-core.o.
This should be noted in the commit log:

In doing so we also change the core tpm module name from 'tpm' to 'tpm-core',
this is because kbuild does not like it if a multi-file module has a .c
file of the same name.

Is that OK? It follows the pattern other systems have in the kernel.

If this is not OK, then I think we have to rename tpm.c to tpm-XX.c to
make the build system work.

I've consolidated all the remarks and changes thus far on my github:
https://github.com/jgunthorpe/linux/commits/tpm-devel
ec94ce9d29298ba75cec83a136fd841f8da9d528

(plus 1 more untested patch to remove the tpm-bios module)

Jason

2013-10-04 15:48:14

by Peter Huewe

Subject: TPM.ko module rename (was tpm: Pull everything related to /dev/tpmX into tpm-dev.c)

Am Donnerstag, 3. Oktober 2013, 07:05:04 schrieb Jason Gunthorpe:
> On Wed, Oct 02, 2013 at 01:14:18AM +0200, Peter H?we wrote:
> > > I botched the makefile changes for the new .c files.
> > >
> > > I believe it should be like this:
> > >
> > > obj-$(CONFIG_TCG_TPM) += tpm-core.o
> > > tpm-core-y := tpm.o tpm-dev.o tpm-sysfs.o
> > >
>
> It looks like my solution changes the tpm module name to tpm-core.o.
> This should be noted in the commit log:
>
> In doing so we also change the core tpm module name from 'tpm' to
> 'tpm-core', this is because kbuild does not like it if a multi-file module
> has a .c file of the same name.
>
> Is that OK? It follows the pattern other systems have in the kernel.


Hmm,
I'm not 100% sure what's the usual procedure for module renames, especially
since this is visible in userspace.

For users the easiest way would be to rename tpm.c to tpm-core.c and leave the
module name as it is.
In git that's just a simple rename operation.

Thanks,
Peter

2013-10-04 15:58:09

by Ashley Lai

Subject: Re: [tpmdd-devel] [PATCH 04/13] tpm: Store devname in the tpm_chip

On Mon, 2013-09-23 at 12:14 -0600, Jason Gunthorpe wrote:
> Just put the memory directly in the chip structure, rather than
> in a 2nd dedicated kmalloc.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>

Acked-by: Ashley Lai <[email protected]>

Thanks,
--Ashley Lai

2013-10-04 16:28:21

by Jason Gunthorpe

Subject: Re: TPM.ko module rename (was tpm: Pull everything related to /dev/tpmX into tpm-dev.c)

On Fri, Oct 04, 2013 at 05:50:01PM +0200, Peter H?we wrote:

> I'm not 100% sure what's the usual procedure for module renames, especially
> since this is visible in userspace.
>
> For users the easiest way would be to rename tpm.c to tpm-core.c and
> leave the module name as it is. In git that's just a simple rename
> operation.

I agree, keeping the old name seems much safer.

I'm thinking of following the pattern seen in RTC. Rename tpm.c to
tpm-interface.c and eventually migrate all the driver model support
items into tpm-class.c.

Does that seem good? I'll respin the patches and test modular rebuilds.

Jason

2013-10-04 16:45:49

by Ashley Lai

Subject: Re: TPM.ko module rename (was tpm: Pull everything related to /dev/tpmX into tpm-dev.c)

On Fri, 4 Oct 2013, Jason Gunthorpe wrote:

> On Fri, Oct 04, 2013 at 05:50:01PM +0200, Peter H?we wrote:
>
>> I'm not 100% sure what's the usual procedure for module renames, especially
>> since this is visible in userspace.
>>
>> For users the easiest way would be to rename tpm.c to tpm-core.c and
>> leave the module name as it is. In git that's just a simple rename
>> operation.
>
> I agree, keeping the old name seems much safer.
>
> I'm thinking of following the pattern seen in RTC. Rename tpm.c to
> tpm-interface.c and eventually migrate all the driver model support
> items into tpm-class.c.
>
> Does that seem good? I'll respin the patches and test modular rebuilds.
>
> Jason
>

Me too, I vote to keep the same module name. Please let me know
when you are done with this patch, I would like to test it out.

Thanks,
--Ashley Lai

2013-10-04 17:08:17

by Jason Gunthorpe

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Mon, Sep 30, 2013 at 05:09:51PM -0500, Joel Schopp wrote:

> > So far, nobody I have talked to has offered any strong opinions on
> > what locality should be used or how it should be set. I think finding
> > a developer of trousers may be the most useful to talk about how the
> > ioctl portion of this would need to be set up - if someone is actually
> > needed.

> I am a TrouSerS developer and am ccing Richard, another TrouSerS
> developer, and ccing the trousers-tech list. It would be good if you
> could elaborate on the question and context for those not following the
> entire thread, myself included.

Two questions:

Is userspace interested in using the TPM Locality feature, and if so
is there any thoughts on what the interface should be?

Is the kernel interested in using the TPM Locality feature? What for?

Jason

2013-10-04 19:17:43

by Stefan Berger

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On 10/04/2013 01:08 PM, Jason Gunthorpe wrote:
> On Mon, Sep 30, 2013 at 05:09:51PM -0500, Joel Schopp wrote:
>
>>> So far, nobody I have talked to has offered any strong opinions on
>>> what locality should be used or how it should be set. I think finding
>>> a developer of trousers may be the most useful to talk about how the
>>> ioctl portion of this would need to be set up - if someone is actually
>>> needed.
>> I am a TrouSerS developer and am ccing Richard, another TrouSerS
>> developer, and ccing the trousers-tech list. It would be good if you
>> could elaborate on the question and context for those not following the
>> entire thread, myself included.
> Two questions:
>
> Is userspace interested in using the TPM Locality feature, and if so
> is there any thoughts on what the interface should be?

In terms of interface it should probably be an ioctl so that whoever
holds the fd to /dev/tpm0 gets to choose the locality.

Locality allows the resetting of certain PCRs. See section 3.7 in

http://www.trustedcomputinggroup.org/files/static_page_files/8E45D739-1A4B-B294-D06274E7047730FD/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf

Locality 4 can only be used by the hardware (section 2.2).

Locality has an influence on the following TPM commands:
TSC_ResetEstablishmentBit, Seal, Sealx, CreateWrapKey, UnSeal,
GetPubKey, CMK_CreateKey, SHA1CompleteExtend, CertifyKey, Extend,
PCR_Reset, NV_ReadValue, NV_WriteValue, and others. Some of the
commands allow operations to succeed if a previously selected locality
is also currently the chosen one. (If you have control over choosing the
locality, at least that part won't prevent you from succeeding..)

http://www.trustedcomputinggroup.org/files/static_page_files/72C33D71-1A4B-B294-D02C7DF86630BE7C/TPM%20Main-Part%203%20Commands_v1.2_rev116_01032011.pdf

The worst would probably be if an application was to reset a PCR while
another one is using that PCR or just for malicious purposes. Not
providing support for choosing locality would mean that applications
could still use PCRs 16 and 23 for their own purposes and can compete
for their exclusive usage while being able to reset only those two.

Are there use case for resetting PCRs from user space? If not I'd not
support choice for locality from user space.

Stefan

>
> Is the kernel interested in using the TPM Locality feature? What for?
>
> Jason
>
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk
> _______________________________________________
> tpmdd-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>

2013-10-04 22:00:34

by Peter Huewe

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

Am Freitag, 4. Oktober 2013, 21:17:36 schrieb Stefan Berger:
> On 10/04/2013 01:08 PM, Jason Gunthorpe wrote:
> > On Mon, Sep 30, 2013 at 05:09:51PM -0500, Joel Schopp wrote:
> >>> So far, nobody I have talked to has offered any strong opinions on
> >>> what locality should be used or how it should be set. I think finding
> >>> a developer of trousers may be the most useful to talk about how the
> >>> ioctl portion of this would need to be set up - if someone is actually
> >>> needed.
> >>
> >> I am a TrouSerS developer and am ccing Richard, another TrouSerS
> >> developer, and ccing the trousers-tech list. It would be good if you
> >> could elaborate on the question and context for those not following the
> >> entire thread, myself included.
> >
> > Two questions:
> >
> > Is userspace interested in using the TPM Locality feature, and if so
> > is there any thoughts on what the interface should be?
>
> In terms of interface it should probably be an ioctl so that whoever
> holds the fd to /dev/tpm0 gets to choose the locality.
>
> Locality allows the resetting of certain PCRs. See section 3.7 in
>
> http://www.trustedcomputinggroup.org/files/static_page_files/8E45D739-1A4B-> B294-D06274E7047730FD/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_032
> 12013.pdf
>
> Locality 4 can only be used by the hardware (section 2.2).


Afaik Locality 3 (and sometimes 2) is often also "locked down"/filtered after
the bios phase.


>From
http://www.intel.com/content/dam/www/public/us/en/documents/guides/intel-txt-software-development-guide.pdf

"The storage spaces accessible within a TPM device are grouped by a locality
attribute and are a separate set of address ranges from the Intel TXT Public
and Private spaces.
The following localities are defined:
Locality 0 : Non trusted and legacy TPM operation
Locality 1 : An environment for use by the Trusted Operating System
Locality 2 : Trusted OS
Locality 3 : Authenticated Code Module
Locality 4 : Intel TXT hardware use only"

(I know that's "only" Intel's view and not a TCG spec)

Thanks,
Peter

2013-10-05 01:48:08

by Ashley Lai

Subject: Re: [tpmdd-devel] [PATCH 05/13] tpm: Use container_of to locate the tpm_chip in tpm_open

On Mon, 2013-09-23 at 12:14 -0600, Jason Gunthorpe wrote:
> misc_open sets the file->private_date to the misc_dev when calling
> open. We can use container_of to go from the misc_dev back to the
> tpm_chip.
>
> Future clean ups will move tpm_open into a new file and this change
> means we do not have to export the tpm_chip list.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>

Acked-by: Ashley Lai <[email protected]>

Thanks,
--Ashley Lai

2013-10-05 02:14:19

by Ashley Lai

Subject: Re: [tpmdd-devel] [PATCH 06/13] tpm: Remove redundant dev_set_drvdata

On Mon, 2013-09-23 at 12:14 -0600, Jason Gunthorpe wrote:
> TPM drivers should not call dev_set_drvdata (or aliases), only the core
> code is allowed to call dev_set_drvdata, and it does it during
> tpm_register_hardware.
>
> These extra sets are harmless, but are an anti-pattern that many drivers
> have copied.
>
> Signed-off-by: Jason Gunthorpe <[email protected]>

> diff --git a/drivers/char/tpm/tpm_spi_stm_st33.c b/drivers/char/tpm/tpm_spi_stm_st33.c
> index 8d3e8e2..f5e3cd6 100644
> --- a/drivers/char/tpm/tpm_spi_stm_st33.c
> +++ b/drivers/char/tpm/tpm_spi_stm_st33.c
> @@ -779,7 +779,6 @@ tpm_st33_spi_probe(struct spi_device *dev)
> tpm_get_timeouts(chip);
>
> /* attach chip datas to client */
Looks good except this comment needs to be removed. Since this driver
is not merged, please remove this line in the new driver.

> - spi_set_drvdata(dev, chip);
> platform_data->bchipf = false;
>

Acked-by: Ashley Lai <[email protected]>

Thanks,
--Ashley Lai

2013-10-07 15:07:08

by Daniel De Graaf

Subject: Re: [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On 10/04/2013 03:17 PM, Stefan Berger wrote:
> On 10/04/2013 01:08 PM, Jason Gunthorpe wrote:
>> On Mon, Sep 30, 2013 at 05:09:51PM -0500, Joel Schopp wrote:
>>
>>>> So far, nobody I have talked to has offered any strong opinions on
>>>> what locality should be used or how it should be set. I think finding
>>>> a developer of trousers may be the most useful to talk about how the
>>>> ioctl portion of this would need to be set up - if someone is actually
>>>> needed.
>>> I am a TrouSerS developer and am ccing Richard, another TrouSerS
>>> developer, and ccing the trousers-tech list. It would be good if you
>>> could elaborate on the question and context for those not following the
>>> entire thread, myself included.
>> Two questions:
>>
>> Is userspace interested in using the TPM Locality feature, and if so
>> is there any thoughts on what the interface should be?
>
> In terms of interface it should probably be an ioctl so that whoever
> holds the fd to /dev/tpm0 gets to choose the locality.
>
> Locality allows the resetting of certain PCRs. See section 3.7 in
>
> http://www.trustedcomputinggroup.org/files/static_page_files/8E45D739-1A4B-B294-D06274E7047730FD/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
>
> Locality 4 can only be used by the hardware (section 2.2).
>
> Locality has an influence on the following TPM commands:
> TSC_ResetEstablishmentBit, Seal, Sealx, CreateWrapKey, UnSeal,
> GetPubKey, CMK_CreateKey, SHA1CompleteExtend, CertifyKey, Extend,
> PCR_Reset, NV_ReadValue, NV_WriteValue, and others. Some of the
> commands allow operations to succeed if a previously selected locality
> is also currently the chosen one. (If you have control over choosing the
> locality, at least that part won't prevent you from succeeding..)
>
> http://www.trustedcomputinggroup.org/files/static_page_files/72C33D71-1A4B-B294-D02C7DF86630BE7C/TPM%20Main-Part%203%20Commands_v1.2_rev116_01032011.pdf
>
> The worst would probably be if an application was to reset a PCR while
> another one is using that PCR or just for malicious purposes. Not
> providing support for choosing locality would mean that applications
> could still use PCRs 16 and 23 for their own purposes and can compete
> for their exclusive usage while being able to reset only those two.

Any important measurements that would be impacted by a userspace-generated
reset should be directed to the non-resettable PCR 0-15 or 17-19 (allowing
DRTM to reset them). Ideally the /dev/tpm0 device would be opened only by
tcsd (which could be measured by IMA and enforced by an LSM), which could
implement its own restrictions on what clients can extend or reset PCRs.

> Are there use case for resetting PCRs from user space? If not I'd not
> support choice for locality from user space.
>
> Stefan

It's not just resetting - extends to PCR 17-22 are also restricted based on
locality, so without this interface they have been read-only.

There are use cases that are significantly simplified by using multiple
resettable PCRs. One (slightly contrived) example: tcsd manages one PCR so
that it holds the hash of the executable making a local IPC request, another
holds the arguments or pathname, another the IMA signature key used to sign
the application. This allows the application (say, a keyring) to seal data
that only it can unseal; without multiple PCRs, a more complex interface
would be required to support a choice between sealing to the more flexible
IMA signature in order to support software upgrades and sealing to the more
secure exact binary image.

--
Daniel De Graaf
National Security Agency

2013-10-08 09:39:33

by Fuchs, Andreas

Subject: AW: [TrouSerS-tech] [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

Some thoughts on those two questions:

1. Yes, userspace could be interested in setting TPM Localities specifically
for uses of PCR_Reset. For example a Browser could be interested in scheduling
Tabs in a PCR. For this it would reset the PCR and replay the old Extends when
switching a tab. Then the Tab could continue Extending on those pcrs.
Use cases may include any user-application that schedules children's tpm-access
via PCR_Reset...
The problem is, that whilst one process may be allowed to do so, another one may not.

2. This brings us to the problem of differentiating processes' access-rights
on the locality-feature and more specifically how to move this through the tcsd (as
another layer of abstraction). From a tpmdd perspective, if you provide localities,
you will not want to allow for everyone to just randomly set them. They actually
correspond to "capabilities" or access-rights on the TPM...

Random Proposal for discussion:
Rather than an ioctl, why not provide a different tpm-device per locality. This way, the
access to the different localities can be restricted via standard user/group of the device.
i.e. /dev/tpm0l1, /dev/tpm0l2, ... or similar approaches...

A privileged application may access /dev/tpm0l2 whilst another one only gets to l1...

Just some random thoughts, not well thought through though... ;-)

Cheers,
Andreas

________________________________________
Von: Jason Gunthorpe [[email protected]]
Gesendet: Freitag, 4. Oktober 2013 19:08
An: Joel Schopp
Cc: Leonidas Da Silva Barbosa; [email protected]; Rajiv Andrade; [email protected]; Richard Maciel Costa; [email protected]; Daniel De Graaf; Sirrix AG
Betreff: Re: [TrouSerS-tech] [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Mon, Sep 30, 2013 at 05:09:51PM -0500, Joel Schopp wrote:

> > So far, nobody I have talked to has offered any strong opinions on
> > what locality should be used or how it should be set. I think finding
> > a developer of trousers may be the most useful to talk about how the
> > ioctl portion of this would need to be set up - if someone is actually
> > needed.

> I am a TrouSerS developer and am ccing Richard, another TrouSerS
> developer, and ccing the trousers-tech list. It would be good if you
> could elaborate on the question and context for those not following the
> entire thread, myself included.

Two questions:

Is userspace interested in using the TPM Locality feature, and if so
is there any thoughts on what the interface should be?

Is the kernel interested in using the TPM Locality feature? What for?

Jason

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk
_______________________________________________
TrouSerS-tech mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/trousers-tech

2013-10-09 17:39:03

by Jason Gunthorpe

Subject: Re: [TrouSerS-tech] [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Tue, Oct 08, 2013 at 09:15:55AM +0000, Fuchs, Andreas wrote:


> Rather than an ioctl, why not provide a different tpm-device per
> locality. This way, the access to the different localities can be
> restricted via standard user/group of the device. i.e. /dev/tpm0l1,
> /dev/tpm0l2, ... or similar approaches...

The way the TPM architecture is setup you will need the middle ware to
do the switching between localities and managing cross locality
resources, so it doesn't make sense for the kernel to export a
locality specific char device.

You'd have to do the above by having trousers create unix domain
sockets for each of the privilege levels and using the usual security
mechanisms to protect access to those sockets.

Jason

2013-10-10 07:43:55

by Fuchs, Andreas

Subject: AW: [TrouSerS-tech] [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

Hi Jason,

the Unix-Domain-Socket or DBus has actually been on my agenda for quite
some time now, as soon as I find some time or MScCS student for it.
If you know any... ;-)

In any case, I like your idea to split trousers IPC to two distinct unix sockets
for localities. In this case, we could also split tcsd into two processes
along with it for accessing the distinct char-devices and thereby make it
more robust against bugs for "locality-escalation".

Also remember that many people have developed alternative stacks
that don't use trousers but operate directly on the char-device.
They would also benefit from char-device access control for localities.

Even with only a single trousers, I see no harm in two devices. For
backwards compatibility, the current /dev/tpm0 could be exported (with
highest level access control) along with tpm0l1, tpm0l2, ... and/or
trousers could open both char-devices if it wanted to.


One more usecase for localities inside the kernel:

The kernel may want to use localityAtRelease OS in order to protect sealed
data (trusted keyrings) such that user-space could not even unseal
those if they got the blob and the auth-secret; user-space malware.
Without cap_compromise_kernel not even root could gain access in those cases...
(I honestly don't know, if current trusted keyring incorporate localityAtRelease
already, so never mind if I am just proposing implemented features)

Cheers,
Andreas





________________________________________
Von: Jason Gunthorpe [[email protected]]
Gesendet: Mittwoch, 9. Oktober 2013 19:38

On Tue, Oct 08, 2013 at 09:15:55AM +0000, Fuchs, Andreas wrote:

> Rather than an ioctl, why not provide a different tpm-device per
> locality. This way, the access to the different localities can be
> restricted via standard user/group of the device. i.e. /dev/tpm0l1,
> /dev/tpm0l2, ... or similar approaches...

The way the TPM architecture is setup you will need the middle ware to
do the switching between localities and managing cross locality
resources, so it doesn't make sense for the kernel to export a
locality specific char device.

You'd have to do the above by having trousers create unix domain
sockets for each of the privilege levels and using the usual security
mechanisms to protect access to those sockets.

Jason

2013-10-10 16:50:40

by Jason Gunthorpe

Subject: Re: [TrouSerS-tech] [tpmdd-devel] [PATCH 09/13] tpm: Pull everything related to sysfs into tpm-sysfs.c

On Thu, Oct 10, 2013 at 07:42:49AM +0000, Fuchs, Andreas wrote:

> In any case, I like your idea to split trousers IPC to two distinct
> unix sockets for localities. In this case, we could also split tcsd
> into two processes along with it for accessing the distinct
> char-devices and thereby make it more robust against bugs for
> "locality-escalation".

You still have to somehow manage cross locality state between the two
daemons..

> Also remember that many people have developed alternative stacks
> that don't use trousers but operate directly on the char-device.
> They would also benefit from char-device access control for localities.

I am one of those people, we actually don't use any middleware at
all. But to make that work I've had to carry the multi-open patch for
years :|

> Even with only a single trousers, I see no harm in two devices. For
> backwards compatibility, the current /dev/tpm0 could be exported (with
> highest level access control) along with tpm0l1, tpm0l2, ... and/or
> trousers could open both char-devices if it wanted to.

Well, we could start with a 'no way out IOCTL'. So trousers can open
/dev/tpm twice and lock the two FDs to a specific locality then drop
privileges and fork priv-sep style sub processes.

The current kernel code is not ready for multiple char devices, it
will need a device class first..

> The kernel may want to use localityAtRelease OS in order to protect sealed
> data (trusted keyrings) such that user-space could not even unseal

It seems reasonable to have TPM data that will only live in the kernel
to be only releasable by the kernel..

Jason