Skip to contentSkip to conversation

New TPM driver, hwrng driver and fixes

GIT PULL Kent Yoder 3 participants 10 messages Jul 27, 2012 – Aug 14, 2012

Subsystems (4)
lore
Share & tools
lore

Kent Yoder

author
#0 lore raw

Hi James,

Please pull from this new branch and ignore the 7-25-12 branch. This new branch includes fixes for comments by hpa. I've also included one additional patch from [1] to close a race and prevent possibly sensitive data from being free'd before being zeroed. I'm attaching this entire diff here since my fixes for hpa's comments aren't public yet.

Thanks,
Kent

[1] https://lkml.org/lkml/2012/7/25/431

The following changes since commit 663728418e3494f8e4a82f5d1b2f23c22d11be35:

Smack: Maintainer Record (2012-07-13 15:59:44 -0700)

are available in the git repository at:
git://github.com/shpedoikal/linux.git tpmdd-hpa-fixes-7-27-12

Bryan Freed (1):
CHROMIUM: tpm: tpm_i2c_infineon: Lock the I2C adapter for a sequence of requests.

Kent Yoder (4):
tpm: modularize event log collection
tpm: Move tpm_get_random api into the TPM device driver
hw_random: add support for the TPM chip as a hardware RNG source
tpm: fix double write race and tpm_release free issue

Peter Huewe (1):
char/tpm: Add new driver for Infineon I2C TIS TPM

 drivers/char/hw_random/Kconfig                  |   13 +
 drivers/char/hw_random/Makefile                 |    1 +
 drivers/char/hw_random/tpm-rng.c                |   55 ++
 drivers/char/tpm/Kconfig                        |   11 +
 drivers/char/tpm/Makefile                       |    2 +
 drivers/char/tpm/tpm.c                          |   68 ++-
 drivers/char/tpm/tpm.h                          |   23 +
 drivers/char/tpm/tpm_acpi.c                     |  104 ++++
 drivers/char/tpm/{tpm_bios.c => tpm_eventlog.c} |  147 +-----
 drivers/char/tpm/tpm_eventlog.h                 |   71 +++
 drivers/char/tpm/tpm_i2c_infineon.c             |  748 +++++++++++++++++++++++
 include/linux/tpm.h                             |    4 +
 security/keys/trusted.c                         |   48 +--
 13 files changed, 1108 insertions(+), 187 deletions(-)
 create mode 100644 drivers/char/hw_random/tpm-rng.c
 create mode 100644 drivers/char/tpm/tpm_acpi.c
 rename drivers/char/tpm/{tpm_bios.c => tpm_eventlog.c} (75%)
 create mode 100644 drivers/char/tpm/tpm_eventlog.h
 create mode 100644 drivers/char/tpm/tpm_i2c_infineon.c
drivers/char/hw_random/Kconfig +13 -0
index f45dad3..4e8c01a 100644
@@ -263,3 +263,16 @@ config HW_RANDOM_PSERIES
 	  module will be called pseries-rng.
 
 	  If unsure, say Y.
+
+config HW_RANDOM_TPM
+	tristate "TPM HW Random Number Generator support"
+	depends on HW_RANDOM && TCG_TPM
+	default HW_RANDOM
+	---help---
+	  This driver provides kernel-side support for the Random Number
+	  Generator in the Trusted Platform Module
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called tpm-rng.
+
+	  If unsure, say Y.
drivers/char/hw_random/Makefile +1 -0
index d901dfa..5dc10da 100644
@@ -23,3 +23,4 @@ obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PICOXCELL) += picoxcell-rng.o
 obj-$(CONFIG_HW_RANDOM_PPC4XX) += ppc4xx-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
+obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
drivers/char/hw_random/tpm-rng.c new file +55 -0

55 lines not on this page · raw message

drivers/char/tpm/Kconfig +11 -0
index a048199..c4aac48 100644
@@ -33,6 +33,17 @@ config TCG_TIS
 	  from within Linux.  To compile this driver as a module, choose
 	  M here; the module will be called tpm_tis.
 
