2023-11-24 02:03:01

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 0/8] Extend struct tpm_buf to support sized buffers (TPM2B)

This patch set extends struct tpm_buf to support TPM2 sized buffers, and
adds reader functions for parsing more complex response data. It is
implemented to support smooth landing of [2]. Sealing of the TPM2 trusted
keys is updated to utilize the new functionality, and thus provides a
legit test case for it.

TPM2 sized buffer, i.e. the buffers in TPM2 format, are defined in the
section 10.4 of the TPM2 Structures [1] specification.

Here's the smoke test that I've run for TPM2:

/usr/lib/kselftests/run_kselftest.sh
tpm2_createprimary --hierarchy o -G rsa2048 -c key.ctxt
tpm2_evictcontrol -c key.ctxt 0x81000001
keyctl add trusted kmk "new 32 keyhandle=0x81000001" @u
keyctl add encrypted 1000100010001000 "new ecryptfs trusted:kmk 64" @u

[1] https://trustedcomputinggroup.org/resource/tpm-library-specification/
[2] https://lore.kernel.org/linux-integrity/[email protected]/

v6:
- 7/8: fixed off-by-one error in the boundary check
v5:
- Fixed glitch in tpm_buf_read() reported by James Bottomley to the v4.
Was forgotten from v4.
- Remove a spurious memset() call introduced in v4.
- Allow command buffer tag to be initially set to zero (caused spurious
warnings).
v4:
- Cleaned up the bit too spread code changes based on the v3 review.
- For testing instructions see the previous cover letter, and use
linux-v6.6.y branch:
https://lore.kernel.org/linux-integrity/[email protected]/
v3:
- Resend with rebase to the latest upstream.

Cc: James Bottomley <[email protected]>
Cc: William Roberts <[email protected]>
Cc: Stefan Berger <[email protected]>
Cc: David Howells <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Mimi Zohar <[email protected]>
Cc: Mario Limonciello <[email protected]>
Cc: Jerry Snitselaar <[email protected]>

James Bottomley (1):
tpm: Move buffer handling from static inlines to real functions

Jarkko Sakkinen (7):
tpm: Remove unused tpm_buf_tag()
tpm: Remove tpm_send()
tpm: Update struct tpm_buf documentation comments
tpm: Store the length of the tpm_buf data separately.
tpm: TPM2B formatted buffers
tpm: Add tpm_buf_read_{u8,u16,u32}
KEYS: trusted: tpm2: Use struct tpm_buf for sized buffers

drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-buf.c | 222 ++++++++++++++++++++++
drivers/char/tpm/tpm-interface.c | 26 +--
include/keys/trusted_tpm.h | 2 -
include/linux/tpm.h | 112 +++--------
security/keys/trusted-keys/trusted_tpm1.c | 23 ++-
security/keys/trusted-keys/trusted_tpm2.c | 54 +++---
7 files changed, 295 insertions(+), 145 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c

--
2.43.0


2023-11-24 02:03:19

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 1/8] tpm: Remove unused tpm_buf_tag()

The helper function has no call sites. Thus, remove it.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v1 [2023-11-21]: A new patch.
---
include/linux/tpm.h | 7 -------
1 file changed, 7 deletions(-)

diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4ee9d13749ad..6588ca87cf93 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -358,13 +358,6 @@ static inline u32 tpm_buf_length(struct tpm_buf *buf)
return be32_to_cpu(head->length);
}

-static inline u16 tpm_buf_tag(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be16_to_cpu(head->tag);
-}
-
static inline void tpm_buf_append(struct tpm_buf *buf,
const unsigned char *new_data,
unsigned int new_len)
--
2.43.0

2023-11-24 02:03:31

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 4/8] tpm: Update struct tpm_buf documentation comments

Remove deprecated portions and document enum values.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v1 [2023-11-21]: A new patch.
v2 [2023-11-24]: Refined the commit message a bit.
---
include/linux/tpm.h | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index bb0e8718a432..0a8c1351adc2 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -297,15 +297,14 @@ struct tpm_header {
};
} __packed;

-/* A string buffer type for constructing TPM commands. This is based on the
- * ideas of string buffer code in security/keys/trusted.h but is heap based
- * in order to keep the stack usage minimal.
- */
-
enum tpm_buf_flags {
+ /* the capacity exceeded: */
TPM_BUF_OVERFLOW = BIT(0),
};

+/*
+ * A string buffer type for constructing TPM commands.
+ */
struct tpm_buf {
unsigned int flags;
u8 *data;
--
2.43.0

2023-11-24 02:03:37

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 5/8] tpm: Store the length of the tpm_buf data separately.

TPM2B buffers, or sized buffers, have a two byte header, which contains the
length of the payload as a 16-bit big-endian number, without counting in
the space taken by the header. This differs from encoding in the TPM header
where the length includes also the bytes taken by the header.

Unbound the length of a tpm_buf from the value stored to the TPM command
header. A separate encoding and decoding step so that different buffer
types can be supported, with variant header format and length encoding.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v3 [2023-11-21]: Removed spurious memset() (albeit not harmful). Expand
tag invariant in tpm_buf_reset() to be allowed to be zero.
v2 [2023-11-21]: Squashed together with the following patch, as the API
of tpm_buf_init() is no longer changed.
---
drivers/char/tpm/tpm-buf.c | 48 +++++++++++++++++------
drivers/char/tpm/tpm-interface.c | 1 +
include/keys/trusted_tpm.h | 2 -
include/linux/tpm.h | 6 +--
security/keys/trusted-keys/trusted_tpm1.c | 9 +++--
5 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 96cee41d5b9c..3f39893f3bb1 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -3,25 +3,44 @@
* Handling of TPM command and other buffers.
*/

+#include <linux/tpm_command.h>
#include <linux/module.h>
#include <linux/tpm.h>

+/**
+ * tpm_buf_init() - Allocate and initialize a TPM command
+ * @buf: A &tpm_buf
+ * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
+ * @ordinal: A command ordinal
+ *
+ * Return: 0 or -ENOMEM
+ */
int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
{
buf->data = (u8 *)__get_free_page(GFP_KERNEL);
if (!buf->data)
return -ENOMEM;

- buf->flags = 0;
tpm_buf_reset(buf, tag, ordinal);
return 0;
}
EXPORT_SYMBOL_GPL(tpm_buf_init);

+/**
+ * tpm_buf_reset() - Initialize a TPM command
+ * @buf: A &tpm_buf
+ * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
+ * @ordinal: A command ordinal
+ */
void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
{
struct tpm_header *head = (struct tpm_header *)buf->data;

+ WARN_ON(tag != TPM_TAG_RQU_COMMAND && tag != TPM2_ST_NO_SESSIONS &&
+ tag != TPM2_ST_SESSIONS && tag != 0);
+
+ buf->flags = 0;
+ buf->length = sizeof(*head);
head->tag = cpu_to_be16(tag);
head->length = cpu_to_be32(sizeof(*head));
head->ordinal = cpu_to_be32(ordinal);
@@ -34,33 +53,40 @@ void tpm_buf_destroy(struct tpm_buf *buf)
}
EXPORT_SYMBOL_GPL(tpm_buf_destroy);

+/**
+ * tpm_buf_length() - Return the number of bytes consumed by the data
+ *
+ * Return: The number of bytes consumed by the buffer
+ */
u32 tpm_buf_length(struct tpm_buf *buf)
{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be32_to_cpu(head->length);
+ return buf->length;
}
EXPORT_SYMBOL_GPL(tpm_buf_length);

