Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
@ 2020-06-25  4:38 Jarkko Sakkinen
  2020-06-25 12:41 ` Stefan Berger
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Jarkko Sakkinen @ 2020-06-25  4:38 UTC (permalink / raw)
  To: linux-integrity
  Cc: Jarkko Sakkinen, James Bottomley, Stefan Berger, Peter Huewe,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
	Jerry Snitselaar, Sumit Garg, Alexey Klimov, open list

Re-allocate context and session buffers when needed. Scale them in page
increments so that the reallocation is only seldomly required, and thus
causes minimal stress to the system. Add a static maximum limit of four
pages for buffer sizes.

Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
Tested only for compilation.
v2: TPM2_SPACE_DEFAULT_BUFFER_SIZE
 drivers/char/tpm/tpm2-space.c | 87 ++++++++++++++++++++++++-----------
 include/linux/tpm.h           |  6 ++-
 2 files changed, 64 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 982d341d8837..b8ece01d6afb 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -15,6 +15,9 @@
 #include <asm/unaligned.h>
 #include "tpm.h"
 
+#define TPM2_SPACE_DEFAULT_BUFFER_SIZE	PAGE_SIZE
+#define TPM2_SPACE_MAX_BUFFER_SIZE	(4 * PAGE_SIZE)
+
 enum tpm2_handle_types {
 	TPM2_HT_HMAC_SESSION	= 0x02000000,
 	TPM2_HT_POLICY_SESSION	= 0x03000000,
@@ -40,16 +43,21 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
 
 int tpm2_init_space(struct tpm_space *space)
 {
-	space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	space->context_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
+				     GFP_KERNEL);
 	if (!space->context_buf)
 		return -ENOMEM;
 
-	space->session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	space->session_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
+				     GFP_KERNEL);
 	if (space->session_buf == NULL) {
 		kfree(space->context_buf);
+		space->context_buf = NULL;
 		return -ENOMEM;
 	}
 
+	space->context_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
+	space->session_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
 	return 0;
 }
 
@@ -116,11 +124,13 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	return 0;
 }
 
-static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
-			     unsigned int buf_size, unsigned int *offset)
+static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 **buf,
+			     unsigned int *buf_size, unsigned int *offset)
 {
-	struct tpm_buf tbuf;
+	unsigned int new_buf_size;
 	unsigned int body_size;
+	struct tpm_buf tbuf;
+	void *new_buf;
 	int rc;
 
 	rc = tpm_buf_init(&tbuf, TPM2_ST_NO_SESSIONS, TPM2_CC_CONTEXT_SAVE);
@@ -131,31 +141,48 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 
 	rc = tpm_transmit_cmd(chip, &tbuf, 0, NULL);
 	if (rc < 0) {
-		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
-			 __func__, rc);
-		tpm_buf_destroy(&tbuf);
-		return -EFAULT;
+		rc = -EFAULT;
+		goto err;
 	} else if (tpm2_rc_value(rc) == TPM2_RC_REFERENCE_H0) {
-		tpm_buf_destroy(&tbuf);
-		return -ENOENT;
+		rc = -ENOENT;
+		goto out;
 	} else if (rc) {
-		dev_warn(&chip->dev, "%s: failed with a TPM error 0x%04X\n",
-			 __func__, rc);
-		tpm_buf_destroy(&tbuf);
-		return -EFAULT;
+		rc = -EFAULT;
+		goto err;
 	}
 
 	body_size = tpm_buf_length(&tbuf) - TPM_HEADER_SIZE;
-	if ((*offset + body_size) > buf_size) {
-		dev_warn(&chip->dev, "%s: out of backing storage\n", __func__);
-		tpm_buf_destroy(&tbuf);
-		return -ENOMEM;
+	/* We grow the buffer in page increments. */
+	new_buf_size = PFN_UP(*offset + body_size);
+
+	if (new_buf_size > TPM2_SPACE_MAX_BUFFER_SIZE) {
+		rc = -ENOMEM;
+		goto err;
 	}
 
-	memcpy(&buf[*offset], &tbuf.data[TPM_HEADER_SIZE], body_size);
+	if (new_buf_size > *buf_size) {
+		new_buf = krealloc(*buf, new_buf_size, GFP_KERNEL);
+		if (!new_buf) {
+			rc = -ENOMEM;
+			goto err;
+		}
+
+		*buf = new_buf;
+		*buf_size = new_buf_size;
+	}
+
+	memcpy(*buf + *offset, &tbuf.data[TPM_HEADER_SIZE], body_size);
 	*offset += body_size;
+
+out:
 	tpm_buf_destroy(&tbuf);
-	return 0;
+	return rc;
+
+err:
+	dev_warn(&chip->dev, "%s: ret=%d\n", __func__, rc);
+
+	tpm_buf_destroy(&tbuf);
+	return rc;
 }
 
 void tpm2_flush_space(struct tpm_chip *chip)
@@ -311,8 +338,10 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u8 *cmd,
 	       sizeof(space->context_tbl));
 	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
 	       sizeof(space->session_tbl));
-	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
-	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
+	memcpy(chip->work_space.context_buf, space->context_buf,
+	       space->context_size);
+	memcpy(chip->work_space.session_buf, space->session_buf,
+	       space->session_size);
 
 	rc = tpm2_load_space(chip);
 	if (rc) {
@@ -492,7 +521,8 @@ static int tpm2_save_space(struct tpm_chip *chip)
 			continue;
 
 		rc = tpm2_save_context(chip, space->context_tbl[i],
-				       space->context_buf, PAGE_SIZE,
+				       &space->context_buf,
+				       &space->context_size,
 				       &offset);
 		if (rc == -ENOENT) {
 			space->context_tbl[i] = 0;
@@ -509,7 +539,8 @@ static int tpm2_save_space(struct tpm_chip *chip)
 			continue;
 
 		rc = tpm2_save_context(chip, space->session_tbl[i],
-				       space->session_buf, PAGE_SIZE,
+				       &space->session_buf,
+				       &space->session_size,
 				       &offset);
 
 		if (rc == -ENOENT) {
@@ -557,8 +588,10 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	       sizeof(space->context_tbl));
 	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
 	       sizeof(space->session_tbl));
-	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
-	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
+	memcpy(space->context_buf, chip->work_space.context_buf,
+	       space->context_size);
+	memcpy(space->session_buf, chip->work_space.session_buf,
+	       space->session_size);
 
 	return 0;
 out:
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 03e9b184411b..9ea39e8f7162 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -92,10 +92,12 @@ enum tpm_duration {
 #define TPM_PPI_VERSION_LEN		3
 
 struct tpm_space {
+	u8  *context_buf;
+	u8  *session_buf;
+	u32 context_size;
+	u32 session_size;
 	u32 context_tbl[3];
-	u8 *context_buf;
 	u32 session_tbl[3];
-	u8 *session_buf;
 };
 
 struct tpm_bios_log {
-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25  4:38 [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically Jarkko Sakkinen
@ 2020-06-25 12:41 ` Stefan Berger
  2020-06-25 21:25   ` Jarkko Sakkinen
  2020-06-25 21:28 ` Jerry Snitselaar
  2020-06-25 21:38 ` Stefan Berger
  2 siblings, 1 reply; 10+ messages in thread
From: Stefan Berger @ 2020-06-25 12:41 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: James Bottomley, Peter Huewe, Jason Gunthorpe, Arnd Bergmann,
	Greg Kroah-Hartman, Jerry Snitselaar, Sumit Garg, Alexey Klimov,
	open list

On 6/25/20 12:38 AM, Jarkko Sakkinen wrote:
> Re-allocate context and session buffers when needed. Scale them in page
> increments so that the reallocation is only seldomly required, and thus
> causes minimal stress to the system. Add a static maximum limit of four
> pages for buffer sizes.
>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>


You don't want to try a fixes tag? None of the previous versions of this 
code will work with newer versions of the TPM 2 then...


    Stefan



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25 12:41 ` Stefan Berger
@ 2020-06-25 21:25   ` Jarkko Sakkinen
  2020-06-25 21:27     ` Stefan Berger
  0 siblings, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2020-06-25 21:25 UTC (permalink / raw)
  To: Stefan Berger
  Cc: linux-integrity, James Bottomley, Peter Huewe, Jason Gunthorpe,
	Arnd Bergmann, Greg Kroah-Hartman, Jerry Snitselaar, Sumit Garg,
	Alexey Klimov, open list

