diff mbox series

[v3,4/6] tpm: Support TPM2 sized buffers (TPM2B)

Message ID 20231024011531.442587-5-jarkko@kernel.org (mailing list archive)
State New
Headers show
Series Extend struct tpm_buf to support sized buffers (TPM2B) | expand

Commit Message

Jarkko Sakkinen Oct. 24, 2023, 1:15 a.m. UTC 
Add boolean parameters @alloc and @sized to tpm_buf_init():

* If @alloc is set to false, buf->data is assumed to be pre-feeded and
  owned by the caller.
* If @sized is set to true, the buffer represents a sized buffer
  (TPM2B).

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm-buf.c                | 32 ++++++++++++++++-------
 drivers/char/tpm/tpm-sysfs.c              |  2 +-
 drivers/char/tpm/tpm1-cmd.c               | 14 +++++-----
 drivers/char/tpm/tpm2-cmd.c               | 22 ++++++++--------
 drivers/char/tpm/tpm2-space.c             |  4 +--
 drivers/char/tpm/tpm_vtpm_proxy.c         |  2 +-
 include/linux/tpm.h                       |  3 ++-
 security/keys/trusted-keys/trusted_tpm1.c |  4 +--
 security/keys/trusted-keys/trusted_tpm2.c |  6 ++---
 9 files changed, 51 insertions(+), 38 deletions(-)

Comments

Jarkko Sakkinen Nov. 6, 2023, 3:25 a.m. UTC | #1 
On Fri, 2023-10-27 at 08:32 -0400, James Bottomley wrote:
> On Tue, 2023-10-24 at 04:15 +0300, Jarkko Sakkinen wrote:
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -7,22 +7,32 @@
> >  #include <linux/tpm.h>
> >  
> >  /**
> > - * tpm_buf_init() - Initialize from the heap
> > + * tpm_buf_init() - Initialize a TPM buffer
> >   * @buf:       A @tpm_buf
> > + * @sized:     Represent a sized buffer (TPM2B)
> > + * @alloc:     Allocate from the heap
> >   *
> >   * Initialize all structure fields to zero, allocate a page from the
> > heap, and
> >   * zero the bytes that the buffer headers will consume.
> >   *
> >   * Return: 0 or -ENOMEM
> >   */
> > -int tpm_buf_init(struct tpm_buf *buf)
> > +int tpm_buf_init(struct tpm_buf *buf, bool alloc, bool sized)
> 
> I think it creates a phenomenally confusing interface to use multiple
> booleans because, unlike flags, it's not self describing at point of
> use.  The confusion is enormously heightened here by having the doc
> book arguments be the reverse of the actual function prototype (I just
> tripped over this).
> 
> The alloc flag is particularly counter intuitive: if you pass in an
> allocated buffer, you expect to be responsible for freeing it again,
> but that's not how you use it; you really use it like a reset not an
> alloc, which looks odd because you already created a separate
> tpm_buf_reset function which can't be used in this case.
> 
> Why not replace the alloc flags with two reset functions: one for TPM2B
> buffers and one for command buffers?
> 
> James

Or you can make that as internal (__tpm_buf_init()) and add two
wrappers.