-void tpm_buf_append(struct tpm_buf *buf,
- const unsigned char *new_data,
- unsigned int new_len)
+/**
+ * tpm_buf_append() - Append data to an initialized buffer
+ * @buf: A &tpm_buf
+ * @new_data: A data blob
+ * @new_length: Size of the appended data
+ */
+void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
{
struct tpm_header *head = (struct tpm_header *)buf->data;
- u32 len = tpm_buf_length(buf);

/* Return silently if overflow has already happened. */
if (buf->flags & TPM_BUF_OVERFLOW)
return;

- if ((len + new_len) > PAGE_SIZE) {
+ if ((buf->length + new_length) > PAGE_SIZE) {
WARN(1, "tpm_buf: overflow\n");
buf->flags |= TPM_BUF_OVERFLOW;
return;
}

- memcpy(&buf->data[len], new_data, new_len);
- head->length = cpu_to_be32(len + new_len);
+ memcpy(&buf->data[buf->length], new_data, new_length);
+ buf->length += new_length;
+ head->length = cpu_to_be32(buf->length);
}
EXPORT_SYMBOL_GPL(tpm_buf_append);

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 163ae247bff2..ea75f2776c2f 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -232,6 +232,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
if (len < min_rsp_body_length + TPM_HEADER_SIZE)
return -EFAULT;

+ buf->length = len;
return 0;
}
EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h
index 7769b726863a..a088b33fd0e3 100644
--- a/include/keys/trusted_tpm.h
+++ b/include/keys/trusted_tpm.h
@@ -6,8 +6,6 @@
#include <linux/tpm_command.h>

/* implementation specific TPM constants */
-#define MAX_BUF_SIZE 1024
-#define TPM_GETRANDOM_SIZE 14
#define TPM_SIZE_OFFSET 2
#define TPM_RETURN_OFFSET 6
#define TPM_DATA_OFFSET 10
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 0a8c1351adc2..1d7b39b5c383 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -306,7 +306,8 @@ enum tpm_buf_flags {
* A string buffer type for constructing TPM commands.
*/
struct tpm_buf {
- unsigned int flags;
+ u32 flags;
+ u32 length;
u8 *data;
};

@@ -329,8 +330,7 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
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);
-void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
- unsigned int new_len);
+void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length);
void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index 37bce84eef99..89c9798d1800 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -367,6 +367,7 @@ int trusted_tpm_send(unsigned char *cmd, size_t buflen)
return rc;

buf.flags = 0;
+ buf.length = buflen;
buf.data = cmd;
dump_tpm_buf(cmd);
rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
@@ -417,7 +418,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
tpm_buf_append_u32(tb, handle);
tpm_buf_append(tb, ononce, TPM_NONCE_SIZE);

- ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, tb->length);
if (ret < 0)
return ret;

@@ -441,7 +442,7 @@ int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
return -ENODEV;

tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_OIAP);
- ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, tb->length);
if (ret < 0)
return ret;

@@ -553,7 +554,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
tpm_buf_append_u8(tb, cont);
tpm_buf_append(tb, td->pubauth, SHA1_DIGEST_SIZE);

- ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, tb->length);
if (ret < 0)
goto out;

@@ -644,7 +645,7 @@ static int tpm_unseal(struct tpm_buf *tb,
tpm_buf_append_u8(tb, cont);
tpm_buf_append(tb, authdata2, SHA1_DIGEST_SIZE);

- ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
+ ret = trusted_tpm_send(tb->data, tb->length);
if (ret < 0) {
pr_info("authhmac failed (%d)\n", ret);
return ret;
--
2.43.0

2023-11-24 02:03:40

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 3/8] tpm: Move buffer handling from static inlines to real functions

From: James Bottomley <[email protected]>

separate out the tpm_buf_... handling functions from static inlines in
tpm.h and move them to their own tpm-buf.c file. This is a precursor
to adding new functions for other TPM type handling because the amount
of code will grow from the current 70 lines in tpm.h to about 200
lines when the additions are done. 200 lines of inline functions is a
bit too much to keep in a header file.

Signed-off-by: James Bottomley <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v3: make tpm_buf_tag static
v4: remove space after spdx tag
v5: fix checkpatch.pl --strict issues
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-buf.c | 87 ++++++++++++++++++++++++++++++++++++++
include/linux/tpm.h | 80 ++++-------------------------------
3 files changed, 97 insertions(+), 71 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 0222b1ddb310..ad3594e383e1 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o
tpm-y += eventlog/common.o
tpm-y += eventlog/tpm1.o
tpm-y += eventlog/tpm2.o
+tpm-y += tpm-buf.o

tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
tpm-$(CONFIG_EFI) += eventlog/efi.o
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
new file mode 100644
index 000000000000..96cee41d5b9c
--- /dev/null
+++ b/drivers/char/tpm/tpm-buf.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Handling of TPM command and other buffers.
+ */
+
+#include <linux/module.h>
+#include <linux/tpm.h>
+
+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ buf->data = (u8 *)__get_free_page(GFP_KERNEL);
+ if (!buf->data)
+ return -ENOMEM;
+
+ buf->flags = 0;
+ tpm_buf_reset(buf, tag, ordinal);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_init);
+
+void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ head->tag = cpu_to_be16(tag);
+ head->length = cpu_to_be32(sizeof(*head));
+ head->ordinal = cpu_to_be32(ordinal);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_reset);
+
+void tpm_buf_destroy(struct tpm_buf *buf)
+{
+ free_page((unsigned long)buf->data);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_destroy);
+
+u32 tpm_buf_length(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ return be32_to_cpu(head->length);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_length);
+
+void tpm_buf_append(struct tpm_buf *buf,
+ const unsigned char *new_data,
+ unsigned int new_len)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+ u32 len = tpm_buf_length(buf);
+
+ /* Return silently if overflow has already happened. */
+ if (buf->flags & TPM_BUF_OVERFLOW)
+ return;
+
+ if ((len + new_len) > PAGE_SIZE) {
+ WARN(1, "tpm_buf: overflow\n");
+ buf->flags |= TPM_BUF_OVERFLOW;
+ return;
+ }
+
+ memcpy(&buf->data[len], new_data, new_len);
+ head->length = cpu_to_be32(len + new_len);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append);
+
+void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
+{
+ tpm_buf_append(buf, &value, 1);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u8);
+
+void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
+{
+ __be16 value2 = cpu_to_be16(value);
+
+ tpm_buf_append(buf, (u8 *)&value2, 2);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
+
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
+ tpm_buf_append(buf, (u8 *)&value2, 4);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index d9d645e9c52c..bb0e8718a432 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -326,77 +326,15 @@ struct tpm2_hash {
unsigned int tpm_id;
};

-static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- head->tag = cpu_to_be16(tag);
- head->length = cpu_to_be32(sizeof(*head));
- head->ordinal = cpu_to_be32(ordinal);
-}
-
-static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- buf->data = (u8 *)__get_free_page(GFP_KERNEL);
- if (!buf->data)
- return -ENOMEM;
-
- buf->flags = 0;
- tpm_buf_reset(buf, tag, ordinal);
- return 0;
-}
-
-static inline void tpm_buf_destroy(struct tpm_buf *buf)
-{
- free_page((unsigned long)buf->data);
-}
-
-static inline u32 tpm_buf_length(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be32_to_cpu(head->length);
-}
-
-static inline void tpm_buf_append(struct tpm_buf *buf,
- const unsigned char *new_data,
- unsigned int new_len)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
- u32 len = tpm_buf_length(buf);
-
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_OVERFLOW)
- return;
-
- if ((len + new_len) > PAGE_SIZE) {
- WARN(1, "tpm_buf: overflow\n");
- buf->flags |= TPM_BUF_OVERFLOW;
- return;
- }
-
- memcpy(&buf->data[len], new_data, new_len);
- head->length = cpu_to_be32(len + new_len);
-}
-
-static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
-{
- tpm_buf_append(buf, &value, 1);
-}
-
-static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
-{
- __be16 value2 = cpu_to_be16(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 2);
-}
-
-static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
-{
- __be32 value2 = cpu_to_be32(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 4);
-}
+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
+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);
+void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
+ unsigned int new_len);
+void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
+void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);

