linux-kernel - Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM spaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Jan 2017 17:17:23 -0800
From:   James Bottomley <jejb@...ux.vnet.ibm.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        tpmdd-devel@...ts.sourceforge.net
Cc:     open list <linux-kernel@...r.kernel.org>,
        linux-security-module@...r.kernel.org
Subject: Re: [tpmdd-devel] [PATCH RFC v2 3/5] tpm: infrastructure for TPM
 spaces

On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct device
> *pdev,
>  	chip->cdev.owner = THIS_MODULE;
>  	chip->cdev.kobj.parent = &chip->dev.kobj;
> 
> +	chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> GFP_KERNEL);
> +	if (!chip->work_space.context_buf) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +

I think the work_buf handling can be greatly simplified by making it a
pointer to the space: it's only usable between tpm2_prepare_space() and
tpm2_commit_space() which are protected by the chip mutex, so there's
no need for it to exist outside of these calls (i.e. it can be NULL).

Doing it this way also saves the allocation and copying overhead of
work_space.

The patch below can be folded to effect this.

James

---

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 13cac09..770a8c0 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -131,7 +131,6 @@ static void tpm_dev_release(struct device *dev)
 	mutex_unlock(&idr_lock);
 
 	kfree(chip->log.bios_event_log);
-	kfree(chip->work_space.context_buf);
 	kfree(chip);
 }
 
@@ -206,12 +205,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	chip->cdev.kobj.parent = &chip->dev.kobj;
 	chip->cdevrm.kobj.parent = &chip->devrm.kobj;
 
-	chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!chip->work_space.context_buf) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
 	return chip;
 
 out:
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8009ed4..adf7810 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -211,7 +211,7 @@ struct tpm_chip {
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
 #endif /* CONFIG_ACPI */
 
-	struct tpm_space work_space;
+	struct tpm_space *work_space;
 	u32 nr_commands;
 	u32 *cc_attrs_tbl;
 };
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 44e5501..285361e 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -27,7 +27,7 @@ enum tpm2_handle_types {
 
 static void tpm2_flush_space(struct tpm_chip *chip)
 {
-	struct tpm_space *space = &chip->work_space;
+	struct tpm_space *space = chip->work_space;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
@@ -45,7 +45,7 @@ struct tpm2_context {
 
 static int tpm2_load_space(struct tpm_chip *chip)
 {
-	struct tpm_space *space = &chip->work_space;
+	struct tpm_space *space = chip->work_space;
 	struct tpm2_context *ctx;
 	struct tpm_buf buf;
 	int i;
@@ -99,7 +99,7 @@ static int tpm2_load_space(struct tpm_chip *chip)
 
 static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t len)
 {
-	struct tpm_space *space = &chip->work_space;
+	struct tpm_space *space = chip->work_space;
 	unsigned int nr_handles;
 	u32 vhandle;
 	u32 phandle;
@@ -147,9 +147,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
 	if (!space)
 		return 0;
 
-	memcpy(&chip->work_space.context_tbl, &space->context_tbl,
-	       sizeof(space->context_tbl));
-	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
+	chip->work_space = space;
 
 	rc = tpm2_load_space(chip);
 	if (rc)
@@ -164,7 +162,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
 
 static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 {
-	struct tpm_space *space = &chip->work_space;
+	struct tpm_space *space = chip->work_space;
 	u32 phandle;
 	u32 vhandle;
 	u32 attrs;
@@ -222,7 +220,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 
 static int tpm2_save_space(struct tpm_chip *chip)
 {
-	struct tpm_space *space = &chip->work_space;
+	struct tpm_space *space = chip->work_space;
 	struct tpm_buf buf;
 	int i;
 	int j;
@@ -295,9 +293,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	if (rc)
 		return rc;
 
-	memcpy(&space->context_tbl, &chip->work_space.context_tbl,
-	       sizeof(space->context_tbl));
-	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
+	chip->work_space = NULL;
 
 	return 0;
 }