BR, Jarkko
Jerry Snitselaar Nov. 7, 2023, 5:20 p.m. UTC | #2 
On Tue, Oct 24, 2023 at 04:15:22AM +0300, Jarkko Sakkinen wrote:
> Add boolean parameters @alloc and @sized to tpm_buf_init():
> 
> * If @alloc is set to false, buf->data is assumed to be pre-feeded and
>   owned by the caller.
> * If @sized is set to true, the buffer represents a sized buffer
>   (TPM2B).
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  drivers/char/tpm/tpm-buf.c                | 32 ++++++++++++++++-------
>  drivers/char/tpm/tpm-sysfs.c              |  2 +-
>  drivers/char/tpm/tpm1-cmd.c               | 14 +++++-----
>  drivers/char/tpm/tpm2-cmd.c               | 22 ++++++++--------
>  drivers/char/tpm/tpm2-space.c             |  4 +--
>  drivers/char/tpm/tpm_vtpm_proxy.c         |  2 +-
>  include/linux/tpm.h                       |  3 ++-
>  security/keys/trusted-keys/trusted_tpm1.c |  4 +--
>  security/keys/trusted-keys/trusted_tpm2.c |  6 ++---
>  9 files changed, 51 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index fa9a4c51157a..f1d92d7e758d 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -7,22 +7,32 @@
>  #include <linux/tpm.h>
>  
>  /**
> - * tpm_buf_init() - Initialize from the heap
> + * tpm_buf_init() - Initialize a TPM buffer
>   * @buf:	A @tpm_buf
> + * @sized:	Represent a sized buffer (TPM2B)
> + * @alloc:	Allocate from the heap
>   *
>   * Initialize all structure fields to zero, allocate a page from the heap, and

Depending on what the decision ends up being on the bools, flags,
separate functions, or wrappers possibly an "if needed" should be
tacked on to the end of "allocate a page from the heap" here.


Flags would be better when coming across calls to the routine in the
code than the bools, but I think switching to wrappers around
a __tpm_buf_init for the different types would be good.


>   * zero the bytes that the buffer headers will consume.
>   *
Jarkko Sakkinen Nov. 15, 2023, 9:24 p.m. UTC | #3 
On Tue Nov 7, 2023 at 7:20 PM EET, Jerry Snitselaar wrote:
> On Tue, Oct 24, 2023 at 04:15:22AM +0300, Jarkko Sakkinen wrote:
> > Add boolean parameters @alloc and @sized to tpm_buf_init():
> > 
> > * If @alloc is set to false, buf->data is assumed to be pre-feeded and
> >   owned by the caller.
> > * If @sized is set to true, the buffer represents a sized buffer
> >   (TPM2B).
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  drivers/char/tpm/tpm-buf.c                | 32 ++++++++++++++++-------
> >  drivers/char/tpm/tpm-sysfs.c              |  2 +-
> >  drivers/char/tpm/tpm1-cmd.c               | 14 +++++-----
> >  drivers/char/tpm/tpm2-cmd.c               | 22 ++++++++--------
> >  drivers/char/tpm/tpm2-space.c             |  4 +--
> >  drivers/char/tpm/tpm_vtpm_proxy.c         |  2 +-
> >  include/linux/tpm.h                       |  3 ++-
> >  security/keys/trusted-keys/trusted_tpm1.c |  4 +--
> >  security/keys/trusted-keys/trusted_tpm2.c |  6 ++---
> >  9 files changed, 51 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index fa9a4c51157a..f1d92d7e758d 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -7,22 +7,32 @@
> >  #include <linux/tpm.h>
> >  
> >  /**
> > - * tpm_buf_init() - Initialize from the heap
> > + * tpm_buf_init() - Initialize a TPM buffer
> >   * @buf:	A @tpm_buf
> > + * @sized:	Represent a sized buffer (TPM2B)
> > + * @alloc:	Allocate from the heap
> >   *
> >   * Initialize all structure fields to zero, allocate a page from the heap, and
>
> Depending on what the decision ends up being on the bools, flags,
> separate functions, or wrappers possibly an "if needed" should be
> tacked on to the end of "allocate a page from the heap" here.
>
>
> Flags would be better when coming across calls to the routine in the
> code than the bools, but I think switching to wrappers around
> a __tpm_buf_init for the different types would be good.

Yeah, I'll bake something based on this discussion.

BR, Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index fa9a4c51157a..f1d92d7e758d 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -7,22 +7,32 @@ 
 #include <linux/tpm.h>
 
 /**
- * tpm_buf_init() - Initialize from the heap
+ * tpm_buf_init() - Initialize a TPM buffer
  * @buf:	A @tpm_buf
+ * @sized:	Represent a sized buffer (TPM2B)
+ * @alloc:	Allocate from the heap
  *
  * Initialize all structure fields to zero, allocate a page from the heap, and
  * zero the bytes that the buffer headers will consume.
  *
  * Return: 0 or -ENOMEM
  */