/*
* Check if TPM device is in the firmware upgrade mode.
--
2.43.0

2023-11-24 02:03:52

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 7/8] tpm: Add tpm_buf_read_{u8,u16,u32}

Declare reader functions for the instances of struct tpm_buf. If the read
goes out of boundary, TPM_BUF_BOUNDARY_ERROR is set, and subsequent read
will do nothing.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v5 [2023-11-24]: Fixed off-by-one error in the boundary check.
v4 [2023-11-21]: Address James Bottomley's feedback for v2 of this
patch, i.e. offset pointer was not correctly dereferenced.
v3 [2023-11-21]: Add possibility to check for boundary error to the
as response to the feedback from Mario Limenciello:
https://lore.kernel.org/linux-integrity/[email protected]/
---
drivers/char/tpm/tpm-buf.c | 79 +++++++++++++++++++++++++++++++++++++-
include/linux/tpm.h | 5 +++
2 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 099b4a56c5d5..32619e9ab4fa 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -107,7 +107,7 @@ void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
return;

if ((buf->length + new_length) > PAGE_SIZE) {
- WARN(1, "tpm_buf: overflow\n");
+ WARN(1, "tpm_buf: write overflow\n");
buf->flags |= TPM_BUF_OVERFLOW;
return;
}
@@ -143,3 +143,80 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
tpm_buf_append(buf, (u8 *)&value2, 4);
}
EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
+
+/**
+ * tpm_buf_read() - Read from a TPM buffer
+ * @buf: &tpm_buf instance
+ * @offset: offset within the buffer
+ * @count: the number of bytes to read
+ * @output: the output buffer
+ */
+static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void *output)
+{
+ off_t next_offset;
+
+ /* Return silently if overflow has already happened. */
+ if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
+ return;
+
+ next_offset = *offset + count;
+ if (next_offset > buf->length) {
+ WARN(1, "tpm_buf: read out of boundary\n");
+ buf->flags |= TPM_BUF_BOUNDARY_ERROR;
+ return;
+ }
+
+ memcpy(output, &buf->data[*offset], count);
+ *offset = next_offset;
+}
+
+/**
+ * tpm_buf_read_u8() - Read 8-bit word from a TPM buffer
+ * @buf: &tpm_buf instance
+ * @offset: offset within the buffer
+ *
+ * Return: next 8-bit word
+ */
+u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset)
+{
+ u8 value;
+
+ tpm_buf_read(buf, offset, sizeof(value), &value);
+
+ return value;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_read_u8);
+
+/**
+ * tpm_buf_read_u16() - Read 16-bit word from a TPM buffer
+ * @buf: &tpm_buf instance
+ * @offset: offset within the buffer
+ *
+ * Return: next 16-bit word
+ */
+u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset)
+{
+ u16 value;
+
+ tpm_buf_read(buf, offset, sizeof(value), &value);
+
+ return be16_to_cpu(value);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_read_u16);
+
+/**
+ * tpm_buf_read_u32() - Read 32-bit word from a TPM buffer
+ * @buf: &tpm_buf instance
+ * @offset: offset within the buffer
+ *
+ * Return: next 32-bit word
+ */
+u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
+{
+ u32 value;
+
+ tpm_buf_read(buf, offset, sizeof(value), &value);
+
+ return be32_to_cpu(value);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_read_u32);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 715db4a91c1f..e8172f81c562 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -302,6 +302,8 @@ enum tpm_buf_flags {
TPM_BUF_OVERFLOW = BIT(0),
/* TPM2B format: */
TPM_BUF_TPM2B = BIT(1),
+ /* read out of boundary: */
+ TPM_BUF_BOUNDARY_ERROR = BIT(2),
};

/*
@@ -338,6 +340,9 @@ void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length);
void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
+u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset);
+u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset);
+u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset);

/*
* Check if TPM device is in the firmware upgrade mode.
--
2.43.0

2023-11-24 02:03:54

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 2/8] tpm: Remove tpm_send()

Open code the last remaining call site for tpm_send().

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v1 [2023-11-21]: A new patch.
---
drivers/char/tpm/tpm-interface.c | 25 -----------------------
include/linux/tpm.h | 5 -----
security/keys/trusted-keys/trusted_tpm1.c | 14 +++++++++++--
3 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 66b16d26eecc..163ae247bff2 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -342,31 +342,6 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
}
EXPORT_SYMBOL_GPL(tpm_pcr_extend);

-/**
- * tpm_send - send a TPM command
- * @chip: a &struct tpm_chip instance, %NULL for the default chip
- * @cmd: a TPM command buffer
- * @buflen: the length of the TPM command buffer
- *
- * Return: same as with tpm_transmit_cmd()
- */
-int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
-{
- struct tpm_buf buf;
- int rc;
-
- chip = tpm_find_get_ops(chip);
- if (!chip)
- return -ENODEV;
-
- buf.data = cmd;
- rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to a send a command");
-
- tpm_put_ops(chip);
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_send);
-
int tpm_auto_startup(struct tpm_chip *chip)
{
int rc;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 6588ca87cf93..d9d645e9c52c 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -422,7 +422,6 @@ extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digest);
extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests);
-extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
extern struct tpm_chip *tpm_default_chip(void);
void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
@@ -443,10 +442,6 @@ static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
return -ENODEV;
}

-static inline int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
-{
- return -ENODEV;
-}
static inline int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max)
{
return -ENODEV;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index aa108bea6739..37bce84eef99 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -356,17 +356,27 @@ static int TSS_checkhmac2(unsigned char *buffer,
*/
int trusted_tpm_send(unsigned char *cmd, size_t buflen)
{
+ struct tpm_buf buf;
int rc;

if (!chip)
return -ENODEV;

+ rc = tpm_try_get_ops(chip);
+ if (rc)
+ return rc;
+
+ buf.flags = 0;
+ buf.data = cmd;
dump_tpm_buf(cmd);
- rc = tpm_send(chip, cmd, buflen);
+ rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
dump_tpm_buf(cmd);
+
if (rc > 0)
- /* Can't return positive return codes values to keyctl */
+ /* TPM error */
rc = -EPERM;
+
+ tpm_put_ops(chip);
return rc;
}
EXPORT_SYMBOL_GPL(trusted_tpm_send);
--
2.43.0

2023-11-24 02:04:02

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 8/8] KEYS: trusted: tpm2: Use struct tpm_buf for sized buffers

Take advantage of the new sized buffer (TPM2B) mode of struct tpm_buf in
tpm2_seal_trusted(). This allows to add robustness to the command
construction without requiring to calculate buffer sizes manually.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v3 [2023-11-21]: A boundary error check as response for the feeedback
from Mario Limenciello:
https://lore.kernel.org/linux-integrity/[email protected]/
v2: Use tpm_buf_read_*
---
security/keys/trusted-keys/trusted_tpm2.c | 54 +++++++++++++----------
1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index bc700f85f80b..97b1dfca2dba 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
struct trusted_key_payload *payload,
struct trusted_key_options *options)
{
+ off_t offset = TPM_HEADER_SIZE;
+ struct tpm_buf buf, sized;
int blob_len = 0;
- struct tpm_buf buf;
u32 hash;
u32 flags;
int i;
@@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
return rc;
}