+config TCG_TIS_I2C_INFINEON
+	tristate "TPM Interface Specification 1.2 Interface (I2C - Infineon)"
+	depends on I2C
+	---help---
+	  If you have a TPM security chip that is compliant with the
+	  TCG TIS 1.2 TPM specification and Infineon's I2C Protocol Stack
+	  Specification 0.20 say Yes and it will be accessible from within
+	  Linux.
+	  To compile this driver as a module, choose M here; the module
+	  will be called tpm_tis_i2c_infineon.
+
 config TCG_NSC
 	tristate "National Semiconductor TPM Interface"
 	depends on X86
drivers/char/tpm/Makefile +2 -0
index ea3a1e0..beac52f6 100644
@@ -4,8 +4,10 @@
 obj-$(CONFIG_TCG_TPM) += tpm.o
 ifdef CONFIG_ACPI
 	obj-$(CONFIG_TCG_TPM) += tpm_bios.o
+	tpm_bios-objs += tpm_eventlog.o tpm_acpi.o
 endif
 obj-$(CONFIG_TCG_TIS) += tpm_tis.o
+obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
 obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
 obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
 obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
drivers/char/tpm/tpm.c +60 -8

104 lines not on this page · raw message

drivers/char/tpm/tpm.h +23 -0
index b1c5280..610fe42 100644
@@ -28,6 +28,12 @@
 #include <linux/io.h>
 #include <linux/tpm.h>
 
+enum tpm_const {
+	TPM_MINOR = 224,	/* officially assigned */
+	TPM_BUFSIZE = 4096,
+	TPM_NUM_DEVICES = 256,
+};
+
 enum tpm_timeout {
 	TPM_TIMEOUT = 5,	/* msecs */
 };
@@ -269,6 +275,21 @@ struct tpm_pcrextend_in {
 	u8	hash[TPM_DIGEST_SIZE];
 }__attribute__((packed));
 
+/* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
+ * bytes, but 128 is still a relatively large number of random bytes and
+ * anything much bigger causes users of struct tpm_cmd_t to start getting
+ * compiler warnings about stack frame size. */
+#define TPM_MAX_RNG_DATA	128
+
+struct tpm_getrandom_out {
+	__be32 rng_data_len;
+	u8     rng_data[TPM_MAX_RNG_DATA];
+}__attribute__((packed));
+
+struct tpm_getrandom_in {
+	__be32 num_bytes;
+}__attribute__((packed));
+
 typedef union {
 	struct	tpm_getcap_params_out getcap_out;
 	struct	tpm_readpubek_params_out readpubek_out;
@@ -277,6 +298,8 @@ typedef union {
 	struct	tpm_pcrread_in	pcrread_in;
 	struct	tpm_pcrread_out	pcrread_out;
 	struct	tpm_pcrextend_in pcrextend_in;
+	struct	tpm_getrandom_in getrandom_in;
+	struct	tpm_getrandom_out getrandom_out;
 } tpm_cmd_params;
 
 struct tpm_cmd_t {
drivers/char/tpm/tpm_acpi.c new file +104 -0

104 lines not on this page · raw message

drivers/char/tpm/tpm_eventlog.c renamed from drivers/char/tpm/tpm_bios.c +5 -142

179 lines not on this page · raw message

drivers/char/tpm/tpm_eventlog.h new file +71 -0

71 lines not on this page · raw message

drivers/char/tpm/tpm_i2c_infineon.c new file +748 -0

748 lines not on this page · raw message

include/linux/tpm.h +4 -0
index fdc718a..d5b2f2d 100644
@@ -32,6 +32,7 @@
 extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
 extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
 extern int tpm_send(u32 chip_num, void *cmd, size_t buflen);
+extern int tpm_get_random(u32 chip_num, u8 *data, size_t *max);
 #else
 static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
 	return -ENODEV;
@@ -42,5 +43,8 @@ static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
 static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
 	return -ENODEV;
 }
+static inline int tpm_get_random(u32 chip_num, u8 *data, size_t *max) {
+	return -ENODEV;
+}
 #endif
 #endif
security/keys/trusted.c +11 -37

108 lines not on this page · raw message

#1 lore raw
Quoted 50 lines · Kent Yoder

Hi James,

Please pull from this new branch and ignore the 7-25-12 branch. This new branch includes fixes for comments by hpa. I've also included one additional patch from [1] to close a race and prevent possibly sensitive data from being free'd before being zeroed. I'm attaching this entire diff here since my fixes for hpa's comments aren't public yet.

Thanks,
Kent

drivers/char/tpm/tpm.c +39 -0
+
+/**
+ * tpm_get_random() - Get random bytes from the tpm's RNG
+ * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * @out: destination buffer for the random bytes
+ * @max: on input, the max number of bytes to write to @out, on output
+ *       this is set to the actual number of bytes written to @out
+ *
+ * Note that @max will be capped at TPM_MAX_RNG_DATA bytes.
+ */
+int tpm_get_random(u32 chip_num, u8 *out, size_t *max)
+{
+	struct tpm_chip *chip;
+	struct tpm_cmd_t tpm_cmd;
+	u32 recd, total = 0, num_bytes = min_t(u32, *max, TPM_MAX_RNG_DATA);
+	int err, retries = 5;
+	u8 *dest = out;
+
+	chip = tpm_chip_find_get(chip_num);
+	if (chip == NULL)
+		return -ENODEV;
+
+	if (!out || !num_bytes || *max > TPM_MAX_RN
+		return -EINVAL;
+
+	do {
+		tpm_cmd.header.in = tpm_getrandom_header;
+		tpm_cmd.params.getrandom_in.num_bytes = cpu_to_be32(num_bytes);
+
+		err = transmit_cmd(chip, &tpm_cmd,
+				TPM_GETRANDOM_RESULT_SIZE + num_bytes,
+				"attempting get random");
+		if (err)
+			goto out_err;
+
+		recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len);
+		memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd);
+
+		dest += recd;

drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev) · line 1380

Quoted 10 lines
drivers/char/tpm/tpm.c +10 -0
+		total += recd;
+		num_bytes -= recd;
+	} while (retries-- && total < *max);
+
+	err = ((total < *max) ? -EAGAIN : 0);
+out_err:
+	return err;
+}
+EXPORT_SYMBOL_GPL(tpm_get_random);
+