On Thu, Jun 25, 2020 at 08:41:18AM -0400, Stefan Berger wrote:
> On 6/25/20 12:38 AM, Jarkko Sakkinen wrote:
> > Re-allocate context and session buffers when needed. Scale them in page
> > increments so that the reallocation is only seldomly required, and thus
> > causes minimal stress to the system. Add a static maximum limit of four
> > pages for buffer sizes.
> > 
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> 
> You don't want to try a fixes tag? None of the previous versions of this
> code will work with newer versions of the TPM 2 then...

It's not a regression.

/Jarkko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25 21:25   ` Jarkko Sakkinen
@ 2020-06-25 21:27     ` Stefan Berger
  2020-06-26 11:48       ` Jarkko Sakkinen
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Berger @ 2020-06-25 21:27 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-integrity, James Bottomley, Peter Huewe, Jason Gunthorpe,
	Arnd Bergmann, Greg Kroah-Hartman, Jerry Snitselaar, Sumit Garg,
	Alexey Klimov, open list

On 6/25/20 5:25 PM, Jarkko Sakkinen wrote:
> On Thu, Jun 25, 2020 at 08:41:18AM -0400, Stefan Berger wrote:
>> On 6/25/20 12:38 AM, Jarkko Sakkinen wrote:
>>> Re-allocate context and session buffers when needed. Scale them in page
>>> increments so that the reallocation is only seldomly required, and thus
>>> causes minimal stress to the system. Add a static maximum limit of four
>>> pages for buffer sizes.
>>>
>>> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
>>> Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>
>> You don't want to try a fixes tag? None of the previous versions of this
>> code will work with newer versions of the TPM 2 then...
> It's not a regression.

Ok, so distros will have to backport it.


     Stefan



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25  4:38 [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically Jarkko Sakkinen
  2020-06-25 12:41 ` Stefan Berger