+ rc = tpm_buf_init_sized(&sized);
+ if (rc) {
+ tpm_buf_destroy(&buf);
+ tpm_put_ops(chip);
+ return rc;
+ }
+
+ tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
tpm_buf_append_u32(&buf, options->keyhandle);
tpm2_buf_append_auth(&buf, TPM2_RS_PW,
NULL /* nonce */, 0,
@@ -266,36 +275,36 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
TPM_DIGEST_SIZE);

/* sensitive */
- tpm_buf_append_u16(&buf, 4 + options->blobauth_len + payload->key_len);
+ tpm_buf_append_u16(&sized, options->blobauth_len);

- tpm_buf_append_u16(&buf, options->blobauth_len);
if (options->blobauth_len)
- tpm_buf_append(&buf, options->blobauth, options->blobauth_len);
+ tpm_buf_append(&sized, options->blobauth, options->blobauth_len);

- tpm_buf_append_u16(&buf, payload->key_len);
- tpm_buf_append(&buf, payload->key, payload->key_len);
+ tpm_buf_append_u16(&sized, payload->key_len);
+ tpm_buf_append(&sized, payload->key, payload->key_len);
+ tpm_buf_append(&buf, sized.data, sized.length);

/* public */
- tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
- tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
- tpm_buf_append_u16(&buf, hash);
+ tpm_buf_reset_sized(&sized);
+ tpm_buf_append_u16(&sized, TPM_ALG_KEYEDHASH);
+ tpm_buf_append_u16(&sized, hash);

/* key properties */
flags = 0;
flags |= options->policydigest_len ? 0 : TPM2_OA_USER_WITH_AUTH;
- flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM |
- TPM2_OA_FIXED_PARENT);
- tpm_buf_append_u32(&buf, flags);
+ flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT);
+ tpm_buf_append_u32(&sized, flags);

/* policy */
- tpm_buf_append_u16(&buf, options->policydigest_len);
+ tpm_buf_append_u16(&sized, options->policydigest_len);
if (options->policydigest_len)
- tpm_buf_append(&buf, options->policydigest,
- options->policydigest_len);
+ tpm_buf_append(&sized, options->policydigest, options->policydigest_len);

/* public parameters */
- tpm_buf_append_u16(&buf, TPM_ALG_NULL);
- tpm_buf_append_u16(&buf, 0);
+ tpm_buf_append_u16(&sized, TPM_ALG_NULL);
+ tpm_buf_append_u16(&sized, 0);
+
+ tpm_buf_append(&buf, sized.data, sized.length);

/* outside info */
tpm_buf_append_u16(&buf, 0);
@@ -312,21 +321,20 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
if (rc)
goto out;

- blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
- if (blob_len > MAX_BLOB_SIZE) {
+ blob_len = tpm_buf_read_u32(&buf, &offset);
+ if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
rc = -E2BIG;
goto out;
}
- if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
+ if (buf.length - offset < blob_len) {
rc = -EFAULT;
goto out;
}

- blob_len = tpm2_key_encode(payload, options,
- &buf.data[TPM_HEADER_SIZE + 4],
- blob_len);
+ blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);

out:
+ tpm_buf_destroy(&sized);
tpm_buf_destroy(&buf);

if (rc > 0) {
--
2.43.0

2023-11-24 02:04:05

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v6 6/8] tpm: TPM2B formatted buffers

Declare tpm_buf_init_sized() and tpm_buf_reset_sized() for creating TPM2B
formatted buffers. These buffers are also known as sized buffers in the
specifications and literature.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
v2: [2021-11-21] Refine the API according to the comments for
https://lore.kernel.org/linux-integrity/[email protected]/
---
drivers/char/tpm/tpm-buf.c | 38 +++++++++++++++++++++++++++++++++++---
include/linux/tpm.h | 4 ++++
2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 3f39893f3bb1..099b4a56c5d5 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -47,6 +47,36 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
}
EXPORT_SYMBOL_GPL(tpm_buf_reset);