Since you no longer modify *max anywhere in this function, why leave it a pointer? Making it pass by value seems more logical at that point (and cleaner).

The only consumer which can make use of partial result is tpm_rng_read(), but that will now return zero unless the buffer is filled.

My suggestion would be to drop the pointer and instead return a positive result (number of bytes) if you read anything and a negative result (-errno) on error.

That way, a caller which can only use an exact number of bytes should do:

	rv = tpm_get_random(chip, buf, size);
	if (rv != size)
		/* error! */

... and tpm_rng_read() can do something smarter.

-hpa

author
#2 lore raw
Quoted 11 lines

Since you no longer modify *max anywhere in this function, why leave it a pointer? Making it pass by value seems more logical at that point (and cleaner).

The only consumer which can make use of partial result is tpm_rng_read(), but that will now return zero unless the buffer is filled.

My suggestion would be to drop the pointer and instead return a positive result (number of bytes) if you read anything and a negative result (-errno) on error.

Sounds good. I'll make this change.

Kent

Quoted 10 lines

That way, a caller which can only use an exact number of bytes should do:

	rv = tpm_get_random(chip, buf, size);
	if (rv != size)
		/* error! */

... and tpm_rng_read() can do something smarter.

-hpa

author Acked-by David Safford Acked-by David Safford
#3 lore raw
In reply to H. Peter Anvin

Hi,

Quoted 13 lines

Since you no longer modify *max anywhere in this function, why leave it a pointer? Making it pass by value seems more logical at that point (and cleaner).

The only consumer which can make use of partial result is tpm_rng_read(), but that will now return zero unless the buffer is filled.

My suggestion would be to drop the pointer and instead return a positive result (number of bytes) if you read anything and a negative result (-errno) on error.

That way, a caller which can only use an exact number of bytes should do:

Quoted 7 lines
	rv = tpm_get_random(chip, buf, size);
	if (rv != size)
		/* error! */

... and tpm_rng_read() can do something smarter.

-hpa

Alrighty, please take a look at these two updated patches that should
address your comments. This made the rng driver even simpler than it was, and simplified the trusted keys changes. I only needed to make one update in tpm_get_random to cover for the case when the error was returned by the tpm itself, where that error code could be a positive value.