@ 2020-06-25 21:28 ` Jerry Snitselaar
  2020-06-26 11:49   ` Jarkko Sakkinen
  2020-06-25 21:38 ` Stefan Berger
  2 siblings, 1 reply; 10+ messages in thread
From: Jerry Snitselaar @ 2020-06-25 21:28 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-integrity, James Bottomley, Stefan Berger, Peter Huewe,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Sumit Garg,
	Alexey Klimov, open list

On Thu Jun 25 20, Jarkko Sakkinen wrote:
>Re-allocate context and session buffers when needed. Scale them in page
>increments so that the reallocation is only seldomly required, and thus
>causes minimal stress to the system. Add a static maximum limit of four
>pages for buffer sizes.
>
>Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
>Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>---
>Tested only for compilation.
>v2: TPM2_SPACE_DEFAULT_BUFFER_SIZE
> drivers/char/tpm/tpm2-space.c | 87 ++++++++++++++++++++++++-----------
> include/linux/tpm.h           |  6 ++-
> 2 files changed, 64 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
>index 982d341d8837..b8ece01d6afb 100644
>--- a/drivers/char/tpm/tpm2-space.c
>+++ b/drivers/char/tpm/tpm2-space.c
>@@ -15,6 +15,9 @@
> #include <asm/unaligned.h>
> #include "tpm.h"
>
>+#define TPM2_SPACE_DEFAULT_BUFFER_SIZE	PAGE_SIZE
>+#define TPM2_SPACE_MAX_BUFFER_SIZE	(4 * PAGE_SIZE)
>+
> enum tpm2_handle_types {
> 	TPM2_HT_HMAC_SESSION	= 0x02000000,
> 	TPM2_HT_POLICY_SESSION	= 0x03000000,
>@@ -40,16 +43,21 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
>
> int tpm2_init_space(struct tpm_space *space)
> {
>-	space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>+	space->context_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
>+				     GFP_KERNEL);
> 	if (!space->context_buf)
> 		return -ENOMEM;
>
>-	space->session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>+	space->session_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
>+				     GFP_KERNEL);
> 	if (space->session_buf == NULL) {
> 		kfree(space->context_buf);
>+		space->context_buf = NULL;
> 		return -ENOMEM;
> 	}
>
>+	space->context_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
>+	space->session_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
> 	return 0;
> }
>
>@@ -116,11 +124,13 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
> 	return 0;
> }
>
>-static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>-			     unsigned int buf_size, unsigned int *offset)
>+static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 **buf,
>+			     unsigned int *buf_size, unsigned int *offset)
> {
>-	struct tpm_buf tbuf;
>+	unsigned int new_buf_size;
> 	unsigned int body_size;
>+	struct tpm_buf tbuf;
>+	void *new_buf;
> 	int rc;
>
> 	rc = tpm_buf_init(&tbuf, TPM2_ST_NO_SESSIONS, TPM2_CC_CONTEXT_SAVE);
>@@ -131,31 +141,48 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>
> 	rc = tpm_transmit_cmd(chip, &tbuf, 0, NULL);
> 	if (rc < 0) {
>-		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
>-			 __func__, rc);
>-		tpm_buf_destroy(&tbuf);
>-		return -EFAULT;
>+		rc = -EFAULT;
>+		goto err;
> 	} else if (tpm2_rc_value(rc) == TPM2_RC_REFERENCE_H0) {
>-		tpm_buf_destroy(&tbuf);
>-		return -ENOENT;
>+		rc = -ENOENT;
>+		goto out;
> 	} else if (rc) {
>-		dev_warn(&chip->dev, "%s: failed with a TPM error 0x%04X\n",
>-			 __func__, rc);
>-		tpm_buf_destroy(&tbuf);
>-		return -EFAULT;
>+		rc = -EFAULT;
>+		goto err;
> 	}
>

Would it be worthwhile to still output something here since it is changing
the value of rc returned from tpm_transmit_cmd()? Wondering if it would
be useful for debugging to know what the returned error was. Other than
that question looks good to me pending what is decided on using PAGE_SIZE.

Regards,
Jerry

> 	body_size = tpm_buf_length(&tbuf) - TPM_HEADER_SIZE;
>-	if ((*offset + body_size) > buf_size) {
>-		dev_warn(&chip->dev, "%s: out of backing storage\n", __func__);
>-		tpm_buf_destroy(&tbuf);
>-		return -ENOMEM;
>+	/* We grow the buffer in page increments. */
>+	new_buf_size = PFN_UP(*offset + body_size);
>+
>+	if (new_buf_size > TPM2_SPACE_MAX_BUFFER_SIZE) {
>+		rc = -ENOMEM;
>+		goto err;
> 	}
>
>-	memcpy(&buf[*offset], &tbuf.data[TPM_HEADER_SIZE], body_size);
>+	if (new_buf_size > *buf_size) {
>+		new_buf = krealloc(*buf, new_buf_size, GFP_KERNEL);
>+		if (!new_buf) {
>+			rc = -ENOMEM;
>+			goto err;
>+		}
>+
>+		*buf = new_buf;
>+		*buf_size = new_buf_size;
>+	}
>+
>+	memcpy(*buf + *offset, &tbuf.data[TPM_HEADER_SIZE], body_size);
> 	*offset += body_size;
>+
>+out:
> 	tpm_buf_destroy(&tbuf);
>-	return 0;
>+	return rc;
>+
>+err:
>+	dev_warn(&chip->dev, "%s: ret=%d\n", __func__, rc);
>+
>+	tpm_buf_destroy(&tbuf);
>+	return rc;
> }
>
> void tpm2_flush_space(struct tpm_chip *chip)
>@@ -311,8 +338,10 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u8 *cmd,
> 	       sizeof(space->context_tbl));
> 	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
> 	       sizeof(space->session_tbl));
>-	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
>-	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
>+	memcpy(chip->work_space.context_buf, space->context_buf,
>+	       space->context_size);
>+	memcpy(chip->work_space.session_buf, space->session_buf,
>+	       space->session_size);
>
> 	rc = tpm2_load_space(chip);
> 	if (rc) {
>@@ -492,7 +521,8 @@ static int tpm2_save_space(struct tpm_chip *chip)
> 			continue;
>
> 		rc = tpm2_save_context(chip, space->context_tbl[i],
>-				       space->context_buf, PAGE_SIZE,
>+				       &space->context_buf,
>+				       &space->context_size,
> 				       &offset);
> 		if (rc == -ENOENT) {
> 			space->context_tbl[i] = 0;
>@@ -509,7 +539,8 @@ static int tpm2_save_space(struct tpm_chip *chip)
> 			continue;
>
> 		rc = tpm2_save_context(chip, space->session_tbl[i],
>-				       space->session_buf, PAGE_SIZE,
>+				       &space->session_buf,
>+				       &space->session_size,
> 				       &offset);
>
> 		if (rc == -ENOENT) {
>@@ -557,8 +588,10 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
> 	       sizeof(space->context_tbl));
> 	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
> 	       sizeof(space->session_tbl));
>-	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
>-	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
>+	memcpy(space->context_buf, chip->work_space.context_buf,
>+	       space->context_size);
>+	memcpy(space->session_buf, chip->work_space.session_buf,
>+	       space->session_size);
>
> 	return 0;
> out:
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>index 03e9b184411b..9ea39e8f7162 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -92,10 +92,12 @@ enum tpm_duration {
> #define TPM_PPI_VERSION_LEN		3
>
> struct tpm_space {
>+	u8  *context_buf;
>+	u8  *session_buf;
>+	u32 context_size;
>+	u32 session_size;
> 	u32 context_tbl[3];
>-	u8 *context_buf;
> 	u32 session_tbl[3];
>-	u8 *session_buf;
> };
>
> struct tpm_bios_log {
>-- 
>2.25.1
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25  4:38 [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically Jarkko Sakkinen
  2020-06-25 12:41 ` Stefan Berger
  2020-06-25 21:28 ` Jerry Snitselaar
@ 2020-06-25 21:38 ` Stefan Berger
  2020-06-26 11:50   ` Jarkko Sakkinen
  2 siblings, 1 reply; 10+ messages in thread
From: Stefan Berger @ 2020-06-25 21:38 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: James Bottomley, Peter Huewe, Jason Gunthorpe, Arnd Bergmann,
	Greg Kroah-Hartman, Jerry Snitselaar, Sumit Garg, Alexey Klimov,
	open list

On 6/25/20 12:38 AM, Jarkko Sakkinen wrote:
> Re-allocate context and session buffers when needed. Scale them in page
> increments so that the reallocation is only seldomly required, and thus
> causes minimal stress to the system. Add a static maximum limit of four
> pages for buffer sizes.
>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> Tested only for compilation.
> v2: TPM2_SPACE_DEFAULT_BUFFER_SIZE
>   drivers/char/tpm/tpm2-space.c | 87 ++++++++++++++++++++++++-----------
>   include/linux/tpm.h           |  6 ++-
>   2 files changed, 64 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 982d341d8837..b8ece01d6afb 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -15,6 +15,9 @@
>   #include <asm/unaligned.h>
>   #include "tpm.h"
>   
> +#define TPM2_SPACE_DEFAULT_BUFFER_SIZE	PAGE_SIZE
> +#define TPM2_SPACE_MAX_BUFFER_SIZE	(4 * PAGE_SIZE)
> +
>   enum tpm2_handle_types {
>   	TPM2_HT_HMAC_SESSION	= 0x02000000,
>   	TPM2_HT_POLICY_SESSION	= 0x03000000,
> @@ -557,8 +588,10 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
>   	       sizeof(space->context_tbl));
>   	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
>   	       sizeof(space->session_tbl));
> -	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
> -	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
> +	memcpy(space->context_buf, chip->work_space.context_buf,
> +	       space->context_size);


You have to allocate the max size the in tpm_chip_alloc (tpm-chip.c):

    chip->work_space.context_buf = kzalloc(TPM2_SPACE_MAX_BUFFER_SIZE, 
GFP_KERNEL);


> +	memcpy(space->session_buf, chip->work_space.session_buf,
> +	       space->session_size);
>   


same for this


>   	return 0;
>   out:
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 03e9b184411b..9ea39e8f7162 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -92,10 +92,12 @@ enum tpm_duration {
>   #define TPM_PPI_VERSION_LEN		3
>   
>   struct tpm_space {
> +	u8  *context_buf;
> +	u8  *session_buf;
> +	u32 context_size;
> +	u32 session_size;
>   	u32 context_tbl[3];
> -	u8 *context_buf;
>   	u32 session_tbl[3];
> -	u8 *session_buf;
>   };
>   
>   struct tpm_bios_log {



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25 21:27     ` Stefan Berger
@ 2020-06-26 11:48       ` Jarkko Sakkinen
  2020-06-26 12:16         ` Stefan Berger
  0 siblings, 1 reply; 10+ messages in thread
