[PATCH v2 0/4] Refactor TPM event log code

[PATCH v2 0/4] Refactor TPM event log code

[Thiebaud Weksteen]

This patchset implements the proposal from Jarkko Sakkinen [1]. I have
included the feedback from Nayna Jain about the function naming.

[1] https://lkml.kernel.org/r/20171024222148.gwnkj5vqsyj43qer@linux.intel.com

Changes since v1:
- Add Suggested-by
- Fix kbuild report

Thiebaud Weksteen (4):
  tpm: Add explicit endianness cast
  tpm: Move eventlog files to a subdirectory
  tpm: Move shared eventlog functions to common.c
  tpm: Move eventlog declarations to its own header

 drivers/char/tpm/Makefile                     |  10 +-
 .../{tpm_eventlog_acpi.c => eventlog/acpi.c}  |   3 +-
 drivers/char/tpm/eventlog/common.c            | 195 +++++++++++++++++
 drivers/char/tpm/eventlog/common.h            |  35 +++
 .../{tpm_eventlog_efi.c => eventlog/efi.c}    |   3 +-
 .../tpm/{tpm_eventlog_of.c => eventlog/of.c}  |   7 +-
 .../tpm/{tpm1_eventlog.c => eventlog/tpm1.c}  | 200 ++----------------
 .../tpm/{tpm2_eventlog.c => eventlog/tpm2.c}  |   3 +-
 drivers/char/tpm/tpm.h                        |  27 ---
 9 files changed, 262 insertions(+), 221 deletions(-)
 rename drivers/char/tpm/{tpm_eventlog_acpi.c => eventlog/acpi.c} (98%)
 create mode 100644 drivers/char/tpm/eventlog/common.c
 create mode 100644 drivers/char/tpm/eventlog/common.h
 rename drivers/char/tpm/{tpm_eventlog_efi.c => eventlog/efi.c} (97%)
 rename drivers/char/tpm/{tpm_eventlog_of.c => eventlog/of.c} (94%)
 rename drivers/char/tpm/{tpm1_eventlog.c => eventlog/tpm1.c} (58%)
 rename drivers/char/tpm/{tpm2_eventlog.c => eventlog/tpm2.c} (99%)

-- 
2.17.0.484.g0c8726318c-goog

[PATCH v2 1/4] tpm: Add explicit endianness cast

[Thiebaud Weksteen]

Signed-off-by: Thiebaud Weksteen <tweek@google.com>
---
 drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_eventlog_of.c b/drivers/char/tpm/tpm_eventlog_of.c
index 96fd5646f866..d74568d58a66 100644
--- a/drivers/char/tpm/tpm_eventlog_of.c
+++ b/drivers/char/tpm/tpm_eventlog_of.c
@@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	 * but physical tpm needs the conversion.
 	 */
 	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