Dave, I'd appreciate one more Ack here, since your code did change.

Thanks,
Kent

commit 477ec712124adb9a3317139288d550ec4f6c2b2e

Author: Kent Yoder <[email protected]>
Date:   Thu Jun 7 13:47:42 2012 -0500

hw_random: add support for the TPM chip as a hardware RNG source

This driver will make use of any available TPM chip on the system as a hwrng source.

drivers/char/hw_random/Kconfig +13 -0
index f45dad3..4e8c01a 100644
@@ -263,3 +263,16 @@ config HW_RANDOM_PSERIES
 	  module will be called pseries-rng.
 
 	  If unsure, say Y.
+
+config HW_RANDOM_TPM
+	tristate "TPM HW Random Number Generator support"
+	depends on HW_RANDOM && TCG_TPM
+	default HW_RANDOM
+	---help---
+	  This driver provides kernel-side support for the Random Number
+	  Generator in the Trusted Platform Module
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called tpm-rng.
+
+	  If unsure, say Y.
drivers/char/hw_random/Makefile +1 -0
index d901dfa..5dc10da 100644
@@ -23,3 +23,4 @@ obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PICOXCELL) += picoxcell-rng.o
 obj-$(CONFIG_HW_RANDOM_PPC4XX) += ppc4xx-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
+obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
drivers/char/hw_random/tpm-rng.c new file +50 -0

50 lines not on this page · raw message

commit ad5f963f7b69b9f798944ebc0bbb02520ecf5cae

Author: Kent Yoder <[email protected]>
Date:   Thu Jun 7 13:47:14 2012 -0500

tpm: Move tpm_get_random api into the TPM device driver

Move the tpm_get_random api from the trusted keys code into the TPM device driver itself so that other callers can make use of it. Also, change the api slightly so that the number of bytes read is returned in the call, since the TPM command can potentially return fewer bytes than requested.

drivers/char/tpm/tpm.c +59 -6

83 lines not on this page · raw message

drivers/char/tpm/tpm.h +23 -0
index b1c5280..610fe42 100644
@@ -28,6 +28,12 @@
 #include <linux/io.h>
 #include <linux/tpm.h>
 
+enum tpm_const {
+	TPM_MINOR = 224,	/* officially assigned */
+	TPM_BUFSIZE = 4096,
+	TPM_NUM_DEVICES = 256,
+};
+
 enum tpm_timeout {
 	TPM_TIMEOUT = 5,	/* msecs */
 };
@@ -269,6 +275,21 @@ struct tpm_pcrextend_in {
 	u8	hash[TPM_DIGEST_SIZE];
 }__attribute__((packed));
 
+/* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
+ * bytes, but 128 is still a relatively large number of random bytes and
+ * anything much bigger causes users of struct tpm_cmd_t to start getting
+ * compiler warnings about stack frame size. */
+#define TPM_MAX_RNG_DATA	128
+
+struct tpm_getrandom_out {
+	__be32 rng_data_len;
+	u8     rng_data[TPM_MAX_RNG_DATA];
+}__attribute__((packed));
+
+struct tpm_getrandom_in {
+	__be32 num_bytes;
+}__attribute__((packed));
+
 typedef union {
 	struct	tpm_getcap_params_out getcap_out;
 	struct	tpm_readpubek_params_out readpubek_out;
@@ -277,6 +298,8 @@ typedef union {
 	struct	tpm_pcrread_in	pcrread_in;
 	struct	tpm_pcrread_out	pcrread_out;
 	struct	tpm_pcrextend_in pcrextend_in;
+	struct	tpm_getrandom_in getrandom_in;
+	struct	tpm_getrandom_out getrandom_out;
 } tpm_cmd_params;
 
 struct tpm_cmd_t {
include/linux/tpm.h +4 -0
index fdc718a..fcb627f 100644
@@ -32,6 +32,7 @@
 extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
 extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
 extern int tpm_send(u32 chip_num, void *cmd, size_t buflen);
+extern int tpm_get_random(u32 chip_num, u8 *data, size_t max);
 #else
 static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
 	return -ENODEV;
@@ -42,5 +43,8 @@ static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
 static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
 	return -ENODEV;
 }
+static inline int tpm_get_random(u32 chip_num, u8 *data, size_t max) {
+	return -ENODEV;
+}
 #endif
 #endif
security/keys/trusted.c +7 -37

86 lines not on this page · raw message

#4 lore raw

drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev) · line 1365