From: Jarkko Sakkinen @ 2020-06-26 11:48 UTC (permalink / raw)
  To: Stefan Berger
  Cc: linux-integrity, James Bottomley, Peter Huewe, Jason Gunthorpe,
	Arnd Bergmann, Greg Kroah-Hartman, Jerry Snitselaar, Sumit Garg,
	Alexey Klimov, open list

On Thu, Jun 25, 2020 at 05:27:50PM -0400, Stefan Berger wrote:
> On 6/25/20 5:25 PM, Jarkko Sakkinen wrote:
> > On Thu, Jun 25, 2020 at 08:41:18AM -0400, Stefan Berger wrote:
> > > On 6/25/20 12:38 AM, Jarkko Sakkinen wrote:
> > > > Re-allocate context and session buffers when needed. Scale them in page
> > > > increments so that the reallocation is only seldomly required, and thus
> > > > causes minimal stress to the system. Add a static maximum limit of four
> > > > pages for buffer sizes.
> > > > 
> > > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > 
> > > You don't want to try a fixes tag? None of the previous versions of this
> > > code will work with newer versions of the TPM 2 then...
> > It's not a regression.
> 
> Ok, so distros will have to backport it.

Now that you mentioned PPC64 in some other email that would make this a
regression since x86 provides less space for keys than PPC64.