-int tpm_buf_init(struct tpm_buf *buf)
+int tpm_buf_init(struct tpm_buf *buf, bool alloc, bool sized)
 {
-	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
-	if (!buf->data)
-		return -ENOMEM;
+	if (alloc) {
+		buf->data = (u8 *)__get_free_page(GFP_KERNEL);
+		if (!buf->data)
+			return -ENOMEM;
+	}
+
+	if (sized) {
+		buf->flags = TPM_BUF_SIZED;
+		buf->length = 2;
+	} else {
+		buf->flags = 0;
+		buf->length = TPM_HEADER_SIZE;
+	}
 
-	buf->flags = 0;
-	buf->length = TPM_HEADER_SIZE;
 	memset(buf->data, 0, TPM_HEADER_SIZE);
 
 	return 0;
@@ -73,8 +83,6 @@  EXPORT_SYMBOL_GPL(tpm_buf_length);
 
 void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
 {
-	struct tpm_header *head = (struct tpm_header *)buf->data;
-
 	/* Return silently if overflow has already happened. */
 	if (buf->flags & TPM_BUF_OVERFLOW)
 		return;
@@ -87,7 +95,11 @@  void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
 
 	memcpy(&buf->data[buf->length], new_data, new_length);
 	buf->length += new_length;
-	head->length = cpu_to_be32(buf->length);
+
+	if (buf->flags & TPM_BUF_SIZED)
+		((__be16 *)buf->data)[0] = cpu_to_be16(buf->length - 2);
+	else
+		((struct tpm_header *)buf->data)->length = cpu_to_be32(buf->length);
 }
 EXPORT_SYMBOL_GPL(tpm_buf_append);
 
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index 557e7f86d98d..047a7064039e 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -44,7 +44,7 @@  static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
 	if (tpm_try_get_ops(chip))
 		return 0;
 
-	if (tpm_buf_init(&tpm_buf))
+	if (tpm_buf_init(&tpm_buf, true, false))
 		goto out_ops;
 
 	tpm_buf_reset(&tpm_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 28fea4e2daaf..396694018590 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -328,7 +328,7 @@  static int tpm1_startup(struct tpm_chip *chip)
 
 	dev_info(&chip->dev, "starting up the TPM manually\n");
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc < 0)
 		return rc;
 
@@ -467,7 +467,7 @@  int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -487,7 +487,7 @@  ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -541,7 +541,7 @@  int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	u32 recd;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -592,7 +592,7 @@  int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -629,7 +629,7 @@  static int tpm1_continue_selftest(struct tpm_chip *chip)
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -761,7 +761,7 @@  int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
 				     "extending dummy pcr before suspend");
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 94dacbf74e0d..1d3e0833641d 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -183,7 +183,7 @@  int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		expected_digest_size = chip->allocated_banks[i].digest_size;
 	}
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -242,7 +242,7 @@  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	int rc;
 	int i;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -302,7 +302,7 @@  int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	if (!num_bytes || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
-	err = tpm_buf_init(&buf);
+	err = tpm_buf_init(&buf, true, false);
 	if (err)
 		return err;
 
@@ -353,7 +353,7 @@  void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc) {
 		dev_warn(&chip->dev, "0x%08x was not flushed, out of memory\n",
 			 handle);
@@ -394,7 +394,7 @@  ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -437,7 +437,7 @@  void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return;
 
@@ -467,7 +467,7 @@  static int tpm2_do_selftest(struct tpm_chip *chip)
 	int rc;
 
 	for (full = 0; full < 2; full++) {
-		rc = tpm_buf_init(&buf);
+		rc = tpm_buf_init(&buf, true, false);
 		if (rc)
 			return rc;
 
@@ -504,7 +504,7 @@  int tpm2_probe(struct tpm_chip *chip)
 	struct tpm_buf buf;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -571,7 +571,7 @@  ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	int rc;
 	int i = 0;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -661,7 +661,7 @@  int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
 		goto out;
 	}
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		goto out;
 
@@ -724,7 +724,7 @@  static int tpm2_startup(struct tpm_chip *chip)
 
 	dev_info(&chip->dev, "starting up the TPM manually\n");
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc < 0)
 		return rc;
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 43584b4176d6..ab2a863baffb 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -76,7 +76,7 @@  static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	unsigned int body_size;
 	int rc;
 
-	rc = tpm_buf_init(&tbuf);
+	rc = tpm_buf_init(&tbuf, true, false);
 	if (rc)
 		return rc;
 
@@ -128,7 +128,7 @@  static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	unsigned int body_size;
 	int rc;
 
-	rc = tpm_buf_init(&tbuf);
+	rc = tpm_buf_init(&tbuf, true, false);
 	if (rc)
 		return rc;
 
diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
index b2b9a15a4a59..52325abd395d 100644
--- a/drivers/char/tpm/tpm_vtpm_proxy.c
+++ b/drivers/char/tpm/tpm_vtpm_proxy.c
@@ -399,7 +399,7 @@  static int vtpm_proxy_request_locality(struct tpm_chip *chip, int locality)
 	const struct tpm_header *header;
 	struct proxy_dev *proxy_dev = dev_get_drvdata(&chip->dev);
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index c355597351c6..687b5173bdab 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -304,6 +304,7 @@  struct tpm_header {
 
 enum tpm_buf_flags {
 	TPM_BUF_OVERFLOW	= BIT(0),
+	TPM_BUF_SIZED		= BIT(1),
 };
 
 struct tpm_buf {
@@ -328,7 +329,7 @@  struct tpm2_hash {
 };
 
 
-int tpm_buf_init(struct tpm_buf *buf);
+int tpm_buf_init(struct tpm_buf *buf, bool alloc, bool sized);
 void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
 void tpm_buf_destroy(struct tpm_buf *buf);
 u32 tpm_buf_length(struct tpm_buf *buf);
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index fcf0eef79ba0..ed5dc3b45d52 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -664,7 +664,7 @@  static int key_seal(struct trusted_key_payload *p,
 	struct tpm_buf tb;
 	int ret;
 
-	ret = tpm_buf_init(&tb);
+	ret = tpm_buf_init(&tb, true, false);
 	if (ret)
 		return ret;
 
@@ -690,7 +690,7 @@  static int key_unseal(struct trusted_key_payload *p,
 	struct tpm_buf tb;
 	int ret;
 
-	ret = tpm_buf_init(&tb);
+	ret = tpm_buf_init(&tb, true, false);
 	if (ret)
 		return ret;
 
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index c54659d06dcb..c41f30770138 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -252,7 +252,7 @@  int tpm2_seal_trusted(struct tpm_chip *chip,
 	if (rc)
 		return rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc) {
 		tpm_put_ops(chip);
 		return rc;
@@ -410,7 +410,7 @@  static int tpm2_load_cmd(struct tpm_chip *chip,
 	if (blob_len > payload->blob_len)
 		return -E2BIG;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;
 
@@ -467,7 +467,7 @@  static int tpm2_unseal_cmd(struct tpm_chip *chip,
 	u8 *data;
 	int rc;
 
-	rc = tpm_buf_init(&buf);
+	rc = tpm_buf_init(&buf, true, false);
 	if (rc)
 		return rc;