Quoted 12 lines · Kent Yoder
drivers/char/tpm/tpm.c +12 -0
+
+	do {
+		tpm_cmd.header.in = tpm_getrandom_header;
+		tpm_cmd.params.getrandom_in.num_bytes = cpu_to_be32(num_bytes);
+
+		err = transmit_cmd(chip, &tpm_cmd,
+				   TPM_GETRANDOM_RESULT_SIZE + num_bytes,
+				   "attempting get random");
+		if (err) {
+			/* err can be positive if it came from the TPM itself,
+			 * so return a negative value here instead. */
+			err = -EFAULT;

-EFAULT is definitely wrong (that means a bad pointer was passed), you can use -EIO instead.

However, I would suggest:

	err = total ? total : -EIO;

... so you report the number of bytes successfully received if we got any. However, since you *also* do that on the retry line,

drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev) · line 1377

Quoted 12 lines
drivers/char/tpm/tpm.c +12 -0
+			goto out_err;
+		}
+
+		recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len);
+		memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd);
+
+		dest += recd;
+		total += recd;
+		num_bytes -= recd;
+	} while (retries-- && total < max);
+
+	err = total;

Should we return something other than 0 if we run out of retries here, too?

Perhaps we should just do the same "err = total ? total : -EIO;" here and the above statement can just turn into a break;.

security/keys/trusted.c static int pcrlock(const int pcrnum) · line 386

Quoted 4 lines
security/keys/trusted.c +1 -1
-	ret = my_get_random(hash, SHA1_DIGEST_SIZE);
+	ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
 	if (ret < 0)
 		return ret;

You are still not checking the return values correctly!

This needs to be something like:

	ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
	if (ret != SHA1_DIGEST_SIZE)
		return -EIO;	/* Or whatever is appropriate here */


	-hpa
#5 lore raw
Quoted 10 lines

drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev) · line 1365

Quoted 3 lines
drivers/char/tpm/tpm.c +3 -0
+                     /* err can be positive if it came from the TPM itself,
+                      * so return a negative value here instead. */
+                     err = -EFAULT;

-EFAULT is definitely wrong (that means a bad pointer was passed), you can use -EIO instead.

However, I would suggest:

        err = total ? total : -EIO;

This is fine w/ me...

Quoted 17 lines

... so you report the number of bytes successfully received if we got any. However, since you *also* do that on the retry line,

drivers/char/tpm/tpm.c int tpm_pm_resume(struct device *dev) · line 1377

Quoted 12 lines
drivers/char/tpm/tpm.c +12 -0
+                     goto out_err;
+             }
+
+             recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len);
+             memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd);
+
+             dest += recd;
+             total += recd;
+             num_bytes -= recd;
+     } while (retries-- && total < max);
+
+     err = total;

Should we return something other than 0 if we run out of retries here, too?

Ugh, I was hoping to avoid this kind of complexity.

Quoted 2 lines

Perhaps we should just do the same "err = total ? total : -EIO;" here and the above statement can just turn into a break;.

Yeah, this seems like the right thing to do.

Quoted 6 lines

security/keys/trusted.c static int pcrlock(const int pcrnum) · line 386

Quoted 4 lines
security/keys/trusted.c +1 -1
-     ret = my_get_random(hash, SHA1_DIGEST_SIZE);
+     ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
      if (ret < 0)
              return ret;

You are still not checking the return values correctly!

Dave, can you weigh in on these individual cases? In some cases
like capping a pcr I think using uninitialized stack data could be better than failing...

Kent

Quoted 13 lines

This needs to be something like:

        ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
        if (ret != SHA1_DIGEST_SIZE)
                return -EIO;    /* Or whatever is appropriate here */


        -hpa

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 1 line
IBM LTC Security
author Acked-by David Safford
#6 lore raw
Quoted 11 lines
Quoted 3 lines

Perhaps we should just do the same "err = total ? total : -EIO;" here and the above statement can just turn into a break;.

Yeah, this seems like the right thing to do.