I studied PPC64 a bit and it actually allows max 256 kB page size, which
is too much for us, given that there is no accounting implemented for
TPM spaces (so far, should be done eventually).

So to summarize: 0 the idea would decrease the limit on PPC64 and
increase it on ther arch's.  `

Dynamic scaling is over to top for fixing the issue, which means that I
will just define static size of 16 kB for the buffer. We can reconsider
it if we hit the roof again.

/Jarkko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25 21:28 ` Jerry Snitselaar
@ 2020-06-26 11:49   ` Jarkko Sakkinen
  0 siblings, 0 replies; 10+ messages in thread
From: Jarkko Sakkinen @ 2020-06-26 11:49 UTC (permalink / raw)
  To: linux-integrity, James Bottomley, Stefan Berger, Peter Huewe,
	Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman, Sumit Garg,
	Alexey Klimov, open list

On Thu, Jun 25, 2020 at 02:28:17PM -0700, Jerry Snitselaar wrote:
> On Thu Jun 25 20, Jarkko Sakkinen wrote:
> > Re-allocate context and session buffers when needed. Scale them in page
> > increments so that the reallocation is only seldomly required, and thus
> > causes minimal stress to the system. Add a static maximum limit of four
> > pages for buffer sizes.
> > 
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > Tested only for compilation.
> > v2: TPM2_SPACE_DEFAULT_BUFFER_SIZE
> > drivers/char/tpm/tpm2-space.c | 87 ++++++++++++++++++++++++-----------
> > include/linux/tpm.h           |  6 ++-
> > 2 files changed, 64 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> > index 982d341d8837..b8ece01d6afb 100644
> > --- a/drivers/char/tpm/tpm2-space.c
> > +++ b/drivers/char/tpm/tpm2-space.c
> > @@ -15,6 +15,9 @@
> > #include <asm/unaligned.h>
> > #include "tpm.h"
> > 
> > +#define TPM2_SPACE_DEFAULT_BUFFER_SIZE	PAGE_SIZE
> > +#define TPM2_SPACE_MAX_BUFFER_SIZE	(4 * PAGE_SIZE)
> > +
> > enum tpm2_handle_types {
> > 	TPM2_HT_HMAC_SESSION	= 0x02000000,
> > 	TPM2_HT_POLICY_SESSION	= 0x03000000,
> > @@ -40,16 +43,21 @@ static void tpm2_flush_sessions(struct tpm_chip *chip, struct tpm_space *space)
> > 
> > int tpm2_init_space(struct tpm_space *space)
> > {
> > -	space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	space->context_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
> > +				     GFP_KERNEL);
> > 	if (!space->context_buf)
> > 		return -ENOMEM;
> > 
> > -	space->session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	space->session_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
> > +				     GFP_KERNEL);
> > 	if (space->session_buf == NULL) {
> > 		kfree(space->context_buf);
> > +		space->context_buf = NULL;
> > 		return -ENOMEM;
> > 	}
> > 
> > +	space->context_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
> > +	space->session_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
> > 	return 0;
> > }
> > 
> > @@ -116,11 +124,13 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
> > 	return 0;
> > }
> > 
> > -static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
> > -			     unsigned int buf_size, unsigned int *offset)
> > +static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 **buf,
> > +			     unsigned int *buf_size, unsigned int *offset)
> > {
> > -	struct tpm_buf tbuf;
> > +	unsigned int new_buf_size;
> > 	unsigned int body_size;
> > +	struct tpm_buf tbuf;
> > +	void *new_buf;
> > 	int rc;
> > 
> > 	rc = tpm_buf_init(&tbuf, TPM2_ST_NO_SESSIONS, TPM2_CC_CONTEXT_SAVE);
> > @@ -131,31 +141,48 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
> > 
> > 	rc = tpm_transmit_cmd(chip, &tbuf, 0, NULL);
> > 	if (rc < 0) {
> > -		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
> > -			 __func__, rc);
> > -		tpm_buf_destroy(&tbuf);
> > -		return -EFAULT;
> > +		rc = -EFAULT;
> > +		goto err;
> > 	} else if (tpm2_rc_value(rc) == TPM2_RC_REFERENCE_H0) {
> > -		tpm_buf_destroy(&tbuf);
> > -		return -ENOENT;
> > +		rc = -ENOENT;
> > +		goto out;
> > 	} else if (rc) {
> > -		dev_warn(&chip->dev, "%s: failed with a TPM error 0x%04X\n",
> > -			 __func__, rc);
> > -		tpm_buf_destroy(&tbuf);
> > -		return -EFAULT;
> > +		rc = -EFAULT;
> > +		goto err;
> > 	}
> > 
> 
> Would it be worthwhile to still output something here since it is changing
> the value of rc returned from tpm_transmit_cmd()? Wondering if it would
> be useful for debugging to know what the returned error was. Other than
> that question looks good to me pending what is decided on using PAGE_SIZE.
> 
> Regards,
> Jerry