+/**
+ * tpm_buf_init_sized() - Allocate and initialize a sized (TPM2B) buffer
+ * @buf: A @tpm_buf
+ *
+ * Return: 0 or -ENOMEM
+ */
+int tpm_buf_init_sized(struct tpm_buf *buf)
+{
+ buf->data = (u8 *)__get_free_page(GFP_KERNEL);
+ if (!buf->data)
+ return -ENOMEM;
+
+ tpm_buf_reset_sized(buf);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
+
+/**
+ * tpm_buf_reset_sized() - Initialize a sized buffer
+ * @buf: A &tpm_buf
+ */
+void tpm_buf_reset_sized(struct tpm_buf *buf)
+{
+ buf->flags = TPM_BUF_TPM2B;
+ buf->length = 2;
+ buf->data[0] = 0;
+ buf->data[1] = 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
+
void tpm_buf_destroy(struct tpm_buf *buf)
{
free_page((unsigned long)buf->data);
@@ -72,8 +102,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;
@@ -86,7 +114,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_TPM2B)
+ ((__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/include/linux/tpm.h b/include/linux/tpm.h
index 1d7b39b5c383..715db4a91c1f 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -300,6 +300,8 @@ struct tpm_header {
enum tpm_buf_flags {
/* the capacity exceeded: */
TPM_BUF_OVERFLOW = BIT(0),
+ /* TPM2B format: */
+ TPM_BUF_TPM2B = BIT(1),
};

/*
@@ -328,6 +330,8 @@ struct tpm2_hash {

int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
+int tpm_buf_init_sized(struct tpm_buf *buf);
+void tpm_buf_reset_sized(struct tpm_buf *buf);
void tpm_buf_destroy(struct tpm_buf *buf);
u32 tpm_buf_length(struct tpm_buf *buf);
void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length);
--
2.43.0

2023-11-27 20:33:44

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 1/8] tpm: Remove unused tpm_buf_tag()



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> The helper function has no call sites. Thus, remove it.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>

> ---
> v1 [2023-11-21]: A new patch.
> ---
> include/linux/tpm.h | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 4ee9d13749ad..6588ca87cf93 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -358,13 +358,6 @@ static inline u32 tpm_buf_length(struct tpm_buf *buf)
> return be32_to_cpu(head->length);
> }
>
> -static inline u16 tpm_buf_tag(struct tpm_buf *buf)
> -{
> - struct tpm_header *head = (struct tpm_header *)buf->data;
> -
> - return be16_to_cpu(head->tag);
> -}
> -
> static inline void tpm_buf_append(struct tpm_buf *buf,
> const unsigned char *new_data,
> unsigned int new_len)

2023-11-27 20:37:41

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 2/8] tpm: Remove tpm_send()



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> Open code the last remaining call site for tpm_send().
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>

Reviewed-by: Stefan Berger <[email protected]>

> ---
> v1 [2023-11-21]: A new patch.
> ---
> drivers/char/tpm/tpm-interface.c | 25 -----------------------
> include/linux/tpm.h | 5 -----
> security/keys/trusted-keys/trusted_tpm1.c | 14 +++++++++++--
> 3 files changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 66b16d26eecc..163ae247bff2 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -342,31 +342,6 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> }
> EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>
> -/**
> - * tpm_send - send a TPM command
> - * @chip: a &struct tpm_chip instance, %NULL for the default chip
> - * @cmd: a TPM command buffer
> - * @buflen: the length of the TPM command buffer
> - *
> - * Return: same as with tpm_transmit_cmd()
> - */
> -int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
> -{
> - struct tpm_buf buf;
> - int rc;
> -
> - chip = tpm_find_get_ops(chip);
> - if (!chip)
> - return -ENODEV;
> -
> - buf.data = cmd;
> - rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to a send a command");
> -
> - tpm_put_ops(chip);
> - return rc;
> -}
> -EXPORT_SYMBOL_GPL(tpm_send);
> -
> int tpm_auto_startup(struct tpm_chip *chip)
> {
> int rc;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 6588ca87cf93..d9d645e9c52c 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -422,7 +422,6 @@ extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
> struct tpm_digest *digest);
> extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> struct tpm_digest *digests);
> -extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
> extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
> extern struct tpm_chip *tpm_default_chip(void);
> void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> @@ -443,10 +442,6 @@ static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> return -ENODEV;
> }
>
> -static inline int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)
> -{
> - return -ENODEV;
> -}
> static inline int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max)
> {
> return -ENODEV;
> diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> index aa108bea6739..37bce84eef99 100644
> --- a/security/keys/trusted-keys/trusted_tpm1.c
> +++ b/security/keys/trusted-keys/trusted_tpm1.c
> @@ -356,17 +356,27 @@ static int TSS_checkhmac2(unsigned char *buffer,
> */
> int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> {
> + struct tpm_buf buf;
> int rc;
>
> if (!chip)
> return -ENODEV;
>
> + rc = tpm_try_get_ops(chip);
> + if (rc)
> + return rc;
> +
> + buf.flags = 0;
> + buf.data = cmd;
> dump_tpm_buf(cmd);
> - rc = tpm_send(chip, cmd, buflen);
> + rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
> dump_tpm_buf(cmd);
> +
> if (rc > 0)
> - /* Can't return positive return codes values to keyctl */
> + /* TPM error */
> rc = -EPERM;
> +
> + tpm_put_ops(chip);
> return rc;
> }
> EXPORT_SYMBOL_GPL(trusted_tpm_send);

2023-11-27 20:41:16

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 3/8] tpm: Move buffer handling from static inlines to real functions



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> From: James Bottomley <[email protected]>
>
> separate out the tpm_buf_... handling functions from static inlines in
> tpm.h and move them to their own tpm-buf.c file. This is a precursor
> to adding new functions for other TPM type handling because the amount
> of code will grow from the current 70 lines in tpm.h to about 200
> lines when the additions are done. 200 lines of inline functions is a
> bit too much to keep in a header file.
>
> Signed-off-by: James Bottomley <[email protected]>
> Signed-off-by: Jarkko Sakkinen <[email protected]>

Reviewed-by: Stefan Berger <[email protected]>

> ---
> v3: make tpm_buf_tag static
> v4: remove space after spdx tag
> v5: fix checkpatch.pl --strict issues
> ---
> drivers/char/tpm/Makefile | 1 +
> drivers/char/tpm/tpm-buf.c | 87 ++++++++++++++++++++++++++++++++++++++
> include/linux/tpm.h | 80 ++++-------------------------------
> 3 files changed, 97 insertions(+), 71 deletions(-)
> create mode 100644 drivers/char/tpm/tpm-buf.c
>
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 0222b1ddb310..ad3594e383e1 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o
> tpm-y += eventlog/common.o
> tpm-y += eventlog/tpm1.o
> tpm-y += eventlog/tpm2.o
> +tpm-y += tpm-buf.o
>
> tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
> tpm-$(CONFIG_EFI) += eventlog/efi.o
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> new file mode 100644
> index 000000000000..96cee41d5b9c
> --- /dev/null
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Handling of TPM command and other buffers.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/tpm.h>
> +
> +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> +{
> + buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> + if (!buf->data)
> + return -ENOMEM;
> +
> + buf->flags = 0;
> + tpm_buf_reset(buf, tag, ordinal);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_init);
> +
> +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> +{
> + struct tpm_header *head = (struct tpm_header *)buf->data;
> +
> + head->tag = cpu_to_be16(tag);
> + head->length = cpu_to_be32(sizeof(*head));
> + head->ordinal = cpu_to_be32(ordinal);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_reset);
> +
> +void tpm_buf_destroy(struct tpm_buf *buf)
> +{
> + free_page((unsigned long)buf->data);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_destroy);
> +
> +u32 tpm_buf_length(struct tpm_buf *buf)
> +{
> + struct tpm_header *head = (struct tpm_header *)buf->data;
> +
> + return be32_to_cpu(head->length);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_length);
> +
> +void tpm_buf_append(struct tpm_buf *buf,
> + const unsigned char *new_data,
> + unsigned int new_len)
> +{
> + struct tpm_header *head = (struct tpm_header *)buf->data;
> + u32 len = tpm_buf_length(buf);
> +
> + /* Return silently if overflow has already happened. */
> + if (buf->flags & TPM_BUF_OVERFLOW)
> + return;
> +
> + if ((len + new_len) > PAGE_SIZE) {
> + WARN(1, "tpm_buf: overflow\n");
> + buf->flags |= TPM_BUF_OVERFLOW;
> + return;
> + }
> +
> + memcpy(&buf->data[len], new_data, new_len);
> + head->length = cpu_to_be32(len + new_len);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_append);
> +
> +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
> +{
> + tpm_buf_append(buf, &value, 1);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_append_u8);
> +
> +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
> +{
> + __be16 value2 = cpu_to_be16(value);
> +
> + tpm_buf_append(buf, (u8 *)&value2, 2);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
> +
> +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
> +{
> + __be32 value2 = cpu_to_be32(value);
> +
> + tpm_buf_append(buf, (u8 *)&value2, 4);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index d9d645e9c52c..bb0e8718a432 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -326,77 +326,15 @@ struct tpm2_hash {
> unsigned int tpm_id;
> };
>
> -static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> -{
> - struct tpm_header *head = (struct tpm_header *)buf->data;
> -
> - head->tag = cpu_to_be16(tag);
> - head->length = cpu_to_be32(sizeof(*head));
> - head->ordinal = cpu_to_be32(ordinal);
> -}
> -
> -static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> -{
> - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> - if (!buf->data)
> - return -ENOMEM;
> -
> - buf->flags = 0;
> - tpm_buf_reset(buf, tag, ordinal);
> - return 0;
> -}
> -
> -static inline void tpm_buf_destroy(struct tpm_buf *buf)
> -{
> - free_page((unsigned long)buf->data);
> -}
> -
> -static inline u32 tpm_buf_length(struct tpm_buf *buf)
> -{
> - struct tpm_header *head = (struct tpm_header *)buf->data;
> -
> - return be32_to_cpu(head->length);
> -}
> -
> -static inline void tpm_buf_append(struct tpm_buf *buf,
> - const unsigned char *new_data,
> - unsigned int new_len)
> -{
> - struct tpm_header *head = (struct tpm_header *)buf->data;
> - u32 len = tpm_buf_length(buf);
> -
> - /* Return silently if overflow has already happened. */
> - if (buf->flags & TPM_BUF_OVERFLOW)
> - return;
> -
> - if ((len + new_len) > PAGE_SIZE) {
> - WARN(1, "tpm_buf: overflow\n");
> - buf->flags |= TPM_BUF_OVERFLOW;
> - return;
> - }
> -
> - memcpy(&buf->data[len], new_data, new_len);
> - head->length = cpu_to_be32(len + new_len);
> -}
> -
> -static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
> -{
> - tpm_buf_append(buf, &value, 1);
> -}
> -
> -static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
> -{
> - __be16 value2 = cpu_to_be16(value);
> -
> - tpm_buf_append(buf, (u8 *) &value2, 2);
> -}
> -
> -static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
> -{
> - __be32 value2 = cpu_to_be32(value);
> -
> - tpm_buf_append(buf, (u8 *) &value2, 4);
> -}
> +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
> +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);
> +void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
> + unsigned int new_len);
> +void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
> +void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
> +void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
>
> /*
> * Check if TPM device is in the firmware upgrade mode.

2023-11-27 20:42:09

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 4/8] tpm: Update struct tpm_buf documentation comments



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> Remove deprecated portions and document enum values.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> v1 [2023-11-21]: A new patch.
> v2 [2023-11-24]: Refined the commit message a bit.
> ---
> include/linux/tpm.h | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index bb0e8718a432..0a8c1351adc2 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -297,15 +297,14 @@ struct tpm_header {
> };
> } __packed;
>
> -/* A string buffer type for constructing TPM commands. This is based on the
> - * ideas of string buffer code in security/keys/trusted.h but is heap based
> - * in order to keep the stack usage minimal.
> - */
> -
> enum tpm_buf_flags {
> + /* the capacity exceeded: */

was exceeded

> TPM_BUF_OVERFLOW = BIT(0),
> };
>
> +/*
> + * A string buffer type for constructing TPM commands.
> + */
> struct tpm_buf {
> unsigned int flags;
> u8 *data;

Reviewed-by: Stefan Berger <[email protected]>

2023-11-27 21:04:31

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 5/8] tpm: Store the length of the tpm_buf data separately.



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> TPM2B buffers, or sized buffers, have a two byte header, which contains the
> length of the payload as a 16-bit big-endian number, without counting in
> the space taken by the header. This differs from encoding in the TPM header
> where the length includes also the bytes taken by the header.
>
> Unbound the length of a tpm_buf from the value stored to the TPM command
> header. A separate encoding and decoding step so that different buffer
> types can be supported, with variant header format and length encoding.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>

Reviewed-by: Stefan Berger <[email protected]>

> ---
> v3 [2023-11-21]: Removed spurious memset() (albeit not harmful). Expand
> tag invariant in tpm_buf_reset() to be allowed to be zero.
> v2 [2023-11-21]: Squashed together with the following patch, as the API
> of tpm_buf_init() is no longer changed.
> ---
> drivers/char/tpm/tpm-buf.c | 48 +++++++++++++++++------
> drivers/char/tpm/tpm-interface.c | 1 +
> include/keys/trusted_tpm.h | 2 -
> include/linux/tpm.h | 6 +--
> security/keys/trusted-keys/trusted_tpm1.c | 9 +++--
> 5 files changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index 96cee41d5b9c..3f39893f3bb1 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -3,25 +3,44 @@
> * Handling of TPM command and other buffers.
> */
>
> +#include <linux/tpm_command.h>
> #include <linux/module.h>
> #include <linux/tpm.h>
>
> +/**
> + * tpm_buf_init() - Allocate and initialize a TPM command
> + * @buf: A &tpm_buf
> + * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
> + * @ordinal: A command ordinal
> + *
> + * Return: 0 or -ENOMEM
> + */
> int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> {
> buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> if (!buf->data)
> return -ENOMEM;
>
> - buf->flags = 0;
> tpm_buf_reset(buf, tag, ordinal);
> return 0;
> }
> EXPORT_SYMBOL_GPL(tpm_buf_init);
>
> +/**
> + * tpm_buf_reset() - Initialize a TPM command
> + * @buf: A &tpm_buf
> + * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
> + * @ordinal: A command ordinal
> + */
> void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> {
> struct tpm_header *head = (struct tpm_header *)buf->data;
>
> + WARN_ON(tag != TPM_TAG_RQU_COMMAND && tag != TPM2_ST_NO_SESSIONS &&
> + tag != TPM2_ST_SESSIONS && tag != 0);
> +
> + buf->flags = 0;
> + buf->length = sizeof(*head);
> head->tag = cpu_to_be16(tag);
> head->length = cpu_to_be32(sizeof(*head));
> head->ordinal = cpu_to_be32(ordinal);
> @@ -34,33 +53,40 @@ void tpm_buf_destroy(struct tpm_buf *buf)
> }
> EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>
> +/**
> + * tpm_buf_length() - Return the number of bytes consumed by the data
> + *
> + * Return: The number of bytes consumed by the buffer
> + */
> u32 tpm_buf_length(struct tpm_buf *buf)
> {
> - struct tpm_header *head = (struct tpm_header *)buf->data;
> -
> - return be32_to_cpu(head->length);
> + return buf->length;
> }
> EXPORT_SYMBOL_GPL(tpm_buf_length);
>
> -void tpm_buf_append(struct tpm_buf *buf,
> - const unsigned char *new_data,
> - unsigned int new_len)
> +/**
> + * tpm_buf_append() - Append data to an initialized buffer
> + * @buf: A &tpm_buf
> + * @new_data: A data blob
> + * @new_length: Size of the appended data
> + */
> +void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
> {
> struct tpm_header *head = (struct tpm_header *)buf->data;
> - u32 len = tpm_buf_length(buf);
>
> /* Return silently if overflow has already happened. */
> if (buf->flags & TPM_BUF_OVERFLOW)
> return;
>
> - if ((len + new_len) > PAGE_SIZE) {
> + if ((buf->length + new_length) > PAGE_SIZE) {
> WARN(1, "tpm_buf: overflow\n");
> buf->flags |= TPM_BUF_OVERFLOW;
> return;
> }
>
> - memcpy(&buf->data[len], new_data, new_len);
> - head->length = cpu_to_be32(len + new_len);
> + memcpy(&buf->data[buf->length], new_data, new_length);
> + buf->length += new_length;
> + head->length = cpu_to_be32(buf->length);
> }
> EXPORT_SYMBOL_GPL(tpm_buf_append);
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 163ae247bff2..ea75f2776c2f 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -232,6 +232,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
> if (len < min_rsp_body_length + TPM_HEADER_SIZE)
> return -EFAULT;
>
> + buf->length = len;
> return 0;
> }
> EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
> diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h
> index 7769b726863a..a088b33fd0e3 100644
> --- a/include/keys/trusted_tpm.h
> +++ b/include/keys/trusted_tpm.h
> @@ -6,8 +6,6 @@
> #include <linux/tpm_command.h>
>
> /* implementation specific TPM constants */
> -#define MAX_BUF_SIZE 1024
> -#define TPM_GETRANDOM_SIZE 14
> #define TPM_SIZE_OFFSET 2
> #define TPM_RETURN_OFFSET 6
> #define TPM_DATA_OFFSET 10
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 0a8c1351adc2..1d7b39b5c383 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -306,7 +306,8 @@ enum tpm_buf_flags {
> * A string buffer type for constructing TPM commands.
> */
> struct tpm_buf {
> - unsigned int flags;
> + u32 flags;
> + u32 length;
> u8 *data;
> };
>
> @@ -329,8 +330,7 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
> 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);
> -void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
> - unsigned int new_len);
> +void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length);
> void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
> void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
> void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
> diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> index 37bce84eef99..89c9798d1800 100644
> --- a/security/keys/trusted-keys/trusted_tpm1.c
> +++ b/security/keys/trusted-keys/trusted_tpm1.c
> @@ -367,6 +367,7 @@ int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> return rc;
>
> buf.flags = 0;
> + buf.length = buflen;
> buf.data = cmd;
> dump_tpm_buf(cmd);
> rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
> @@ -417,7 +418,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
> tpm_buf_append_u32(tb, handle);
> tpm_buf_append(tb, ononce, TPM_NONCE_SIZE);
>
> - ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, tb->length);
> if (ret < 0)
> return ret;
>
> @@ -441,7 +442,7 @@ int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
> return -ENODEV;
>
> tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_OIAP);
> - ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, tb->length);
> if (ret < 0)
> return ret;
>
> @@ -553,7 +554,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> tpm_buf_append_u8(tb, cont);
> tpm_buf_append(tb, td->pubauth, SHA1_DIGEST_SIZE);
>
> - ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, tb->length);
> if (ret < 0)
> goto out;
>
> @@ -644,7 +645,7 @@ static int tpm_unseal(struct tpm_buf *tb,
> tpm_buf_append_u8(tb, cont);
> tpm_buf_append(tb, authdata2, SHA1_DIGEST_SIZE);
>
> - ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> + ret = trusted_tpm_send(tb->data, tb->length);
> if (ret < 0) {
> pr_info("authhmac failed (%d)\n", ret);
> return ret;

2023-11-27 21:08:32

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 6/8] tpm: TPM2B formatted buffers



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> Declare tpm_buf_init_sized() and tpm_buf_reset_sized() for creating TPM2B
> formatted buffers. These buffers are also known as sized buffers in the
> specifications and literature.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>

Reviewed-by: Stefan Berger <[email protected]>

> ---
> v2: [2021-11-21] Refine the API according to the comments for
> https://lore.kernel.org/linux-integrity/[email protected]/
> ---
> drivers/char/tpm/tpm-buf.c | 38 +++++++++++++++++++++++++++++++++++---
> include/linux/tpm.h | 4 ++++
> 2 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index 3f39893f3bb1..099b4a56c5d5 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -47,6 +47,36 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> }
> EXPORT_SYMBOL_GPL(tpm_buf_reset);
>
> +/**
> + * tpm_buf_init_sized() - Allocate and initialize a sized (TPM2B) buffer
> + * @buf: A @tpm_buf
> + *
> + * Return: 0 or -ENOMEM
> + */
> +int tpm_buf_init_sized(struct tpm_buf *buf)
> +{
> + buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> + if (!buf->data)
> + return -ENOMEM;
> +
> + tpm_buf_reset_sized(buf);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
> +
> +/**
> + * tpm_buf_reset_sized() - Initialize a sized buffer
> + * @buf: A &tpm_buf
> + */
> +void tpm_buf_reset_sized(struct tpm_buf *buf)
> +{
> + buf->flags = TPM_BUF_TPM2B;
> + buf->length = 2;
> + buf->data[0] = 0;
> + buf->data[1] = 0;
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
> +
> void tpm_buf_destroy(struct tpm_buf *buf)
> {
> free_page((unsigned long)buf->data);
> @@ -72,8 +102,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;
> @@ -86,7 +114,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_TPM2B)
> + ((__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/include/linux/tpm.h b/include/linux/tpm.h
> index 1d7b39b5c383..715db4a91c1f 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -300,6 +300,8 @@ struct tpm_header {
> enum tpm_buf_flags {
> /* the capacity exceeded: */
> TPM_BUF_OVERFLOW = BIT(0),
> + /* TPM2B format: */
> + TPM_BUF_TPM2B = BIT(1),
> };
>
> /*
> @@ -328,6 +330,8 @@ struct tpm2_hash {
>
> int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
> void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
> +int tpm_buf_init_sized(struct tpm_buf *buf);
> +void tpm_buf_reset_sized(struct tpm_buf *buf);
> void tpm_buf_destroy(struct tpm_buf *buf);
> u32 tpm_buf_length(struct tpm_buf *buf);
> void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length);

2023-11-27 21:10:34

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 7/8] tpm: Add tpm_buf_read_{u8,u16,u32}



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> Declare reader functions for the instances of struct tpm_buf. If the read
> goes out of boundary, TPM_BUF_BOUNDARY_ERROR is set, and subsequent read
> will do nothing.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>

> ---
> v5 [2023-11-24]: Fixed off-by-one error in the boundary check.
> v4 [2023-11-21]: Address James Bottomley's feedback for v2 of this
> patch, i.e. offset pointer was not correctly dereferenced.
> v3 [2023-11-21]: Add possibility to check for boundary error to the
> as response to the feedback from Mario Limenciello:
> https://lore.kernel.org/linux-integrity/[email protected]/
> ---
> drivers/char/tpm/tpm-buf.c | 79 +++++++++++++++++++++++++++++++++++++-
> include/linux/tpm.h | 5 +++
> 2 files changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index 099b4a56c5d5..32619e9ab4fa 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -107,7 +107,7 @@ void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
> return;
>
> if ((buf->length + new_length) > PAGE_SIZE) {
> - WARN(1, "tpm_buf: overflow\n");
> + WARN(1, "tpm_buf: write overflow\n");
> buf->flags |= TPM_BUF_OVERFLOW;
> return;
> }
> @@ -143,3 +143,80 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
> tpm_buf_append(buf, (u8 *)&value2, 4);
> }
> EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
> +
> +/**
> + * tpm_buf_read() - Read from a TPM buffer
> + * @buf: &tpm_buf instance
> + * @offset: offset within the buffer
> + * @count: the number of bytes to read
> + * @output: the output buffer
> + */
> +static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void *output)
> +{
> + off_t next_offset;
> +
> + /* Return silently if overflow has already happened. */
> + if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
> + return;
> +
> + next_offset = *offset + count;
> + if (next_offset > buf->length) {
> + WARN(1, "tpm_buf: read out of boundary\n");
> + buf->flags |= TPM_BUF_BOUNDARY_ERROR;
> + return;
> + }
> +
> + memcpy(output, &buf->data[*offset], count);
> + *offset = next_offset;
> +}
> +
> +/**
> + * tpm_buf_read_u8() - Read 8-bit word from a TPM buffer
> + * @buf: &tpm_buf instance
> + * @offset: offset within the buffer
> + *
> + * Return: next 8-bit word
> + */
> +u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset)
> +{
> + u8 value;
> +
> + tpm_buf_read(buf, offset, sizeof(value), &value);
> +
> + return value;
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_read_u8);
> +
> +/**
> + * tpm_buf_read_u16() - Read 16-bit word from a TPM buffer
> + * @buf: &tpm_buf instance
> + * @offset: offset within the buffer
> + *
> + * Return: next 16-bit word
> + */
> +u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset)
> +{
> + u16 value;
> +
> + tpm_buf_read(buf, offset, sizeof(value), &value);
> +
> + return be16_to_cpu(value);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_read_u16);
> +
> +/**
> + * tpm_buf_read_u32() - Read 32-bit word from a TPM buffer
> + * @buf: &tpm_buf instance
> + * @offset: offset within the buffer
> + *
> + * Return: next 32-bit word
> + */
> +u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
> +{
> + u32 value;
> +
> + tpm_buf_read(buf, offset, sizeof(value), &value);
> +
> + return be32_to_cpu(value);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_read_u32);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 715db4a91c1f..e8172f81c562 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -302,6 +302,8 @@ enum tpm_buf_flags {
> TPM_BUF_OVERFLOW = BIT(0),
> /* TPM2B format: */
> TPM_BUF_TPM2B = BIT(1),
> + /* read out of boundary: */
> + TPM_BUF_BOUNDARY_ERROR = BIT(2),
> };
>
> /*
> @@ -338,6 +340,9 @@ void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length);
> void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
> void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
> void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
> +u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset);
> +u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset);
> +u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset);
>
> /*
> * Check if TPM device is in the firmware upgrade mode.

2023-11-27 21:26:10

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 8/8] KEYS: trusted: tpm2: Use struct tpm_buf for sized buffers



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> Take advantage of the new sized buffer (TPM2B) mode of struct tpm_buf in
> tpm2_seal_trusted(). This allows to add robustness to the command
> construction without requiring to calculate buffer sizes manually.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>

Reviewed-by: Stefan Berger <[email protected]>

> ---
> v3 [2023-11-21]: A boundary error check as response for the feeedback
> from Mario Limenciello:
> https://lore.kernel.org/linux-integrity/[email protected]/
> v2: Use tpm_buf_read_*
> ---
> security/keys/trusted-keys/trusted_tpm2.c | 54 +++++++++++++----------
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index bc700f85f80b..97b1dfca2dba 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -228,8 +228,9 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> struct trusted_key_payload *payload,
> struct trusted_key_options *options)
> {
> + off_t offset = TPM_HEADER_SIZE;
> + struct tpm_buf buf, sized;
> int blob_len = 0;
> - struct tpm_buf buf;
> u32 hash;
> u32 flags;
> int i;
> @@ -258,6 +259,14 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> return rc;
> }
>
> + rc = tpm_buf_init_sized(&sized);
> + if (rc) {
> + tpm_buf_destroy(&buf);
> + tpm_put_ops(chip);
> + return rc;
> + }
> +
> + tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
> tpm_buf_append_u32(&buf, options->keyhandle);
> tpm2_buf_append_auth(&buf, TPM2_RS_PW,
> NULL /* nonce */, 0,
> @@ -266,36 +275,36 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> TPM_DIGEST_SIZE);
>
> /* sensitive */
> - tpm_buf_append_u16(&buf, 4 + options->blobauth_len + payload->key_len);
> + tpm_buf_append_u16(&sized, options->blobauth_len);
>
> - tpm_buf_append_u16(&buf, options->blobauth_len);
> if (options->blobauth_len)
> - tpm_buf_append(&buf, options->blobauth, options->blobauth_len);
> + tpm_buf_append(&sized, options->blobauth, options->blobauth_len);
>
> - tpm_buf_append_u16(&buf, payload->key_len);
> - tpm_buf_append(&buf, payload->key, payload->key_len);
> + tpm_buf_append_u16(&sized, payload->key_len);
> + tpm_buf_append(&sized, payload->key, payload->key_len);
> + tpm_buf_append(&buf, sized.data, sized.length);
>
> /* public */
> - tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
> - tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
> - tpm_buf_append_u16(&buf, hash);
> + tpm_buf_reset_sized(&sized);
> + tpm_buf_append_u16(&sized, TPM_ALG_KEYEDHASH);
> + tpm_buf_append_u16(&sized, hash);
>
> /* key properties */
> flags = 0;
> flags |= options->policydigest_len ? 0 : TPM2_OA_USER_WITH_AUTH;
> - flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM |
> - TPM2_OA_FIXED_PARENT);
> - tpm_buf_append_u32(&buf, flags);
> + flags |= payload->migratable ? 0 : (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT);
> + tpm_buf_append_u32(&sized, flags);
>
> /* policy */
> - tpm_buf_append_u16(&buf, options->policydigest_len);
> + tpm_buf_append_u16(&sized, options->policydigest_len);
> if (options->policydigest_len)
> - tpm_buf_append(&buf, options->policydigest,
> - options->policydigest_len);
> + tpm_buf_append(&sized, options->policydigest, options->policydigest_len);
>
> /* public parameters */
> - tpm_buf_append_u16(&buf, TPM_ALG_NULL);
> - tpm_buf_append_u16(&buf, 0);
> + tpm_buf_append_u16(&sized, TPM_ALG_NULL);
> + tpm_buf_append_u16(&sized, 0);
> +
> + tpm_buf_append(&buf, sized.data, sized.length);
>
> /* outside info */
> tpm_buf_append_u16(&buf, 0);
> @@ -312,21 +321,20 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> if (rc)
> goto out;
>
> - blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
> - if (blob_len > MAX_BLOB_SIZE) {
> + blob_len = tpm_buf_read_u32(&buf, &offset);
> + if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
> rc = -E2BIG;
> goto out;
> }
> - if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
> + if (buf.length - offset < blob_len) {
> rc = -EFAULT;
> goto out;
> }
>
> - blob_len = tpm2_key_encode(payload, options,
> - &buf.data[TPM_HEADER_SIZE + 4],
> - blob_len);
> + blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
>
> out:
> + tpm_buf_destroy(&sized);
> tpm_buf_destroy(&buf);
>
> if (rc > 0) {

2023-11-28 12:35:41

by Stefan Berger

[permalink] [raw]
Subject: Re: [PATCH v6 7/8] tpm: Add tpm_buf_read_{u8,u16,u32}



On 11/23/23 21:02, Jarkko Sakkinen wrote:
> Declare reader functions for the instances of struct tpm_buf. If the read
> goes out of boundary, TPM_BUF_BOUNDARY_ERROR is set, and subsequent read
> will do nothing.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---

> + */
> +u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset)
> +{
> + u16 value;

This should be __be16 ...

> +
> + tpm_buf_read(buf, offset, sizeof(value), &value);
> +
> + return be16_to_cpu(value);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_read_u16);
> +
> +/**
> + * tpm_buf_read_u32() - Read 32-bit word from a TPM buffer
> + * @buf: &tpm_buf instance
> + * @offset: offset within the buffer
> + *
> + * Return: next 32-bit word
> + */
> +u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset)
> +{
> + u32 value;

... and this __be32 to avoid this here:

drivers/char/tpm/tpm-buf.c:203:16: warning: cast to restricted __be16
drivers/char/tpm/tpm-buf.c:220:16: warning: cast to restricted __be32

2023-12-04 02:41:14

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v6 4/8] tpm: Update struct tpm_buf documentation comments

On Mon Nov 27, 2023 at 10:41 PM EET, Stefan Berger wrote:
>
>
> On 11/23/23 21:02, Jarkko Sakkinen wrote:
> > Remove deprecated portions and document enum values.
> >
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > v1 [2023-11-21]: A new patch.
> > v2 [2023-11-24]: Refined the commit message a bit.
> > ---
> > include/linux/tpm.h | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index bb0e8718a432..0a8c1351adc2 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -297,15 +297,14 @@ struct tpm_header {
> > };
> > } __packed;
> >
> > -/* A string buffer type for constructing TPM commands. This is based on the
> > - * ideas of string buffer code in security/keys/trusted.h but is heap based
> > - * in order to keep the stack usage minimal.
> > - */
> > -
> > enum tpm_buf_flags {
> > + /* the capacity exceeded: */
>
> was exceeded

+1, agreed a better form :-)

>
> > TPM_BUF_OVERFLOW = BIT(0),
> > };
> >
> > +/*
> > + * A string buffer type for constructing TPM commands.
> > + */
> > struct tpm_buf {
> > unsigned int flags;
> > u8 *data;
>
> Reviewed-by: Stefan Berger <[email protected]>

If possible give this to the James' patch set, thank you for the review.

BR, Jarkko