-		size = be32_to_cpup(sizep);
-		base = be64_to_cpup(basep);
+		size = be32_to_cpup((__be32 *)sizep);
+		base = be64_to_cpup((__be64 *)basep);
 	} else {
 		size = *sizep;
 		base = *basep;
-- 
2.17.0.484.g0c8726318c-goog

[PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

[Thiebaud Weksteen]

Signed-off-by: Thiebaud Weksteen <tweek@google.com>
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/Makefile                                 | 8 ++++----
 drivers/char/tpm/{tpm_eventlog_acpi.c => eventlog/acpi.c} | 2 +-
 drivers/char/tpm/{tpm_eventlog_efi.c => eventlog/efi.c}   | 2 +-
 drivers/char/tpm/{tpm_eventlog_of.c => eventlog/of.c}     | 2 +-
 drivers/char/tpm/{tpm1_eventlog.c => eventlog/tpm1.c}     | 2 +-
 drivers/char/tpm/{tpm2_eventlog.c => eventlog/tpm2.c}     | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)
 rename drivers/char/tpm/{tpm_eventlog_acpi.c => eventlog/acpi.c} (99%)
 rename drivers/char/tpm/{tpm_eventlog_efi.c => eventlog/efi.c} (98%)
 rename drivers/char/tpm/{tpm_eventlog_of.c => eventlog/of.c} (99%)
 rename drivers/char/tpm/{tpm1_eventlog.c => eventlog/tpm1.c} (99%)
 rename drivers/char/tpm/{tpm2_eventlog.c => eventlog/tpm2.c} (99%)

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index acd758381c58..5dcf5bd35a3d 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -4,11 +4,11 @@
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-	 tpm-dev-common.o tpmrm-dev.o tpm1_eventlog.o tpm2_eventlog.o \
+	 tpm-dev-common.o tpmrm-dev.o eventlog/tpm1.o eventlog/tpm2.o \
          tpm2-space.o
-tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o
-tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o
-tpm-$(CONFIG_OF) += tpm_eventlog_of.o
+tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
+tpm-$(CONFIG_EFI) += eventlog/efi.o
+tpm-$(CONFIG_OF) += eventlog/of.o
 obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o
 obj-$(CONFIG_TCG_TIS) += tpm_tis.o
 obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
diff --git a/drivers/char/tpm/tpm_eventlog_acpi.c b/drivers/char/tpm/eventlog/acpi.c
similarity index 99%
rename from drivers/char/tpm/tpm_eventlog_acpi.c
rename to drivers/char/tpm/eventlog/acpi.c
index 66f19e93c216..8476be2e9526 100644
--- a/drivers/char/tpm/tpm_eventlog_acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -27,7 +27,7 @@
 #include <linux/acpi.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 struct acpi_tcpa {
 	struct acpi_table_header hdr;
diff --git a/drivers/char/tpm/tpm_eventlog_efi.c b/drivers/char/tpm/eventlog/efi.c
similarity index 98%
rename from drivers/char/tpm/tpm_eventlog_efi.c
rename to drivers/char/tpm/eventlog/efi.c
index e3f9ffd341d2..e1593c5271a4 100644
--- a/drivers/char/tpm/tpm_eventlog_efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -14,7 +14,7 @@
 #include <linux/efi.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 /* read binary bios log from EFI configuration table */
 int tpm_read_log_efi(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/tpm_eventlog_of.c b/drivers/char/tpm/eventlog/of.c
similarity index 99%
rename from drivers/char/tpm/tpm_eventlog_of.c
rename to drivers/char/tpm/eventlog/of.c
index d74568d58a66..d98c184d5c13 100644
--- a/drivers/char/tpm/tpm_eventlog_of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -19,7 +19,7 @@
 #include <linux/of.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 int tpm_read_log_of(struct tpm_chip *chip)
 {
diff --git a/drivers/char/tpm/tpm1_eventlog.c b/drivers/char/tpm/eventlog/tpm1.c
similarity index 99%
rename from drivers/char/tpm/tpm1_eventlog.c
rename to drivers/char/tpm/eventlog/tpm1.c
index add798bd69d0..d6aea3ca950e 100644
--- a/drivers/char/tpm/tpm1_eventlog.c
+++ b/drivers/char/tpm/eventlog/tpm1.c
@@ -28,7 +28,7 @@
 #include <linux/slab.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 
 static const char* tcpa_event_type_strings[] = {
diff --git a/drivers/char/tpm/tpm2_eventlog.c b/drivers/char/tpm/eventlog/tpm2.c
similarity index 99%
rename from drivers/char/tpm/tpm2_eventlog.c
rename to drivers/char/tpm/eventlog/tpm2.c
index 1ce4411292ba..f0723fa9ae14 100644
--- a/drivers/char/tpm/tpm2_eventlog.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -23,7 +23,7 @@
 #include <linux/slab.h>
 #include <linux/tpm_eventlog.h>
 
-#include "tpm.h"
+#include "../tpm.h"
 
 /*
  * calc_tpm2_event_size() - calculate the event size, where event
-- 
2.17.0.484.g0c8726318c-goog

[PATCH v2 3/4] tpm: Move shared eventlog functions to common.c

[Thiebaud Weksteen]

Functions and structures specific to TPM1 are renamed from tpm* to tpm1*.

Signed-off-by: Thiebaud Weksteen <tweek@google.com>
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/Makefile          |   4 +-
 drivers/char/tpm/eventlog/common.c | 195 ++++++++++++++++++++++++++++
 drivers/char/tpm/eventlog/tpm1.c   | 197 +++--------------------------
 drivers/char/tpm/tpm.h             |   2 +
 4 files changed, 214 insertions(+), 184 deletions(-)
 create mode 100644 drivers/char/tpm/eventlog/common.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 5dcf5bd35a3d..4e9c33ca1f8f 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -4,8 +4,8 @@
 #
 obj-$(CONFIG_TCG_TPM) += tpm.o
 tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \
-	 tpm-dev-common.o tpmrm-dev.o eventlog/tpm1.o eventlog/tpm2.o \
-         tpm2-space.o
+	 tpm-dev-common.o tpmrm-dev.o eventlog/common.o eventlog/tpm1.o \
+	 eventlog/tpm2.o tpm2-space.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
 tpm-$(CONFIG_EFI) += eventlog/efi.o
 tpm-$(CONFIG_OF) += eventlog/of.o
diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c
new file mode 100644
index 000000000000..54934b5a1566
--- /dev/null
+++ b/drivers/char/tpm/eventlog/common.c
@@ -0,0 +1,195 @@
+/*
+ * Copyright (C) 2005, 2012 IBM Corporation
+ *
+ * Authors:
+ *	Kent Yoder <key@linux.vnet.ibm.com>
+ *	Seiji Munetoh <munetoh@jp.ibm.com>
+ *	Stefan Berger <stefanb@us.ibm.com>
+ *	Reiner Sailer <sailer@watson.ibm.com>
+ *	Kylene Hall <kjhall@us.ibm.com>
+ *	Nayna Jain <nayna@linux.vnet.ibm.com>
+ *
+ * Access to the event log created by a system's firmware / BIOS
+ *
+ * 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; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ */
+
+#include <linux/seq_file.h>
+#include <linux/fs.h>
+#include <linux/security.h>
+#include <linux/module.h>
+#include <linux/tpm_eventlog.h>
+
+#include "../tpm.h"
+
+
+static int tpm_bios_measurements_open(struct inode *inode,
+					    struct file *file)
+{
+	int err;
+	struct seq_file *seq;
+	struct tpm_chip_seqops *chip_seqops;
+	const struct seq_operations *seqops;
+	struct tpm_chip *chip;
+
+	inode_lock(inode);
+	if (!inode->i_private) {
+		inode_unlock(inode);
+		return -ENODEV;
+	}
+	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
+	seqops = chip_seqops->seqops;
+	chip = chip_seqops->chip;
+	get_device(&chip->dev);
+	inode_unlock(inode);
+
+	/* now register seq file */
+	err = seq_open(file, seqops);
+	if (!err) {
+		seq = file->private_data;
+		seq->private = chip;
+	}
+
+	return err;
+}
+
+static int tpm_bios_measurements_release(struct inode *inode,
+					 struct file *file)
+{
+	struct seq_file *seq = (struct seq_file *)file->private_data;
+	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
+
+	put_device(&chip->dev);
+
+	return seq_release(inode, file);
+}
+
+static const struct file_operations tpm_bios_measurements_ops = {
+	.owner = THIS_MODULE,
+	.open = tpm_bios_measurements_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = tpm_bios_measurements_release,
+};
+
+static int tpm_read_log(struct tpm_chip *chip)
+{
+	int rc;
+
+	if (chip->log.bios_event_log != NULL) {
+		dev_dbg(&chip->dev,
+			"%s: ERROR - event log already initialized\n",
+			__func__);
+		return -EFAULT;
+	}
+
+	rc = tpm_read_log_acpi(chip);
+	if (rc != -ENODEV)
+		return rc;
+
+	rc = tpm_read_log_efi(chip);
+	if (rc != -ENODEV)
+		return rc;
+
+	return tpm_read_log_of(chip);
+}
+
+/*
+ * tpm_bios_log_setup() - Read the event log from the firmware
+ * @chip: TPM chip to use.
+ *
+ * If an event log is found then the securityfs files are setup to
+ * export it to userspace, otherwise nothing is done.
+ *
+ * Returns -ENODEV if the firmware has no event log or securityfs is not
+ * supported.
+ */
+int tpm_bios_log_setup(struct tpm_chip *chip)
+{
+	const char *name = dev_name(&chip->dev);
+	unsigned int cnt;
+	int log_version;
+	int rc = 0;
+
+	rc = tpm_read_log(chip);
+	if (rc < 0)
+		return rc;
+	log_version = rc;
+
+	cnt = 0;
+	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
+	/* NOTE: securityfs_create_dir can return ENODEV if securityfs is
+	 * compiled out. The caller should ignore the ENODEV return code.
+	 */
+	if (IS_ERR(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
+
+	chip->bin_log_seqops.chip = chip;
+	if (log_version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+		chip->bin_log_seqops.seqops =
+			&tpm2_binary_b_measurements_seqops;
+	else
+		chip->bin_log_seqops.seqops =
+			&tpm1_binary_b_measurements_seqops;
+
+
+	chip->bios_dir[cnt] =
+	    securityfs_create_file("binary_bios_measurements",
+				   0440, chip->bios_dir[0],
+				   (void *)&chip->bin_log_seqops,
+				   &tpm_bios_measurements_ops);
+	if (IS_ERR(chip->bios_dir[cnt]))
+		goto err;
+	cnt++;
+
+	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
+
+		chip->ascii_log_seqops.chip = chip;
+		chip->ascii_log_seqops.seqops =
+			&tpm1_ascii_b_measurements_seqops;
+
+		chip->bios_dir[cnt] =
+			securityfs_create_file("ascii_bios_measurements",
+					       0440, chip->bios_dir[0],
+					       (void *)&chip->ascii_log_seqops,
+					       &tpm_bios_measurements_ops);
+		if (IS_ERR(chip->bios_dir[cnt]))
+			goto err;
+		cnt++;
+	}
+
+	return 0;
+
+err:
+	rc = PTR_ERR(chip->bios_dir[cnt]);
+	chip->bios_dir[cnt] = NULL;
+	tpm_bios_log_teardown(chip);
+	return rc;
+}
+
+void tpm_bios_log_teardown(struct tpm_chip *chip)
+{
+	int i;
+	struct inode *inode;
+
+	/* securityfs_remove currently doesn't take care of handling sync
+	 * between removal and opening of pseudo files. To handle this, a
+	 * workaround is added by making i_private = NULL here during removal
+	 * and to check it during open(), both within inode_lock()/unlock().
+	 * This design ensures that open() either safely gets kref or fails.
+	 */
+	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
+		if (chip->bios_dir[i]) {
+			inode = d_inode(chip->bios_dir[i]);
+			inode_lock(inode);
+			inode->i_private = NULL;
+			inode_unlock(inode);
+			securityfs_remove(chip->bios_dir[i]);
+		}
+	}
+}
diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c
index d6aea3ca950e..8f30316e9bb6 100644
--- a/drivers/char/tpm/eventlog/tpm1.c
+++ b/drivers/char/tpm/eventlog/tpm1.c
@@ -71,7 +71,7 @@ static const char* tcpa_pc_event_id_strings[] = {
 };
 
 /* returns pointer to start of pos. entry of tcg log */
-static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
+static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos)
 {
 	loff_t i;
 	struct tpm_chip *chip = m->private;
@@ -118,7 +118,7 @@ static void *tpm_bios_measurements_start(struct seq_file *m, loff_t *pos)
 	return addr;
 }
 
-static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
+static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
 					loff_t *pos)
 {
 	struct tcpa_event *event = v;
@@ -149,7 +149,7 @@ static void *tpm_bios_measurements_next(struct seq_file *m, void *v,
 	return v;
 }
 
-static void tpm_bios_measurements_stop(struct seq_file *m, void *v)
+static void tpm1_bios_measurements_stop(struct seq_file *m, void *v)
 {
 }
 
@@ -232,7 +232,7 @@ static int get_event_name(char *dest, struct tcpa_event *event,
 
 }
 
-static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
+static int tpm1_binary_bios_measurements_show(struct seq_file *m, void *v)
 {
 	struct tcpa_event *event = v;
 	struct tcpa_event temp_event;
@@ -261,18 +261,7 @@ static int tpm_binary_bios_measurements_show(struct seq_file *m, void *v)
 
 }
 
-static int tpm_bios_measurements_release(struct inode *inode,
-					 struct file *file)
-{
-	struct seq_file *seq = (struct seq_file *)file->private_data;
-	struct tpm_chip *chip = (struct tpm_chip *)seq->private;
-
-	put_device(&chip->dev);
-
-	return seq_release(inode, file);
-}
-
-static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
+static int tpm1_ascii_bios_measurements_show(struct seq_file *m, void *v)
 {
 	int len = 0;
 	char *eventname;
@@ -305,172 +294,16 @@ static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-static const struct seq_operations tpm_ascii_b_measurements_seqops = {
-	.start = tpm_bios_measurements_start,
-	.next = tpm_bios_measurements_next,
-	.stop = tpm_bios_measurements_stop,
-	.show = tpm_ascii_bios_measurements_show,
+const struct seq_operations tpm1_ascii_b_measurements_seqops = {
+	.start = tpm1_bios_measurements_start,
+	.next = tpm1_bios_measurements_next,
+	.stop = tpm1_bios_measurements_stop,
+	.show = tpm1_ascii_bios_measurements_show,
 };
 
-static const struct seq_operations tpm_binary_b_measurements_seqops = {
-	.start = tpm_bios_measurements_start,
-	.next = tpm_bios_measurements_next,
-	.stop = tpm_bios_measurements_stop,
-	.show = tpm_binary_bios_measurements_show,
-};
-
-static int tpm_bios_measurements_open(struct inode *inode,
-					    struct file *file)
-{
-	int err;
-	struct seq_file *seq;
-	struct tpm_chip_seqops *chip_seqops;
-	const struct seq_operations *seqops;
-	struct tpm_chip *chip;
-
-	inode_lock(inode);
-	if (!inode->i_private) {
-		inode_unlock(inode);
-		return -ENODEV;
-	}
-	chip_seqops = (struct tpm_chip_seqops *)inode->i_private;
-	seqops = chip_seqops->seqops;
-	chip = chip_seqops->chip;
-	get_device(&chip->dev);
-	inode_unlock(inode);
-
-	/* now register seq file */
-	err = seq_open(file, seqops);
-	if (!err) {
-		seq = file->private_data;
-		seq->private = chip;
-	}
-
-	return err;
-}
-
-static const struct file_operations tpm_bios_measurements_ops = {
-	.owner = THIS_MODULE,
-	.open = tpm_bios_measurements_open,
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = tpm_bios_measurements_release,
+const struct seq_operations tpm1_binary_b_measurements_seqops = {
+	.start = tpm1_bios_measurements_start,
+	.next = tpm1_bios_measurements_next,
+	.stop = tpm1_bios_measurements_stop,
+	.show = tpm1_binary_bios_measurements_show,
 };
-
-static int tpm_read_log(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (chip->log.bios_event_log != NULL) {
-		dev_dbg(&chip->dev,
-			"%s: ERROR - event log already initialized\n",
-			__func__);
-		return -EFAULT;
-	}
-
-	rc = tpm_read_log_acpi(chip);
-	if (rc != -ENODEV)
-		return rc;
-
-	rc = tpm_read_log_efi(chip);
-	if (rc != -ENODEV)
-		return rc;
-
-	return tpm_read_log_of(chip);
-}
-
-/*
- * tpm_bios_log_setup() - Read the event log from the firmware
- * @chip: TPM chip to use.
- *
- * If an event log is found then the securityfs files are setup to
- * export it to userspace, otherwise nothing is done.
- *
- * Returns -ENODEV if the firmware has no event log or securityfs is not
- * supported.
- */
-int tpm_bios_log_setup(struct tpm_chip *chip)
-{
-	const char *name = dev_name(&chip->dev);
-	unsigned int cnt;
-	int log_version;
-	int rc = 0;
-
-	rc = tpm_read_log(chip);
-	if (rc < 0)
-		return rc;
-	log_version = rc;
-
-	cnt = 0;
-	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
-	/* NOTE: securityfs_create_dir can return ENODEV if securityfs is
-	 * compiled out. The caller should ignore the ENODEV return code.
-	 */
-	if (IS_ERR(chip->bios_dir[cnt]))
-		goto err;
-	cnt++;
-
-	chip->bin_log_seqops.chip = chip;
-	if (log_version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
-		chip->bin_log_seqops.seqops =
-			&tpm2_binary_b_measurements_seqops;
-	else
-		chip->bin_log_seqops.seqops =
-			&tpm_binary_b_measurements_seqops;
-
-
-	chip->bios_dir[cnt] =
-	    securityfs_create_file("binary_bios_measurements",
-				   0440, chip->bios_dir[0],
-				   (void *)&chip->bin_log_seqops,
-				   &tpm_bios_measurements_ops);
-	if (IS_ERR(chip->bios_dir[cnt]))
-		goto err;
-	cnt++;
-
-	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
-
-		chip->ascii_log_seqops.chip = chip;
-		chip->ascii_log_seqops.seqops =
-			&tpm_ascii_b_measurements_seqops;
-
-		chip->bios_dir[cnt] =
-			securityfs_create_file("ascii_bios_measurements",
-					       0440, chip->bios_dir[0],
-					       (void *)&chip->ascii_log_seqops,
-					       &tpm_bios_measurements_ops);
-		if (IS_ERR(chip->bios_dir[cnt]))
-			goto err;
-		cnt++;
-	}
-
-	return 0;
-
-err:
-	rc = PTR_ERR(chip->bios_dir[cnt]);
-	chip->bios_dir[cnt] = NULL;
-	tpm_bios_log_teardown(chip);
-	return rc;
-}
-
-void tpm_bios_log_teardown(struct tpm_chip *chip)
-{
-	int i;
-	struct inode *inode;
-
-	/* securityfs_remove currently doesn't take care of handling sync
-	 * between removal and opening of pseudo files. To handle this, a
-	 * workaround is added by making i_private = NULL here during removal
-	 * and to check it during open(), both within inode_lock()/unlock().
-	 * This design ensures that open() either safely gets kref or fails.
-	 */
-	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
-		if (chip->bios_dir[i]) {
-			inode = d_inode(chip->bios_dir[i]);
-			inode_lock(inode);
-			inode->i_private = NULL;
-			inode_unlock(inode);
-			securityfs_remove(chip->bios_dir[i]);
-		}
-	}
-}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f895fba4e20d..a583c5001904 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -580,6 +580,8 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 		      u32 cc, u8 *buf, size_t *bufsiz);
 
+extern const struct seq_operations tpm1_ascii_b_measurements_seqops;
+extern const struct seq_operations tpm1_binary_b_measurements_seqops;
 extern const struct seq_operations tpm2_binary_b_measurements_seqops;
 
 #if defined(CONFIG_ACPI)
-- 
2.17.0.484.g0c8726318c-goog

[PATCH v2 4/4] tpm: Move eventlog declarations to its own header

[Thiebaud Weksteen]

Reduce the size of tpm.h by moving eventlog declarations to a separate
header.

Signed-off-by: Thiebaud Weksteen <tweek@google.com>
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/eventlog/acpi.c   |  1 +
 drivers/char/tpm/eventlog/common.c |  2 +-
 drivers/char/tpm/eventlog/common.h | 35 ++++++++++++++++++++++++++++++
 drivers/char/tpm/eventlog/efi.c    |  1 +
 drivers/char/tpm/eventlog/of.c     |  1 +
 drivers/char/tpm/eventlog/tpm1.c   |  1 +
 drivers/char/tpm/eventlog/tpm2.c   |  1 +
 drivers/char/tpm/tpm.h             | 29 -------------------------
 8 files changed, 41 insertions(+), 30 deletions(-)
 create mode 100644 drivers/char/tpm/eventlog/common.h

diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 8476be2e9526..7c53b1973b62 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -28,6 +28,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 struct acpi_tcpa {
 	struct acpi_table_header hdr;
diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c
index 54934b5a1566..5a8720df2b51 100644
--- a/drivers/char/tpm/eventlog/common.c
+++ b/drivers/char/tpm/eventlog/common.c
@@ -25,7 +25,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
-
+#include "common.h"
 
 static int tpm_bios_measurements_open(struct inode *inode,
 					    struct file *file)
diff --git a/drivers/char/tpm/eventlog/common.h b/drivers/char/tpm/eventlog/common.h
new file mode 100644
index 000000000000..47ff8136ceb5
--- /dev/null
+++ b/drivers/char/tpm/eventlog/common.h
@@ -0,0 +1,35 @@
+#ifndef __TPM_EVENTLOG_COMMON_H__
+#define __TPM_EVENTLOG_COMMON_H__
+
+#include "../tpm.h"
+
+extern const struct seq_operations tpm1_ascii_b_measurements_seqops;
+extern const struct seq_operations tpm1_binary_b_measurements_seqops;
+extern const struct seq_operations tpm2_binary_b_measurements_seqops;
+
+#if defined(CONFIG_ACPI)
+int tpm_read_log_acpi(struct tpm_chip *chip);
+#else
+static inline int tpm_read_log_acpi(struct tpm_chip *chip)
+{
+	return -ENODEV;
+}
+#endif
+#if defined(CONFIG_OF)
+int tpm_read_log_of(struct tpm_chip *chip);
+#else
+static inline int tpm_read_log_of(struct tpm_chip *chip)
+{
+	return -ENODEV;
+}
+#endif
+#if defined(CONFIG_EFI)
+int tpm_read_log_efi(struct tpm_chip *chip);
+#else
+static inline int tpm_read_log_efi(struct tpm_chip *chip)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif
diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index e1593c5271a4..68f1e7ee60ce 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -15,6 +15,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 /* read binary bios log from EFI configuration table */
 int tpm_read_log_efi(struct tpm_chip *chip)
diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index d98c184d5c13..f8ad4c3ffe8e 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -20,6 +20,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 int tpm_read_log_of(struct tpm_chip *chip)
 {
diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c
index 8f30316e9bb6..58c84784ba25 100644
--- a/drivers/char/tpm/eventlog/tpm1.c
+++ b/drivers/char/tpm/eventlog/tpm1.c
@@ -29,6 +29,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 
 static const char* tcpa_event_type_strings[] = {
diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c
index f0723fa9ae14..1b8fa9de2cac 100644
--- a/drivers/char/tpm/eventlog/tpm2.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -24,6 +24,7 @@
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
+#include "common.h"
 
 /*
  * calc_tpm2_event_size() - calculate the event size, where event
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a583c5001904..067f305e36f0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -580,35 +580,6 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 		      u32 cc, u8 *buf, size_t *bufsiz);
 
-extern const struct seq_operations tpm1_ascii_b_measurements_seqops;
-extern const struct seq_operations tpm1_binary_b_measurements_seqops;
-extern const struct seq_operations tpm2_binary_b_measurements_seqops;
-
-#if defined(CONFIG_ACPI)
-int tpm_read_log_acpi(struct tpm_chip *chip);
-#else
-static inline int tpm_read_log_acpi(struct tpm_chip *chip)
-{
-	return -ENODEV;
-}
-#endif
-#if defined(CONFIG_OF)
-int tpm_read_log_of(struct tpm_chip *chip);
-#else
-static inline int tpm_read_log_of(struct tpm_chip *chip)
-{
-	return -ENODEV;
-}
-#endif
-#if defined(CONFIG_EFI)
-int tpm_read_log_efi(struct tpm_chip *chip);
-#else
-static inline int tpm_read_log_efi(struct tpm_chip *chip)
-{
-	return -ENODEV;
-}
-#endif
-
 int tpm_bios_log_setup(struct tpm_chip *chip);
 void tpm_bios_log_teardown(struct tpm_chip *chip);
 #endif
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Jason Gunthorpe]

On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> Signed-off-by: Thiebaud Weksteen <tweek@google.com>
>  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_eventlog_of.c b/drivers/char/tpm/tpm_eventlog_of.c
> index 96fd5646f866..d74568d58a66 100644
> +++ b/drivers/char/tpm/tpm_eventlog_of.c
> @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  	 * but physical tpm needs the conversion.
>  	 */
>  	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
> -		size = be32_to_cpup(sizep);
> -		base = be64_to_cpup(basep);
> +		size = be32_to_cpup((__be32 *)sizep);
> +		base = be64_to_cpup((__be64 *)basep);

Er, no.. change the definitions of sizep and basep to be __be

Jason

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Thiebaud Weksteen]

On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> >  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_eventlog_of.c
b/drivers/char/tpm/tpm_eventlog_of.c
> > index 96fd5646f866..d74568d58a66 100644
> > +++ b/drivers/char/tpm/tpm_eventlog_of.c
> > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> >        * but physical tpm needs the conversion.
> >        */
> >       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
> > -             size = be32_to_cpup(sizep);
> > -             base = be64_to_cpup(basep);
> > +             size = be32_to_cpup((__be32 *)sizep);
> > +             base = be64_to_cpup((__be64 *)basep);

> Er, no.. change the definitions of sizep and basep to be __be

> Jason

Please read the comment before the condition. sizep and
basep may contain either little endian or big endian and this block is used
to adjust that. Let me know if there is a better way for handling this.

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Jason Gunthorpe]

On Tue, Apr 17, 2018 at 08:32:33AM +0000, Thiebaud Weksteen wrote:
> On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > >  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_eventlog_of.c
> b/drivers/char/tpm/tpm_eventlog_of.c
> > > index 96fd5646f866..d74568d58a66 100644
> > > +++ b/drivers/char/tpm/tpm_eventlog_of.c
> > > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > >        * but physical tpm needs the conversion.
> > >        */
> > >       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
> > > -             size = be32_to_cpup(sizep);
> > > -             base = be64_to_cpup(basep);
> > > +             size = be32_to_cpup((__be32 *)sizep);
> > > +             base = be64_to_cpup((__be64 *)basep);
> 
> > Er, no.. change the definitions of sizep and basep to be __be
> 
> > Jason
> 
> Please read the comment before the condition. sizep and
> basep may contain either little endian or big endian and this block is used
> to adjust that. Let me know if there is a better way for handling this.

Well a cast like that will throw sparse warnings, you need __force at
least

Jason

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Thiebaud Weksteen]

On Tue, Apr 17, 2018 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Tue, Apr 17, 2018 at 08:32:33AM +0000, Thiebaud Weksteen wrote:
> > On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> > > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > >  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_eventlog_of.c
> > b/drivers/char/tpm/tpm_eventlog_of.c
> > > > index 96fd5646f866..d74568d58a66 100644
> > > > +++ b/drivers/char/tpm/tpm_eventlog_of.c
> > > > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > > >        * but physical tpm needs the conversion.
> > > >        */
> > > >       if (of_property_match_string(np, "compatible", "IBM,vtpm") <
0) {
> > > > -             size = be32_to_cpup(sizep);
> > > > -             base = be64_to_cpup(basep);
> > > > +             size = be32_to_cpup((__be32 *)sizep);
> > > > +             base = be64_to_cpup((__be64 *)basep);
> >
> > > Er, no.. change the definitions of sizep and basep to be __be
> >
> > > Jason
> >
> > Please read the comment before the condition. sizep and
> > basep may contain either little endian or big endian and this block is
used
> > to adjust that. Let me know if there is a better way for handling this.

> Well a cast like that will throw sparse warnings, you need __force at
> least

> Jason

I don't think so. Since the variable is only defined as u32*, no specific
warning is generated. I've used `make C=2 drivers/char/tpm/` with this
patch applied and no new warning is being triggered.

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Jarkko Sakkinen]

On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> Signed-off-by: Thiebaud Weksteen <tweek@google.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

[Jarkko Sakkinen]

On Thu, Apr 12, 2018 at 12:13:48PM +0200, Thiebaud Weksteen wrote:
> Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v2 3/4] tpm: Move shared eventlog functions to common.c

[Jarkko Sakkinen]

On Thu, Apr 12, 2018 at 12:13:49PM +0200, Thiebaud Weksteen wrote:
> Functions and structures specific to TPM1 are renamed from tpm* to tpm1*.
> 
> Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v2 4/4] tpm: Move eventlog declarations to its own header

[Jarkko Sakkinen]

On Thu, Apr 12, 2018 at 12:13:50PM +0200, Thiebaud Weksteen wrote:
> Reduce the size of tpm.h by moving eventlog declarations to a separate
> header.
> 
> Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Jason Gunthorpe]

On Thu, Apr 19, 2018 at 01:09:12PM +0000, Thiebaud Weksteen wrote:
> On Tue, Apr 17, 2018 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> > On Tue, Apr 17, 2018 at 08:32:33AM +0000, Thiebaud Weksteen wrote:
> > > On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> > > > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > > >  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_eventlog_of.c
> > > b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > index 96fd5646f866..d74568d58a66 100644
> > > > > +++ b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > > > >        * but physical tpm needs the conversion.
> > > > >        */
> > > > >       if (of_property_match_string(np, "compatible", "IBM,vtpm") <
> 0) {
> > > > > -             size = be32_to_cpup(sizep);
> > > > > -             base = be64_to_cpup(basep);
> > > > > +             size = be32_to_cpup((__be32 *)sizep);
> > > > > +             base = be64_to_cpup((__be64 *)basep);
> > >
> > > > Er, no.. change the definitions of sizep and basep to be __be
> > >
> > > > Jason
> > >
> > > Please read the comment before the condition. sizep and
> > > basep may contain either little endian or big endian and this block is
> used
> > > to adjust that. Let me know if there is a better way for handling this.
> 
> > Well a cast like that will throw sparse warnings, you need __force at
> > least
> 
> I don't think so. Since the variable is only defined as u32*, no specific
> warning is generated. I've used `make C=2 drivers/char/tpm/` with this
> patch applied and no new warning is being triggered.

I'm surprised to hear you say that..

Sparse is supposed to require force on all cast that change the
annotation, and there are many examples in the kernel that have force
in that case.

Jason

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Thiebaud Weksteen]

On Fri, Apr 20, 2018 at 4:57 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Thu, Apr 19, 2018 at 01:09:12PM +0000, Thiebaud Weksteen wrote:
> > On Tue, Apr 17, 2018 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > > On Tue, Apr 17, 2018 at 08:32:33AM +0000, Thiebaud Weksteen wrote:
> > > > On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <jgg@ziepe.ca>
wrote:
> > > >
> > > > > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> > > > > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > > > >  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/tpm_eventlog_of.c
> > > > b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > > index 96fd5646f866..d74568d58a66 100644
> > > > > > +++ b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > > > > >        * but physical tpm needs the conversion.
> > > > > >        */
> > > > > >       if (of_property_match_string(np, "compatible",
"IBM,vtpm") <
> > 0) {
> > > > > > -             size = be32_to_cpup(sizep);
> > > > > > -             base = be64_to_cpup(basep);
> > > > > > +             size = be32_to_cpup((__be32 *)sizep);
> > > > > > +             base = be64_to_cpup((__be64 *)basep);
> > > >
> > > > > Er, no.. change the definitions of sizep and basep to be __be
> > > >
> > > > > Jason
> > > >
> > > > Please read the comment before the condition. sizep and
> > > > basep may contain either little endian or big endian and this block
is
> > used
> > > > to adjust that. Let me know if there is a better way for handling
this.
> >
> > > Well a cast like that will throw sparse warnings, you need __force at
> > > least
> >
> > I don't think so. Since the variable is only defined as u32*, no
specific
> > warning is generated. I've used `make C=2 drivers/char/tpm/` with this
> > patch applied and no new warning is being triggered.

> I'm surprised to hear you say that..

> Sparse is supposed to require force on all cast that change the
> annotation, and there are many examples in the kernel that have force
> in that case.

> Jason

+linux-sparse@vger.kernel.org and sparse@chrisli.org for a sanity check.

If you look at the man page of sparse, under the bitwise option, it states:
"Sparse will warn on [...] any conversion of one restricted type into
another, except via a cast that includes __attribute__((force)).". In our
case, it is a conversion from unrestricted to restricted which does not
fall in this category.

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Luc Van Oostenryck]

On Mon, Apr 23, 2018 at 09:22:06AM +0000, Thiebaud Weksteen wrote:
> On Fri, Apr 20, 2018 at 4:57 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> > On Thu, Apr 19, 2018 at 01:09:12PM +0000, Thiebaud Weksteen wrote:
> > > On Tue, Apr 17, 2018 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > > On Tue, Apr 17, 2018 at 08:32:33AM +0000, Thiebaud Weksteen wrote:
> > > > > On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <jgg@ziepe.ca>
> wrote:
> > > > >
> > > > > > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote:
> > > > > > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > > > > >  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/char/tpm/tpm_eventlog_of.c
> > > > > b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > > > index 96fd5646f866..d74568d58a66 100644
> > > > > > > +++ b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > > > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > > > > > >        * but physical tpm needs the conversion.
> > > > > > >        */
> > > > > > >       if (of_property_match_string(np, "compatible",
> "IBM,vtpm") <
> > > 0) {
> > > > > > > -             size = be32_to_cpup(sizep);
> > > > > > > -             base = be64_to_cpup(basep);
> > > > > > > +             size = be32_to_cpup((__be32 *)sizep);
> > > > > > > +             base = be64_to_cpup((__be64 *)basep);
> > > > >
> > > > > > Er, no.. change the definitions of sizep and basep to be __be
> > > > >
> > > > > > Jason
> > > > >
> > > > > Please read the comment before the condition. sizep and
> > > > > basep may contain either little endian or big endian and this block
> is
> > > used
> > > > > to adjust that. Let me know if there is a better way for handling
> this.
> > >
> > > > Well a cast like that will throw sparse warnings, you need __force at
> > > > least
> > >
> > > I don't think so. Since the variable is only defined as u32*, no
> specific
> > > warning is generated. I've used `make C=2 drivers/char/tpm/` with this
> > > patch applied and no new warning is being triggered.

Interesting.

> > I'm surprised to hear you say that..

Same for me.

> > Sparse is supposed to require force on all cast that change the
> > annotation, and there are many examples in the kernel that have force
> > in that case.


Yes, sparse is supposed to warn in such cases and the __force is there
to quiets the warning when it is known that the cast is in fact correct.

 
> +linux-sparse@vger.kernel.org and sparse@chrisli.org for a sanity check.
> 
> If you look at the man page of sparse, under the bitwise option, it states:
> "Sparse will warn on [...] any conversion of one restricted type into
> another, except via a cast that includes __attribute__((force)).". In our
> case, it is a conversion from unrestricted to restricted which does not
> fall in this category.

The man page is not very clear here. It must be understood in the context
where each use of '__bitwise' will create a new *distinct* type.
Given this and the normal type checking, sparse should warn
"on any conversion of one restricted type into another *or* between a
restricted and the corresponding plain/unrestricted type" (or consider
that an 'unrestricted type' is 'a restricted type with no restriction',
which is, I think, what was meant here).

The fact that sparse doesn't warn in your case is clearly a bug in
sparse's type checking.

Regards,
-- Luc Van Oostenryck

Re: [PATCH v2 1/4] tpm: Add explicit endianness cast

[Thiebaud Weksteen]

On Mon, Apr 23, 2018 at 12:06 PM Luc Van Oostenryck <
luc.vanoostenryck@gmail.com> wrote:

> On Mon, Apr 23, 2018 at 09:22:06AM +0000, Thiebaud Weksteen wrote:
> > On Fri, Apr 20, 2018 at 4:57 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > > On Thu, Apr 19, 2018 at 01:09:12PM +0000, Thiebaud Weksteen wrote:
> > > > On Tue, Apr 17, 2018 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca>
wrote:
> > > >
> > > > > On Tue, Apr 17, 2018 at 08:32:33AM +0000, Thiebaud Weksteen wrote:
> > > > > > On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe <jgg@ziepe.ca>
> > wrote:
> > > > > >
> > > > > > > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen
wrote:
> > > > > > > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > > > > > >  drivers/char/tpm/tpm_eventlog_of.c | 4 ++--
> > > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/char/tpm/tpm_eventlog_of.c
> > > > > > b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > > > > index 96fd5646f866..d74568d58a66 100644
> > > > > > > > +++ b/drivers/char/tpm/tpm_eventlog_of.c
> > > > > > > > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
> > > > > > > >        * but physical tpm needs the conversion.
> > > > > > > >        */
> > > > > > > >       if (of_property_match_string(np, "compatible",
> > "IBM,vtpm") <
> > > > 0) {
> > > > > > > > -             size = be32_to_cpup(sizep);
> > > > > > > > -             base = be64_to_cpup(basep);
> > > > > > > > +             size = be32_to_cpup((__be32 *)sizep);
> > > > > > > > +             base = be64_to_cpup((__be64 *)basep);
> > > > > >
> > > > > > > Er, no.. change the definitions of sizep and basep to be __be
> > > > > >
> > > > > > > Jason
> > > > > >
> > > > > > Please read the comment before the condition. sizep and
> > > > > > basep may contain either little endian or big endian and this
block
> > is
> > > > used
> > > > > > to adjust that. Let me know if there is a better way for
handling
> > this.
> > > >
> > > > > Well a cast like that will throw sparse warnings, you need
__force at
> > > > > least
> > > >
> > > > I don't think so. Since the variable is only defined as u32*, no
> > specific
> > > > warning is generated. I've used `make C=2 drivers/char/tpm/` with
this
> > > > patch applied and no new warning is being triggered.

> Interesting.

> > > I'm surprised to hear you say that..

> Same for me.

> > > Sparse is supposed to require force on all cast that change the
> > > annotation, and there are many examples in the kernel that have force
> > > in that case.


> Yes, sparse is supposed to warn in such cases and the __force is there
> to quiets the warning when it is known that the cast is in fact correct.


> > +linux-sparse@vger.kernel.org and sparse@chrisli.org for a sanity check.
> >
> > If you look at the man page of sparse, under the bitwise option, it
states:
> > "Sparse will warn on [...] any conversion of one restricted type into
> > another, except via a cast that includes __attribute__((force)).". In
our
> > case, it is a conversion from unrestricted to restricted which does not
> > fall in this category.

> The man page is not very clear here. It must be understood in the context
> where each use of '__bitwise' will create a new *distinct* type.
> Given this and the normal type checking, sparse should warn
> "on any conversion of one restricted type into another *or* between a
> restricted and the corresponding plain/unrestricted type" (or consider
> that an 'unrestricted type' is 'a restricted type with no restriction',
> which is, I think, what was meant here).


Thanks for the explanation, that make sense. I believe the issue happens
when dealing with restricted pointer types more than regular types. Also,
this is not new and has probably been going on for the last 13 years. For
instance, 81179bb6a54c2c626b4cbcc084ca974bb2d7f2a3 explicitly removed the
__force attribute. I'll send a validation patch for sparse and update this
patch.

> The fact that sparse doesn't warn in your case is clearly a bug in
> sparse's type checking.

> Regards,
> -- Luc Van Oostenryck

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

[Mimi Zohar]

[Cc'ing Petr Vorel and the ltp mailing list]

Hi Jarrko,

On Fri, 2018-04-20 at 08:39 +0300, Jarkko Sakkinen wrote:
> On Thu, Apr 12, 2018 at 12:13:48PM +0200, Thiebaud Weksteen wrote:
> > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

I just noticed this is queued in your next branch.

Petr Vorel has been updating the IMA LTP tests.  One of those IMA LTP
tests includes walking the TPM binary_runtime_measurements in order to
calculate the IMA boot-aggregate.  The IMA boot-aggregate is the first
measurement in the IMA measurement list.

Mimi

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

[Petr Vorel]

Hi Mimi,

> [Cc'ing Petr Vorel and the ltp mailing list]

> Hi Jarrko,

> On Fri, 2018-04-20 at 08:39 +0300, Jarkko Sakkinen wrote:
> > On Thu, Apr 12, 2018 at 12:13:48PM +0200, Thiebaud Weksteen wrote:
> > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

> I just noticed this is queued in your next branch.

> Petr Vorel has been updating the IMA LTP tests.  One of those IMA LTP
> tests includes walking the TPM binary_runtime_measurements in order to
> calculate the IMA boot-aggregate.  The IMA boot-aggregate is the first
> measurement in the IMA measurement list.

Did you meant that this commit ([2]) in linux-tpmdd/next changed location of
binary_runtime_measurements in sysfs?

IMHO these commits ([1], [2], [3]) just put source code into eventlog/ subdirectory:

> Mimi

Kind regards,
Petr

[1] 75d647f5de69 tpm: Move eventlog declarations to its own header
[2] 9b01b5356629 tpm: Move shared eventlog functions to common.c
[3] 0bfb23746052 tpm: Move eventlog files to a subdirectory

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

[Jarkko Sakkinen]

On Mon, May 07, 2018 at 04:36:37PM -0400, Mimi Zohar wrote:
> [Cc'ing Petr Vorel and the ltp mailing list]
> 
> Hi Jarrko,
> 
> On Fri, 2018-04-20 at 08:39 +0300, Jarkko Sakkinen wrote:
> > On Thu, Apr 12, 2018 at 12:13:48PM +0200, Thiebaud Weksteen wrote:
> > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> I just noticed this is queued in your next branch.
> 
> Petr Vorel has been updating the IMA LTP tests.  One of those IMA LTP
> tests includes walking the TPM binary_runtime_measurements in order to
> calculate the IMA boot-aggregate.  The IMA boot-aggregate is the first
> measurement in the IMA measurement list.
> 
> Mimi

Sorry, not following how these connect directly?

/Jarkko

Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory

[Mimi Zohar]

On Wed, 2018-05-09 at 16:51 +0200, Petr Vorel wrote:
> Hi Mimi,
> 
> > [Cc'ing Petr Vorel and the ltp mailing list]
> 
> > Hi Jarrko,
> 
> > On Fri, 2018-04-20 at 08:39 +0300, Jarkko Sakkinen wrote:
> > > On Thu, Apr 12, 2018 at 12:13:48PM +0200, Thiebaud Weksteen wrote:
> > > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> > I just noticed this is queued in your next branch.
> 
> > Petr Vorel has been updating the IMA LTP tests.  One of those IMA LTP
> > tests includes walking the TPM binary_runtime_measurements in order to
> > calculate the IMA boot-aggregate.  The IMA boot-aggregate is the first
> > measurement in the IMA measurement list.
> 
> Did you meant that this commit ([2]) in linux-tpmdd/next changed location of
> binary_runtime_measurements in sysfs?
> 
> IMHO these commits ([1], [2], [3]) just put source code into eventlog/ subdirectory:

I mistakenly thought the eventlog itself was being moved, not the
source code.

thanks,

Mimi

> 
> Kind regards,
> Petr
> 
> [1] 75d647f5de69 tpm: Move eventlog declarations to its own header
> [2] 9b01b5356629 tpm: Move shared eventlog functions to common.c
> [3] 0bfb23746052 tpm: Move eventlog files to a subdirectory
>