I'll submit a new version that will just allocate a static buffer of 16
kB. Dynamic scaling is saved for future.

/Jarkko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-25 21:38 ` Stefan Berger
@ 2020-06-26 11:50   ` Jarkko Sakkinen
  0 siblings, 0 replies; 10+ messages in thread
From: Jarkko Sakkinen @ 2020-06-26 11:50 UTC (permalink / raw)
  To: Stefan Berger
  Cc: linux-integrity, James Bottomley, Peter Huewe, Jason Gunthorpe,
	Arnd Bergmann, Greg Kroah-Hartman, Jerry Snitselaar, Sumit Garg,
	Alexey Klimov, open list

On Thu, Jun 25, 2020 at 05:38:03PM -0400, Stefan Berger wrote:
> On 6/25/20 12:38 AM, Jarkko Sakkinen wrote:
> > Re-allocate context and session buffers when needed. Scale them in page
> > increments so that the reallocation is only seldomly required, and thus
> > causes minimal stress to the system. Add a static maximum limit of four
> > pages for buffer sizes.
> > 
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > Tested only for compilation.
> > v2: TPM2_SPACE_DEFAULT_BUFFER_SIZE
> >   drivers/char/tpm/tpm2-space.c | 87 ++++++++++++++++++++++++-----------
> >   include/linux/tpm.h           |  6 ++-
> >   2 files changed, 64 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> > index 982d341d8837..b8ece01d6afb 100644
> > --- a/drivers/char/tpm/tpm2-space.c
> > +++ b/drivers/char/tpm/tpm2-space.c
> > @@ -15,6 +15,9 @@
> >   #include <asm/unaligned.h>
> >   #include "tpm.h"
> > +#define TPM2_SPACE_DEFAULT_BUFFER_SIZE	PAGE_SIZE
> > +#define TPM2_SPACE_MAX_BUFFER_SIZE	(4 * PAGE_SIZE)
> > +
> >   enum tpm2_handle_types {
> >   	TPM2_HT_HMAC_SESSION	= 0x02000000,
> >   	TPM2_HT_POLICY_SESSION	= 0x03000000,
> > @@ -557,8 +588,10 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
> >   	       sizeof(space->context_tbl));
> >   	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
> >   	       sizeof(space->session_tbl));
> > -	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
> > -	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
> > +	memcpy(space->context_buf, chip->work_space.context_buf,
> > +	       space->context_size);
> 
> 
> You have to allocate the max size the in tpm_chip_alloc (tpm-chip.c):
> 
>    chip->work_space.context_buf = kzalloc(TPM2_SPACE_MAX_BUFFER_SIZE,
> GFP_KERNEL);
> 
> 
> > +	memcpy(space->session_buf, chip->work_space.session_buf,
> > +	       space->session_size);
> 
> 
> same for this