Quoted 5 lines
Quoted 4 lines
(quoted diff) +1 -1
-     ret = my_get_random(hash, SHA1_DIGEST_SIZE);
+     ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
      if (ret < 0)
              return ret;
Quoted 5 lines
Quoted 1 line

You are still not checking the return values correctly!

Dave, can you weigh in on these individual cases? In some cases
like capping a pcr I think using uninitialized stack data could be better than failing...

I got in touch with Dave today, who'd like the full checking done now that we can. Here's a new patch, let me know what you think...

Thanks,
Kent

commit 85d3caa4bb08da4d1761560ae0fdf13f94abf53a

Author: Kent Yoder <[email protected]>
Date:   Thu Jun 7 13:47:14 2012 -0500

tpm: Move tpm_get_random api into the TPM device driver

Move the tpm_get_random api from the trusted keys code into the TPM device driver itself so that other callers can make use of it. Also, change the api slightly so that the number of bytes read is returned in the call, since the TPM command can potentially return fewer bytes than requested.

drivers/char/tpm/tpm.c +53 -6
index beb98c3..43753ec 100644
@@ -32,12 +32,6 @@
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
-enum tpm_const {
-	TPM_MINOR = 224,	/* officially assigned */
-	TPM_BUFSIZE = 4096,
-	TPM_NUM_DEVICES = 256,
-};
-
 enum tpm_duration {
 	TPM_SHORT = 0,
 	TPM_MEDIUM = 1,
@@ -483,6 +477,7 @@ 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)
 
 static const struct tpm_input_header tpm_getcap_header = {
 	.tag = TPM_TAG_RQU_COMMAND,
@@ -1327,6 +1322,58 @@ int tpm_pm_resume(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
+#define TPM_GETRANDOM_RESULT_SIZE	18
+static struct tpm_input_header tpm_getrandom_header = {
+	.tag = TPM_TAG_RQU_COMMAND,
+	.length = cpu_to_be32(14),
+	.ordinal = TPM_ORD_GET_RANDOM
+};
+
+/**
+ * tpm_get_random() - Get random bytes from the tpm's RNG
+ * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * @out: destination buffer for the random bytes
+ * @max: the max number of bytes to write to @out
+ *
+ * Returns < 0 on error and the number of bytes read on success
+ */
+int tpm_get_random(u32 chip_num, u8 *out, size_t max)
+{
+	struct tpm_chip *chip;
+	struct tpm_cmd_t tpm_cmd;
+	u32 recd, num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA);
+	int err, total = 0, retries = 5;
+	u8 *dest = out;
+
+	chip = tpm_chip_find_get(chip_num);
+	if (chip == NULL)
+		return -ENODEV;
+
+	if (!out || !num_bytes || max > TPM_MAX_RNG_DATA)
+		return -EINVAL;
+
+	do {
+		tpm_cmd.header.in = tpm_getrandom_header;
+		tpm_cmd.params.getrandom_in.num_bytes = cpu_to_be32(num_bytes);
+
+		err = transmit_cmd(chip, &tpm_cmd,
+				   TPM_GETRANDOM_RESULT_SIZE + num_bytes,
+				   "attempting get random");
+		if (err)
+			break;
+
+		recd = be32_to_cpu(tpm_cmd.params.getrandom_out.rng_data_len);
+		memcpy(dest, tpm_cmd.params.getrandom_out.rng_data, recd);
+
+		dest += recd;
+		total += recd;
+		num_bytes -= recd;
+	} while (retries-- && total < max);
+
+	return total ? total : -EIO;
+}
+EXPORT_SYMBOL_GPL(tpm_get_random);
+
 /* In case vendor provided release function, call it too.*/
 
 void tpm_dev_vendor_release(struct tpm_chip *chip)
drivers/char/tpm/tpm.h +23 -0
index b1c5280..610fe42 100644
@@ -28,6 +28,12 @@
 #include <linux/io.h>
 #include <linux/tpm.h>
 
+enum tpm_const {
+	TPM_MINOR = 224,	/* officially assigned */
+	TPM_BUFSIZE = 4096,
+	TPM_NUM_DEVICES = 256,
+};
+
 enum tpm_timeout {
 	TPM_TIMEOUT = 5,	/* msecs */
 };
@@ -269,6 +275,21 @@ struct tpm_pcrextend_in {
 	u8	hash[TPM_DIGEST_SIZE];
 }__attribute__((packed));
 
+/* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
+ * bytes, but 128 is still a relatively large number of random bytes and
+ * anything much bigger causes users of struct tpm_cmd_t to start getting
+ * compiler warnings about stack frame size. */
+#define TPM_MAX_RNG_DATA	128
+
+struct tpm_getrandom_out {
+	__be32 rng_data_len;
+	u8     rng_data[TPM_MAX_RNG_DATA];
+}__attribute__((packed));
+
+struct tpm_getrandom_in {
+	__be32 num_bytes;
+}__attribute__((packed));
+
 typedef union {
 	struct	tpm_getcap_params_out getcap_out;
 	struct	tpm_readpubek_params_out readpubek_out;
@@ -277,6 +298,8 @@ typedef union {
 	struct	tpm_pcrread_in	pcrread_in;
 	struct	tpm_pcrread_out	pcrread_out;
 	struct	tpm_pcrextend_in pcrextend_in;
+	struct	tpm_getrandom_in getrandom_in;
+	struct	tpm_getrandom_out getrandom_out;
 } tpm_cmd_params;
 
 struct tpm_cmd_t {
include/linux/tpm.h +4 -0
index fdc718a..fcb627f 100644
@@ -32,6 +32,7 @@
 extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
 extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
 extern int tpm_send(u32 chip_num, void *cmd, size_t buflen);
+extern int tpm_get_random(u32 chip_num, u8 *data, size_t max);
 #else
 static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
 	return -ENODEV;
@@ -42,5 +43,8 @@ static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
 static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
 	return -ENODEV;
 }
+static inline int tpm_get_random(u32 chip_num, u8 *data, size_t max) {
+	return -ENODEV;
+}
 #endif
 #endif
security/keys/trusted.c +12 -42

96 lines not on this page · raw message

Reviewed-by
#7 lore raw
Quoted 17 lines · Kent Yoder
Quoted 11 lines
Quoted 3 lines

Perhaps we should just do the same "err = total ? total : -EIO;" here and the above statement can just turn into a break;.

Yeah, this seems like the right thing to do.

Quoted 5 lines
Quoted 4 lines
security/keys/trusted.c +1 -1
-     ret = my_get_random(hash, SHA1_DIGEST_SIZE);
+     ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
      if (ret < 0)
              return ret;
Quoted 5 lines
Quoted 1 line

You are still not checking the return values correctly!

Dave, can you weigh in on these individual cases? In some cases
like capping a pcr I think using uninitialized stack data could be better than failing...

Quoted 6 lines

I got in touch with Dave today, who'd like the full checking done now that we can. Here's a new patch, let me know what you think...

Thanks,
Kent

Looks good to me.

-hpa

#8 lore raw
In reply to Kent Yoder
Quoted 8 lines · Kent Yoder

Hi James,

Please pull from this new branch and ignore the 7-25-12 branch. This new branch includes fixes for comments by hpa. I've also included one additional patch from [1] to close a race and prevent possibly sensitive data from being free'd before being zeroed. I'm attaching this entire diff here since my fixes for hpa's comments aren't public yet.

Quoted 5 lines

Thanks,
Kent

[1] https://lkml.org/lkml/2012/7/25/431

Hi guys,

I was wondering what the status on this tree is?

-hpa

author
#9 lore raw
Quoted 12 lines · H. Peter Anvin
Quoted 11 lines · Kent Yoder

Hi James,

Please pull from this new branch and ignore the 7-25-12 branch. This new branch includes fixes for comments by hpa. I've also included one additional patch from [1] to close a race and prevent possibly sensitive data from being free'd before being zeroed. I'm attaching this entire diff here since my fixes for hpa's comments aren't public yet.

Thanks,
Kent

Quoted 6 lines
Quoted 2 lines

[1] https://lkml.org/lkml/2012/7/25/431

Hi guys,

I was wondering what the status on this tree is?

Hi, I'm staging these patches against latest upstream and will be
hopefully adding another new driver and PPI patches from Intel on top, to be included in a new pullreq for James.

Kent

Quoted 3 lines

-hpa