That is not true. They should allocated as 4 kB in the dynamic scaling
scheme. The idea is to use krealloc() to increase the buffer size.

/Jarkko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
  2020-06-26 11:48       ` Jarkko Sakkinen
@ 2020-06-26 12:16         ` Stefan Berger
  0 siblings, 0 replies; 10+ messages in thread
From: Stefan Berger @ 2020-06-26 12:16 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-integrity, James Bottomley, Peter Huewe, Jason Gunthorpe,
	Arnd Bergmann, Greg Kroah-Hartman, Jerry Snitselaar, Sumit Garg,
	Alexey Klimov, open list

On 6/26/20 7:48 AM, Jarkko Sakkinen wrote:
> On Thu, Jun 25, 2020 at 05:27:50PM -0400, Stefan Berger wrote:
>> On 6/25/20 5:25 PM, Jarkko Sakkinen wrote:
>>> On Thu, Jun 25, 2020 at 08:41:18AM -0400, Stefan Berger wrote:
>>>> On 6/25/20 12:38 AM, Jarkko Sakkinen wrote:
>>>>> Re-allocate context and session buffers when needed. Scale them in page
>>>>> increments so that the reallocation is only seldomly required, and thus
>>>>> causes minimal stress to the system. Add a static maximum limit of four
>>>>> pages for buffer sizes.
>>>>>
>>>>> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
>>>>> Suggested-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> You don't want to try a fixes tag? None of the previous versions of this
>>>> code will work with newer versions of the TPM 2 then...
>>> It's not a regression.
>> Ok, so distros will have to backport it.
> Now that you mentioned PPC64 in some other email that would make this a
> regression since x86 provides less space for keys than PPC64.
>
> I studied PPC64 a bit and it actually allows max 256 kB page size, which
> is too much for us, given that there is no accounting implemented for
> TPM spaces (so far, should be done eventually).
>
> So to summarize: 0 the idea would decrease the limit on PPC64 and
> increase it on ther arch's.  `
>
> Dynamic scaling is over to top for fixing the issue, which means that I
> will just define static size of 16 kB for the buffer. We can reconsider
> it if we hit the roof again.

16kb is plenty of space for years to come. Maybe just enlarge the buffer 
for the regression and then do dynamic allocation as the final solution 
for the tip. I can try to test compile it on one or two long term stable 
kernels. Hopefully it applies cleanly. Simple test just in case you had 
a setup with a VM and libtpms master:

# echo hi | clevis encrypt tpm2 '{"key":"rsa"}' | clevis decrypt
hi

This only works once patched, gets stuck in the decrypt step otherwise.


    Stefan


>
> /Jarkko



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25  4:38 [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically Jarkko Sakkinen
2020-06-25 12:41 ` Stefan Berger
2020-06-25 21:25   ` Jarkko Sakkinen
2020-06-25 21:27     ` Stefan Berger
2020-06-26 11:48       ` Jarkko Sakkinen
2020-06-26 12:16         ` Stefan Berger
2020-06-25 21:28 ` Jerry Snitselaar
2020-06-26 11:49   ` Jarkko Sakkinen
2020-06-25 21:38 ` Stefan Berger
2020-06-26 11:50   ` Jarkko Sakkinen

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git