Extend struct tpm_buf to support sized buffers (TPM2B)
Subsystems (4)
Jarkko Sakkinen
maintainerv3 cover Extend struct tpm_buf to support sized buffers (TPM2B)
This patch set implements my ideas on how to extend struct tpm_buf to support TPM2 sized buffers (TPM2B). See Section 10.4 in TPM2 Structures specification for more information.
The goal is to do initial groundwork for smoother landing of integrity protection patches by James Bottomley.
I tested the patch set with:
https://github.com/jarkkojs/buildroot-tpmdd/tree/linux-6.5.y
Compilation:
make qemu_x86_64_defconfig
make 2>&1 | tee build.txt;
TPM1 startup: output/images/start-qemu.sh --use-system-swtpm --rtc --tpm1 TPM2 startup: output/images/start-qemu.sh --use-system-swtpm --rtc
For TPM2 I executed the following as the smoke test for these patches:
/usr/lib/kselftests/run_kselftest.shtpm2_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
For TPM1 I tried:
keyctl add trusted kmk "new 32" @u
This caused TPM error 18, which AFAIK means that there is not SRK (?), which is probably an issue in my swtpm configuration, which is visible in board/qemu/start-qemu.sh.in.
v3:
- Resend with rebase to the latest upstream.
James Bottomley (1):
tpm: Move buffer handling from static inlines to real functions
Jarkko Sakkinen (5):
tpm: Store TPM buffer length
tpm: Detach tpm_buf_reset() from tpm_buf_init()
tpm: Support TPM2 sized buffers (TPM2B)
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 | 195 ++++++++++++++++++++++ drivers/char/tpm/tpm-interface.c | 18 +- drivers/char/tpm/tpm-sysfs.c | 3 +- drivers/char/tpm/tpm1-cmd.c | 26 ++- drivers/char/tpm/tpm2-cmd.c | 36 ++-- drivers/char/tpm/tpm2-space.c | 7 +- drivers/char/tpm/tpm_vtpm_proxy.c | 13 +- include/linux/tpm.h | 96 ++--------- security/keys/trusted-keys/trusted_tpm1.c | 12 +- security/keys/trusted-keys/trusted_tpm2.c | 60 ++++--- 11 files changed, 325 insertions(+), 142 deletions(-) create mode 100644 drivers/char/tpm/tpm-buf.c
Signature · 1 line
2.42.0
Jarkko Sakkinen
maintainerPATCH v3 1/6 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.
---
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 | 86 ++++--------------------------------- 3 files changed, 97 insertions(+), 77 deletions(-) create mode 100644 drivers/char/tpm/tpm-buf.c
drivers/char/tpm/Makefile +1 -0
@@ -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
drivers/char/tpm/tpm-buf.c new file +87 -0
87 lines not on this page · raw message
include/linux/tpm.h +9 -77
93 lines not on this page · raw message
Signature · 1 line
2.42.0
Re: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
Jarkko Sakkinen
maintainerRe: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>
Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>
Thanks I'll add it to the next round.
For the tpm_buf_read(), I was thinking along the lines of:
/**
* tpm_buf_read() - Read from a TPM buffer
* @buf: &tpm_buf instance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns false when the
* amount of bytes requested would overflow the buffer, which is expected to
* only happen in the case of hardware failure.
*/
static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output)
{
off_t next = *pos + count;
if (next >= buf->length) {
pr_warn("%s: %lu >= %lu\n", __func__, next, *offset);
return false;
}
memcpy(output, &buf->data[*pos], count);
*offset = next;
return true;
}BR, Jarkko
Re: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
Quoted 36 lines · Jarkko Sakkinen
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Thanks I'll add it to the next round.
For the
tpm_buf_read(), I was thinking along the lines of:/**
*tpm_buf_read()- Read from a TPM buffer
* @buf:&tpm_bufinstance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns false when the
* amount of bytes requested would overflow the buffer, which is expected to
* only happen in the case of hardware failure.
*/
static booltpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output){ off_t next = *pos + count; if (next >= buf->length) { pr_warn("%s: %lu >= %lu\n", __func__, next, *offset); return false; } memcpy(output, &buf->data[*pos], count); *offset = next; return true; }
Quoted 2 lines
BR, Jarkko
Then the callers will check, and return -EIO?
James Bottomley
maintainerRe: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
Quoted 41 lines · Jerry Snitselaar
Quoted 40 lines · Jarkko Sakkinen
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Thanks I'll add it to the next round.
For the
tpm_buf_read(), I was thinking along the lines of:/**
*tpm_buf_read()- Read from a TPM buffer
* @buf: &tpm_buf instance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns false when the
* amount of bytes requested would overflow the buffer, which is expected to
* only happen in the case of hardware failure.
*/
static booltpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output)
{
off_t next = *pos + count;if (next >=
buf->length) {
pr_warn("%s: %lu >= %lu\n",__func__, next, *offset);
return false;
}memcpy(output, &buf->data[*pos], count);
*offset = next;
return true;
}
Quoted 4 lines
Quoted 2 lines
BR, Jarkko
Then the callers will check, and return
-EIO?
Really, no, why would we do that?
The initial buffer is a page and no TPM currently can have a command that big, so if the buffer overflows, it's likely a programming error (failure to terminate loop or something) rather than a runtime one (a user actually induced a command that big and wanted it to be sent to the TPM). The only reason you might need to check is the no-alloc case and you passed in a much smaller buffer, but even there, I would guess it will come down to a coding fault not a possible runtime error.
James
Re: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
Quoted 57 lines · James Bottomley
Quoted 41 lines · Jerry Snitselaar
Quoted 40 lines · Jarkko Sakkinen
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Thanks I'll add it to the next round.
For the
tpm_buf_read(), I was thinking along the lines of:/**
*tpm_buf_read()- Read from a TPM buffer
* @buf: &tpm_buf instance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns false when the
* amount of bytes requested would overflow the buffer, which is expected to
* only happen in the case of hardware failure.
*/
static booltpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output)
{
off_t next = *pos + count;if (next >=
buf->length) {
pr_warn("%s: %lu >= %lu\n",__func__, next, *offset);
return false;
}memcpy(output, &buf->data[*pos], count);
*offset = next;
return true;
}Quoted 4 lines
Quoted 2 lines
BR, Jarkko
Then the callers will check, and return
-EIO?Really, no, why would we do that?
The initial buffer is a page and no TPM currently can have a command that big, so if the buffer overflows, it's likely a programming error (failure to terminate loop or something) rather than a runtime one (a user actually induced a command that big and wanted it to be sent to the TPM). The only reason you might need to check is the no-alloc case and you passed in a much smaller buffer, but even there, I would guess it will come down to a coding fault not a possible runtime error.
Quoted 2 lines
James
I was clarifying based on this exchange below between Jarkko and Mario discussing patch 5/6. From https://lore.kernel.org/all/CWGM2YH00DJ3.JKSYNNEWVRW4@suppilovahvero/ :
Quoted 25 lines
Quoted 9 lines
In the overflow case wouldn't you want to pass an error code up instead of just showing a WARN trace?
The helper functions can't tell the difference, and the net outcome is going to be that if there is overflow you get a warning trace in the kernel log and whatever garbage "value" happened to have going to the caller. It's a programmer error but it's also unpredictable what happens here.
Quoted 3 lines
I think it's cleaner to have callers of tpm_buf_read_u8/tpm_buf_read_u16/tpm_buf_read_u32 to to be able to know something wrong happened.
I think you have a fair point here and I also think it is also a bigger issue for the response parsing than programmer error. I.e. faulty or malicious TPM could return corrupted data, which makes
WARN()wrong choice.So, as a corrective measure I think it should be
pr_warn()instead, and instead of returning u8/u16/u32, all functions should return 'ssize_t' and-EIOin the case of overflow.Thank you, it was a really good catch.
BR, Jarkko
Jarkko Sakkinen
maintainerRe: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
In reply to James BottomleyQuoted 49 lines · James Bottomley
Quoted 41 lines · Jerry Snitselaar
Quoted 40 lines · Jarkko Sakkinen
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Thanks I'll add it to the next round.
For the
tpm_buf_read(), I was thinking along the lines of:/**
*tpm_buf_read()- Read from a TPM buffer
* @buf: &tpm_buf instance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns false when the
* amount of bytes requested would overflow the buffer, which is expected to
* only happen in the case of hardware failure.
*/
static booltpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output)
{
off_t next = *pos + count;if (next >=
buf->length) {
pr_warn("%s: %lu >= %lu\n",__func__, next, *offset);
return false;
}memcpy(output, &buf->data[*pos], count);
*offset = next;
return true;
}Quoted 4 lines
Quoted 2 lines
BR, Jarkko
Then the callers will check, and return
-EIO?Really, no, why would we do that?
Quoted 7 lines
The initial buffer is a page and no TPM currently can have a command that big, so if the buffer overflows, it's likely a programming error (failure to terminate loop or something) rather than a runtime one (a user actually induced a command that big and wanted it to be sent to the TPM). The only reason you might need to check is the no-alloc case and you passed in a much smaller buffer, but even there, I would guess it will come down to a coding fault not a possible runtime error.
Yeah, this was my thinking too. So in HMAC case you anyway would not need to check it because crypto is destined to fail anyway.
Returning boolean here does no harm so I thought that this is overally good compromise.
BR, Jarkko
Jarkko Sakkinen
maintainerRe: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
Quoted 59 lines · Jarkko Sakkinen
Quoted 49 lines · James Bottomley
Quoted 41 lines · Jerry Snitselaar
Quoted 40 lines · Jarkko Sakkinen
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Thanks I'll add it to the next round.
For the
tpm_buf_read(), I was thinking along the lines of:/**
*tpm_buf_read()- Read from a TPM buffer
* @buf: &tpm_buf instance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns false when the
* amount of bytes requested would overflow the buffer, which is expected to
* only happen in the case of hardware failure.
*/
static booltpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output)
{
off_t next = *pos + count;if (next >=
buf->length) {
pr_warn("%s: %lu >= %lu\n",__func__, next, *offset);
return false;
}memcpy(output, &buf->data[*pos], count);
*offset = next;
return true;
}Quoted 4 lines
Quoted 2 lines
BR, Jarkko
Then the callers will check, and return
-EIO?Really, no, why would we do that?
Quoted 7 lines
The initial buffer is a page and no TPM currently can have a command that big, so if the buffer overflows, it's likely a programming error (failure to terminate loop or something) rather than a runtime one (a user actually induced a command that big and wanted it to be sent to the TPM). The only reason you might need to check is the no-alloc case and you passed in a much smaller buffer, but even there, I would guess it will come down to a coding fault not a possible runtime error.
Quoted 5 lines
Yeah, this was my thinking too. So in HMAC case you anyway would not need to check it because crypto is destined to fail anyway.
Returning boolean here does no harm so I thought that this is overally good compromise.
Or actually maybe we should go just with void, as it does have even then "return value", as it emits to klog, right?
BR, Jarkko
James Bottomley
maintainerRe: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
Quoted 65 lines · Jarkko Sakkinen
Quoted 64 lines · Jarkko Sakkinen
Quoted 61 lines · James Bottomley
Quoted 45 lines · Jerry Snitselaar
Quoted 43 lines · Jarkko Sakkinen
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Thanks I'll add it to the next round.
For the
tpm_buf_read(), I was thinking along the lines of:/**
*tpm_buf_read()- Read from a TPM buffer
* @buf: &tpm_buf instance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns
false when the
* amount of bytes requested would overflow the buffer, which is
expected to
* only happen in the case of hardware failure.
*/
static bool tpm_buf_read(const structtpm_buf*buf,off_t*pos,size_tcount, void *output)
{
off_t next = *pos + count;if (next >=
buf->length) {
pr_warn("%s: %lu >= %lu\n",__func__, next, *offset);
return false;
}memcpy(output, &buf->data[*pos], count);
*offset = next;
return true;
}Quoted 4 lines
Quoted 2 lines
BR, Jarkko
Then the callers will check, and return
-EIO?Really, no, why would we do that?
The initial buffer is a page and no TPM currently can have a command that big, so if the buffer overflows, it's likely a programming error (failure to terminate loop or something) rather than a runtime one (a user actually induced a command that big and wanted it to be sent to the TPM). The only reason you might need to check is the no-alloc case and you passed in a much smaller buffer, but even there, I would guess it will come down to a coding fault not a possible runtime error.
Quoted 8 lines
Quoted 5 lines
Yeah, this was my thinking too. So in HMAC case you anyway would not need to check it because crypto is destined to fail anyway.
Returning boolean here does no harm so I thought that this is overally good compromise.
Or actually maybe we should go just with void, as it does have even then "return value", as it emits to klog, right?
Right, these functions are almost drop in replacements for the get_inc_XX ones. The latter were void returning because all the knowledge of what is being looked for is in the calling routine, because it knows at a macro level what structure the buffer should have.
James
Jarkko Sakkinen
maintainerRe: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions
In reply to Jerry SnitselaarQuoted 37 lines · Jerry Snitselaar
Quoted 36 lines · Jarkko Sakkinen
Quoted 1 line · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Quoted 2 lines · Jerry Snitselaar
Reviewed-by Jerry Snitselaar <[email protected]>Thanks I'll add it to the next round.
For the
tpm_buf_read(), I was thinking along the lines of:/**
*tpm_buf_read()- Read from a TPM buffer
* @buf:&tpm_bufinstance
* @pos: position within the buffer
* @count: the number of bytes to read
* @output: the output buffer
*
* Read bytes from a TPM buffer, and update the position. Returns false when the
* amount of bytes requested would overflow the buffer, which is expected to
* only happen in the case of hardware failure.
*/
static booltpm_buf_read(const struct tpm_buf *buf, off_t *pos, size_t count, void *output){
off_t next = *pos + count;if (next >=
buf->length) {
pr_warn("%s: %lu >= %lu\n",__func__, next, *offset); return false;
}memcpy(output, &buf->data[*pos], count);
*offset = next;
return true;
}
Quoted 4 lines
Quoted 2 lines
BR, Jarkko
Then the callers will check, and return
-EIO?
Most likely. My thinking with boolean was that this way it sort of documents that this function can have only single possible error state.
If it was int there could be possibly some other error codes to handle...
Just would prefer this based on my experience calling other in-kernel functions. I often need to use lxr a lot just to get full understanding of all possible POSIX codes...
BR, Jarkko
Jarkko Sakkinen
maintainerPATCH v3 2/6 tpm: Store TPM buffer length
In reply to Jarkko SakkinenBoth TPM commands and sized buffers (TPM2B) have a fixed size header, which is followed by the body. Store TPM buffer length to a new field in the struct tpm_buf.
The invariant here is that the length field must always contain the total length of the buffer, i.e. the sum header and body length. The value must then be mapped to the length representation of the buffer type, and this correspondence must be maintained.
--- drivers/char/tpm/tpm-buf.c | 36 ++++++++++++++++------- drivers/char/tpm/tpm-interface.c | 18 +++++++++--- include/linux/tpm.h | 10 +++---- security/keys/trusted-keys/trusted_tpm1.c | 8 ++--- 4 files changed, 49 insertions(+), 23 deletions(-)
drivers/char/tpm/tpm-buf.c +26 -10
@@ -18,6 +18,12 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) }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;
@@ -25,6 +31,8 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) head->tag =cpu_to_be16 (tag); head->length =cpu_to_be32 (sizeof (*head)); head->ordinal =cpu_to_be32 (ordinal); + + buf->length =sizeof (*head); }EXPORT_SYMBOL_GPL (tpm_buf_reset);
@@ -34,33 +42,41 @@ 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 buffer +* +* 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);
drivers/char/tpm/tpm-interface.c +14 -4
@@ -217,6 +217,9 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,int err;ssize_t len; +/* The recorded length is expected to match the command length: */ +WARN_ON (buf->length !=be32_to_cpu (header->length)); + len =tpm_transmit (chip, buf->data,PAGE_SIZE );if (len <0 )return len;
@@ -232,6 +235,11 @@ 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; + +/* The recorded length is expected to match the response length: */ +WARN_ON (buf->length !=be32_to_cpu (header->length)); +return 0 ; }EXPORT_SYMBOL_GPL (tpm_transmit_cmd);
@@ -345,12 +353,12 @@ 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 +* @data: command blob +* @cmdlen: length of the command * * Return: same as with tpm_transmit_cmd() */ -int tpm_send (struct tpm_chip *chip,void *cmd,size_t buflen) +int tpm_send (struct tpm_chip *chip,void *data,size_t length) {struct tpm_buf buf;int rc;
@@ -359,7 +367,9 @@ int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen)if (!chip)return -ENODEV ; - buf.data = cmd; + buf.flags =0 ; + buf.length = length; + buf.data = data; rc =tpm_transmit_cmd (chip, &buf,0 ,"attempting to a send a command" );tpm_put_ops (chip);
include/linux/tpm.h +5 -5
@@ -307,7 +307,8 @@ enum tpm_buf_flags { };struct tpm_buf { -unsigned int flags; +u32 flags; +u16 length;u8 *data; };
@@ -331,8 +332,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);
@@ -361,7 +361,7 @@ 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); +int tpm_send (struct tpm_chip *chip,void *data,size_t length);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);
@@ -382,7 +382,7 @@ 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) +static inline int tpm_send (struct tpm_chip *chip,void *data,size_t length) {return -ENODEV ; }
security/keys/trusted-keys/trusted_tpm1.c +4 -4
@@ -407,7 +407,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;
@@ -431,7 +431,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;
@@ -543,7 +543,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;
@@ -634,7 +634,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;
Signature · 1 line
2.42.0
Re: [PATCH v3 2/6] tpm: Store TPM buffer length
Jarkko Sakkinen
maintainerRe: [PATCH v3 2/6] tpm: Store TPM buffer length
Re: [PATCH v3 2/6] tpm: Store TPM buffer length
In reply to Jarkko SakkinenQuoted 22 lines · Jarkko Sakkinen
Both TPM commands and sized buffers (TPM2B) have a fixed size header, which is followed by the body. Store TPM buffer length to a new field in the struct
tpm_buf.The invariant here is that the length field must always contain the total length of the buffer, i.e. the sum header and body length. The value must then be mapped to the length representation of the buffer type, and this correspondence must be maintained.
Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/char/tpm/tpm-buf.c | 36 ++++++++++++++++------- drivers/char/tpm/tpm-interface.c | 18 +++++++++--- include/linux/tpm.h | 10 +++---- security/keys/trusted-keys/trusted_tpm1.c | 8 ++--- 4 files changed, 49 insertions(+), 23 deletions(-)drivers/char/tpm/tpm-buf.c +0
-0
drivers/char/tpm/tpm-buf.c int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) · line 23
Quoted 6 lines
drivers/char/tpm/tpm-buf.c +3
-0@@ -18,6 +18,12 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) }EXPORT_SYMBOL_GPL (tpm_buf_init); +/** +* tpm_buf_reset() - Initialize a TPM command +* @buf: A @tpm_buf
One minor thing I meant to mention, did you intend this to be &tpm_buf like it is for tpm_buf_append?
Regards,
Jerry
Jarkko Sakkinen
maintainerRe: [PATCH v3 2/6] tpm: Store TPM buffer length
Quoted 26 lines · Jerry Snitselaar
Quoted 25 lines · Jarkko Sakkinen
Both TPM commands and sized buffers (TPM2B) have a fixed size header, which is followed by the body. Store TPM buffer length to a new field in the struct
tpm_buf.The invariant here is that the length field must always contain the total length of the buffer, i.e. the sum header and body length. The value must then be mapped to the length representation of the buffer type, and this correspondence must be maintained.
Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/char/tpm/tpm-buf.c | 36 ++++++++++++++++------- drivers/char/tpm/tpm-interface.c | 18 +++++++++--- include/linux/tpm.h | 10 +++---- security/keys/trusted-keys/trusted_tpm1.c | 8 ++--- 4 files changed, 49 insertions(+), 23 deletions(-)drivers/char/tpm/tpm-buf.c +0
-0@@ -18,6 +18,12 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) }EXPORT_SYMBOL_GPL (tpm_buf_init);
Quoted 6 lines
drivers/char/tpm/tpm-buf.c int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) · line 23
Quoted 3 lines
drivers/char/tpm/tpm-buf.c +3
-0+/** +* tpm_buf_reset() - Initialize a TPM command +* @buf: A @tpm_buf One minor thing I meant to mention, did you intend this to be
&tpm_buflike it is fortpm_buf_append?
Yep, thanks. I'll change it to &tpm_buf.
BR, Jarkko
Jarkko Sakkinen
maintainerPATCH v3 3/6 tpm: Detach tpm_buf_reset() from tpm_buf_init()
In reply to Jarkko SakkinenIn order to use tpm_buf_init() for other buffer types, detach tpm_buf_reset() from tpm_buf_init().
--- drivers/char/tpm/tpm-buf.c | 15 ++++++++-- drivers/char/tpm/tpm-sysfs.c | 3 +- drivers/char/tpm/tpm1-cmd.c | 26 +++++++++++----- drivers/char/tpm/tpm2-cmd.c | 36 ++++++++++++++++------- drivers/char/tpm/tpm2-space.c | 7 +++-- drivers/char/tpm/tpm_vtpm_proxy.c | 13 ++++---- include/linux/tpm.h | 2 +- security/keys/trusted-keys/trusted_tpm1.c | 4 +-- security/keys/trusted-keys/trusted_tpm2.c | 9 ++++-- 9 files changed, 80 insertions(+), 35 deletions(-)
drivers/char/tpm/tpm-buf.c +13 -2
@@ -6,14 +6,25 @@#include <linux/module.h> #include <linux/tpm.h> -int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +/** +* tpm_buf_init() - Initialize from the heap +* @buf: A @tpm_buf +* +* 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) { buf->data = (u8 *)__get_free_page (GFP_KERNEL );if (!buf->data)return -ENOMEM ; buf->flags =0 ; -tpm_buf_reset (buf, tag, ordinal); + buf->length =TPM_HEADER_SIZE ; +memset (buf->data,0 ,TPM_HEADER_SIZE ); +return 0 ; }EXPORT_SYMBOL_GPL (tpm_buf_init);
drivers/char/tpm/tpm-sysfs.c +2 -1
@@ -44,9 +44,10 @@ 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,TPM_TAG_RQU_COMMAND ,TPM_ORD_READPUBEK )) +if (tpm_buf_init (&tpm_buf)) goto out_ops; +tpm_buf_reset (&tpm_buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_READPUBEK );tpm_buf_append (&tpm_buf, anti_replay,sizeof (anti_replay)); if (tpm_transmit_cmd(chip, &tpm_buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
drivers/char/tpm/tpm1-cmd.c +19 -7
@@ -328,10 +328,11 @@ static int tpm1_startup(struct tpm_chip *chip)dev_info (&chip->dev,"starting up the TPM manually\n" ); - rc =tpm_buf_init (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_STARTUP ); + rc =tpm_buf_init (&buf);if (rc <0 )return rc; +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_STARTUP );tpm_buf_append_u16 (&buf,TPM_ST_CLEAR ); rc =tpm_transmit_cmd (chip, &buf,0 ,"attempting to start the TPM" );
@@ -466,10 +467,11 @@ 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,TPM_TAG_RQU_COMMAND ,TPM_ORD_PCR_EXTEND ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_PCR_EXTEND );tpm_buf_append_u32 (&buf, pcr_idx);tpm_buf_append (&buf, hash,TPM_DIGEST_SIZE );
@@ -485,10 +487,12 @@ 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,TPM_TAG_RQU_COMMAND ,TPM_ORD_GET_CAP ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_GET_CAP ); +if (subcap_id ==TPM_CAP_VERSION_1_1 || subcap_id ==TPM_CAP_VERSION_1_2 ) {tpm_buf_append_u32 (&buf, subcap_id);
@@ -537,10 +541,12 @@ int tpm1_get_random(struct tpm_chip *chip, u8 *dest, size_t max)u32 recd;int rc; - rc =tpm_buf_init (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_GET_RANDOM ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_GET_RANDOM ); + do { tpm_buf_append_u32(&buf, num_bytes);
@@ -586,10 +592,11 @@ 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,TPM_TAG_RQU_COMMAND ,TPM_ORD_PCRREAD ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_PCRREAD );tpm_buf_append_u32 (&buf, pcr_idx); rc = tpm_transmit_cmd(chip, &buf, TPM_DIGEST_SIZE,
@@ -622,10 +629,12 @@ static int tpm1_continue_selftest(struct tpm_chip *chip)struct tpm_buf buf;int rc; - rc =tpm_buf_init (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_CONTINUE_SELFTEST ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_CONTINUE_SELFTEST ); + rc =tpm_transmit_cmd (chip, &buf,0 ,"continue selftest" );tpm_buf_destroy (&buf);return rc;
@@ -752,9 +761,12 @@ 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,TPM_TAG_RQU_COMMAND ,TPM_ORD_SAVESTATE ); + rc =tpm_buf_init (&buf);if (rc)return rc; + +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_SAVESTATE ); +/* now do the actual savestate */ for (try =0 ; try <TPM_RETRY ; try++) { rc =tpm_transmit_cmd (chip, &buf,0 ,NULL );
drivers/char/tpm/tpm2-cmd.c +25 -11
@@ -183,10 +183,12 @@ 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,TPM2_ST_NO_SESSIONS ,TPM2_CC_PCR_READ ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_PCR_READ ); + pcr_select[pcr_idx >>3 ] =1 << (pcr_idx &0x7 );tpm_buf_append_u32 (&buf,1 );
@@ -240,10 +242,11 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,int rc;int i; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_PCR_EXTEND ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM2_ST_SESSIONS ,TPM2_CC_PCR_EXTEND );tpm_buf_append_u32 (&buf, pcr_idx); auth_area.handle =cpu_to_be32 (TPM2_RS_PW );
@@ -299,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,0 ,0 ); + err =tpm_buf_init (&buf);if (err)return err;
@@ -350,13 +353,14 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle)struct tpm_buf buf;int rc; - rc =tpm_buf_init (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_FLUSH_CONTEXT ); + rc =tpm_buf_init (&buf);if (rc) {dev_warn (&chip->dev,"0x%08x was not flushed, out of memory\n" , handle);return ; } +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_FLUSH_CONTEXT );tpm_buf_append_u32 (&buf, handle);tpm_transmit_cmd (chip, &buf,0 ,"flushing context" );
@@ -390,9 +394,11 @@ 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,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY ); + rc =tpm_buf_init (&buf);if (rc)return rc; + +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY );tpm_buf_append_u32 (&buf,TPM2_CAP_TPM_PROPERTIES );tpm_buf_append_u32 (&buf, property_id);tpm_buf_append_u32 (&buf,1 );
@@ -431,9 +437,11 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)struct tpm_buf buf;int rc; - rc =tpm_buf_init (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_SHUTDOWN ); + rc =tpm_buf_init (&buf);if (rc)return ; + +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_SHUTDOWN );tpm_buf_append_u16 (&buf, shutdown_type);tpm_transmit_cmd (chip, &buf,0 ,"stopping the TPM" );tpm_buf_destroy (&buf);
@@ -459,10 +467,11 @@ static int tpm2_do_selftest(struct tpm_chip *chip)int rc;for (full =0 ; full <2 ; full++) { - rc =tpm_buf_init (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_SELF_TEST ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_SELF_TEST );tpm_buf_append_u8 (&buf, full); rc =tpm_transmit_cmd (chip, &buf,0 ,"attempting the self test" );
@@ -495,9 +504,11 @@ int tpm2_probe(struct tpm_chip *chip)struct tpm_buf buf;int rc; - rc =tpm_buf_init (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY ); + rc =tpm_buf_init (&buf);if (rc)return rc; + +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY );tpm_buf_append_u32 (&buf,TPM2_CAP_TPM_PROPERTIES );tpm_buf_append_u32 (&buf,TPM_PT_TOTAL_COMMANDS );tpm_buf_append_u32 (&buf,1 );
@@ -560,10 +571,11 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)int rc;int i =0 ; - rc =tpm_buf_init (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY );tpm_buf_append_u32 (&buf,TPM2_CAP_PCRS );tpm_buf_append_u32 (&buf,0 );tpm_buf_append_u32 (&buf,1 );
@@ -649,10 +661,11 @@ int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip) goto out; } - rc =tpm_buf_init (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY ); + rc =tpm_buf_init (&buf);if (rc) goto out; +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_GET_CAPABILITY );tpm_buf_append_u32 (&buf,TPM2_CAP_COMMANDS );tpm_buf_append_u32 (&buf,TPM2_CC_FIRST );tpm_buf_append_u32 (&buf, nr_commands);
@@ -711,10 +724,11 @@ static int tpm2_startup(struct tpm_chip *chip)dev_info (&chip->dev,"starting up the TPM manually\n" ); - rc =tpm_buf_init (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_STARTUP ); + rc =tpm_buf_init (&buf);if (rc <0 )return rc; +tpm_buf_reset (&buf,TPM2_ST_NO_SESSIONS ,TPM2_CC_STARTUP );tpm_buf_append_u16 (&buf,TPM2_SU_CLEAR ); rc =tpm_transmit_cmd (chip, &buf,0 ,"attempting to start the TPM" );tpm_buf_destroy (&buf);
drivers/char/tpm/tpm2-space.c +5 -2
@@ -76,10 +76,12 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,unsigned int body_size;int rc; - rc =tpm_buf_init (&tbuf,TPM2_ST_NO_SESSIONS ,TPM2_CC_CONTEXT_LOAD ); + rc =tpm_buf_init (&tbuf);if (rc)return rc; +tpm_buf_reset (&tbuf,TPM2_ST_NO_SESSIONS ,TPM2_CC_CONTEXT_LOAD ); + ctx = (struct tpm2_context *)&buf[*offset]; body_size =sizeof (*ctx) +be16_to_cpu (ctx->blob_size);tpm_buf_append (&tbuf, &buf[*offset], body_size);
@@ -126,10 +128,11 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,unsigned int body_size;int rc; - rc =tpm_buf_init (&tbuf,TPM2_ST_NO_SESSIONS ,TPM2_CC_CONTEXT_SAVE ); + rc =tpm_buf_init (&tbuf);if (rc)return rc; +tpm_buf_reset (&tbuf,TPM2_ST_NO_SESSIONS ,TPM2_CC_CONTEXT_SAVE );tpm_buf_append_u32 (&tbuf, handle); rc =tpm_transmit_cmd (chip, &tbuf,0 ,NULL );
drivers/char/tpm/tpm_vtpm_proxy.c +7 -6
@@ -399,14 +399,15 @@ 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); -if (chip->flags &TPM_CHIP_FLAG_TPM2 ) - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS , -TPM2_CC_SET_LOCALITY ); -else - rc =tpm_buf_init (&buf,TPM_TAG_RQU_COMMAND , -TPM_ORD_SET_LOCALITY ); + rc =tpm_buf_init (&buf);if (rc)return rc; + +if (chip->flags &TPM_CHIP_FLAG_TPM2 ) +tpm_buf_reset (&buf,TPM2_ST_SESSIONS ,TPM2_CC_SET_LOCALITY ); +else +tpm_buf_reset (&buf,TPM_TAG_RQU_COMMAND ,TPM_ORD_SET_LOCALITY ); +tpm_buf_append_u8 (&buf, locality); proxy_dev->state |=STATE_DRIVER_COMMAND ;
include/linux/tpm.h +1 -1
@@ -328,7 +328,7 @@ struct tpm2_hash { }; -int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal); +int tpm_buf_init (struct tpm_buf *buf);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);
security/keys/trusted-keys/trusted_tpm1.c +2 -2
@@ -664,7 +664,7 @@ static int key_seal(struct trusted_key_payload *p,struct tpm_buf tb;int ret; - ret =tpm_buf_init (&tb,0 ,0 ); + ret =tpm_buf_init (&tb);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,0 ,0 ); + ret =tpm_buf_init (&tb);if (ret)return ret;
security/keys/trusted-keys/trusted_tpm2.c +6 -3
@@ -252,12 +252,13 @@ int tpm2_seal_trusted(struct tpm_chip *chip,if (rc)return rc; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_CREATE ); + rc =tpm_buf_init (&buf);if (rc) {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,
@@ -409,10 +410,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip,if (blob_len > payload->blob_len)return -E2BIG ; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_LOAD ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM2_ST_SESSIONS ,TPM2_CC_LOAD );tpm_buf_append_u32 (&buf, options->keyhandle); tpm2_buf_append_auth(&buf, TPM2_RS_PW, NULL /* nonce */, 0,
@@ -465,10 +467,11 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,u8 *data;int rc; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_UNSEAL ); + rc =tpm_buf_init (&buf);if (rc)return rc; +tpm_buf_reset (&buf,TPM2_ST_SESSIONS ,TPM2_CC_UNSEAL );tpm_buf_append_u32 (&buf, blob_handle); tpm2_buf_append_auth(&buf, options->policyhandle ?
Signature · 1 line
2.42.0
Re: [PATCH v3 3/6] tpm: Detach tpm_buf_reset() from tpm_buf_init()
Jarkko Sakkinen
maintainerRe: [PATCH v3 3/6] tpm: Detach tpm_buf_reset() from tpm_buf_init()
Jarkko Sakkinen
maintainerPATCH v3 4/6 tpm: Support TPM2 sized buffers (TPM2B)
In reply to Jarkko SakkinenAdd 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).
--- 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(-)
drivers/char/tpm/tpm-buf.c +22 -10
@@ -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);
drivers/char/tpm/tpm-sysfs.c +1 -1
@@ -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 );
drivers/char/tpm/tpm1-cmd.c +7 -7
@@ -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;
drivers/char/tpm/tpm2-cmd.c +11 -11
@@ -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;
drivers/char/tpm/tpm2-space.c +2 -2
@@ -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;
drivers/char/tpm/tpm_vtpm_proxy.c +1 -1
@@ -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;
include/linux/tpm.h +2 -1
@@ -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);
security/keys/trusted-keys/trusted_tpm1.c +2 -2
@@ -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;
security/keys/trusted-keys/trusted_tpm2.c +3 -3
@@ -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;
Signature · 1 line
2.42.0
James Bottomley
maintainerRe: [PATCH v3 4/6] tpm: Support TPM2 sized buffers (TPM2B)
Quoted 17 lines · Jarkko Sakkinen
+++
b/drivers/char/tpm/tpm-buf.cdrivers/char/tpm/tpm-buf.c +0
-0@@ -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
*/
drivers/char/tpm/tpm-buf.c line 20
Quoted 2 lines
drivers/char/tpm/tpm-buf.c +1
-1-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
Jarkko Sakkinen
maintainerRe: [PATCH v3 4/6] tpm: Support TPM2 sized buffers (TPM2B)
Quoted 33 lines · James Bottomley
Quoted 17 lines · Jarkko Sakkinen
+++
b/drivers/char/tpm/tpm-buf.cdrivers/char/tpm/tpm-buf.c +0
-0@@ -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
*/drivers/char/tpm/tpm-buf.c line 20
Quoted 2 lines
drivers/char/tpm/tpm-buf.c +1
-1-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_resetfunction which can't be used in this case.
Quoted 4 lines
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
Re: [PATCH v3 4/6] tpm: Support TPM2 sized buffers (TPM2B)
In reply to Jarkko SakkinenQuoted 25 lines · Jarkko Sakkinen
Add boolean parameters @alloc and @sized to
tpm_buf_init():* If @alloc is set to false,
buf->datais 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 <[email protected]>--- 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(-)drivers/char/tpm/tpm-buf.c +0
-0
drivers/char/tpm/tpm-buf.c line 15
Quoted 10 lines
drivers/char/tpm/tpm-buf.c +3
-1@@ -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.
Quoted 2 lines
drivers/char/tpm/tpm-buf.c +0
-0* zero the bytes that the buffer headers will consume. *
Jarkko Sakkinen
maintainerRe: [PATCH v3 4/6] tpm: Support TPM2 sized buffers (TPM2B)
Quoted 42 lines · Jerry Snitselaar
Quoted 25 lines · Jarkko Sakkinen
Add boolean parameters @alloc and @sized to
tpm_buf_init():* If @alloc is set to false,
buf->datais 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 <[email protected]>--- 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(-)drivers/char/tpm/tpm-buf.c +0
-0drivers/char/tpm/tpm-buf.c line 15
Quoted 10 lines
drivers/char/tpm/tpm-buf.c +3
-1@@ -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.
Quoted 3 lines
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_initfor the different types would be good.
Yeah, I'll bake something based on this discussion.
BR, Jarkko
Jarkko Sakkinen
maintainerPATCH v3 5/6 tpm: Add tpm_buf_read_{u8,u16,u32}
In reply to Jarkko SakkinenAdd tpm_buf_read_u8(), tpm_buf_read_u16() and tpm_read_u32() for the sake of more convenient parsing of TPM responses.
--- drivers/char/tpm/tpm-buf.c | 69 ++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 3 ++ 2 files changed, 72 insertions(+)
drivers/char/tpm/tpm-buf.c +69 -0
@@ -124,3 +124,72 @@ 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 (const struct tpm_buf *buf,off_t *offset,size_t count,void *output) +{ +if (*(offset + count) >= buf->length) { +WARN (1 ,"tpm_buf: overflow\n" ); +return ; + } + +memcpy (output, &buf->data[*offset], count); + *offset += count; +} + +/** +* 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 (const 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 (const 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 (const 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);
include/linux/tpm.h +3 -0
@@ -337,6 +337,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 (const struct tpm_buf *buf,off_t *offset); +u16 tpm_buf_read_u16 (const struct tpm_buf *buf,off_t *offset); +u32 tpm_buf_read_u32 (const struct tpm_buf *buf,off_t *offset);/* * Check if TPM device is in the firmware upgrade mode.
Signature · 1 line
2.42.0
Re: [PATCH v3 5/6] tpm: Add tpm_buf_read_{u8,u16,u32}
Quoted 21 lines · Jarkko Sakkinen
Add
tpm_buf_read_u8(),tpm_buf_read_u16()andtpm_read_u32()for the sake of more convenient parsing of TPM responses.Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/char/tpm/tpm-buf.c | 69 ++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 3 ++ 2 files changed, 72 insertions(+)drivers/char/tpm/tpm-buf.c +4
-0@@ -124,3 +124,72 @@ 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
drivers/char/tpm/tpm-buf.c void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) · line 140
Quoted 10 lines
drivers/char/tpm/tpm-buf.c +10
-0+* @offset: offset within the buffer +* @count: the number of bytes to read +* @output: the output buffer +*/ +static void tpm_buf_read (const struct tpm_buf *buf,off_t *offset,size_t count,void *output) +{ +if (*(offset + count) >= buf->length) { +WARN (1 ,"tpm_buf: overflow\n" ); +return ; + }
In the overflow case wouldn't you want to pass an error code up instead of just showing a WARN trace?
The helper functions can't tell the difference, and the net outcome is going to be that if there is overflow you get a warning trace in the kernel log and whatever garbage "value" happened to have going to the caller. It's a programmer error but it's also unpredictable what happens here.
I think it's cleaner to have callers of tpm_buf_read_u8/tpm_buf_read_u16/tpm_buf_read_u32 to to be able to know something wrong happened.
Quoted 69 lines
drivers/char/tpm/tpm-buf.c +55
-0+ +memcpy (output, &buf->data[*offset], count); + *offset += count; +} + +/** +* 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 (const 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 (const 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 (const 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);include/linux/tpm.h +3
-0@@ -337,6 +337,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 (const struct tpm_buf *buf,off_t *offset); +u16 tpm_buf_read_u16 (const struct tpm_buf *buf,off_t *offset); +u32 tpm_buf_read_u32 (const struct tpm_buf *buf,off_t *offset);/* * Check if TPM device is in the firmware upgrade mode.
Jarkko Sakkinen
maintainerRe: [PATCH v3 5/6] tpm: Add tpm_buf_read_{u8,u16,u32}
Quoted 42 lines · Mario Limonciello
Quoted 21 lines · Jarkko Sakkinen
Add
tpm_buf_read_u8(),tpm_buf_read_u16()andtpm_read_u32()for the sake of more convenient parsing of TPM responses.Signed-off-by Jarkko Sakkinen <[email protected]>--- drivers/char/tpm/tpm-buf.c | 69 ++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 3 ++ 2 files changed, 72 insertions(+)drivers/char/tpm/tpm-buf.c +4
-0@@ -124,3 +124,72 @@ 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 drivers/char/tpm/tpm-buf.c void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value) · line 140
Quoted 10 lines
drivers/char/tpm/tpm-buf.c +10
-0+* @offset: offset within the buffer +* @count: the number of bytes to read +* @output: the output buffer +*/ +static void tpm_buf_read (const struct tpm_buf *buf,off_t *offset,size_t count,void *output) +{ +if (*(offset + count) >= buf->length) { +WARN (1 ,"tpm_buf: overflow\n" ); +return ; + }In the overflow case wouldn't you want to pass an error code up instead of just showing a WARN trace?
The helper functions can't tell the difference, and the net outcome is going to be that if there is overflow you get a warning trace in the kernel log and whatever garbage "value" happened to have going to the caller. It's a programmer error but it's also unpredictable what happens here.
Quoted 3 lines
I think it's cleaner to have callers of tpm_buf_read_u8/tpm_buf_read_u16/tpm_buf_read_u32 to to be able to know something wrong happened.
I think you have a fair point here and I also think it is also a bigger issue for the response parsing than programmer error. I.e. faulty or malicious TPM could return corrupted data, which makes WARN() wrong choice.
So, as a corrective measure I think it should be pr_warn() instead, and instead of returning u8/u16/u32, all functions should return 'ssize_t' and -EIO in the case of overflow.
Thank you, it was a really good catch.
BR, Jarkko
James Bottomley
maintainerRe: [PATCH v3 5/6] tpm: Add tpm_buf_read_{u8,u16,u32}
In reply to Jarkko SakkinenQuoted 15 lines · Jarkko Sakkinen
+++
b/drivers/char/tpm/tpm-buf.cdrivers/char/tpm/tpm-buf.c +0
-0@@ -124,3 +124,72 @@ 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);
drivers/char/tpm/tpm-buf.c +9
-0+ +/** +* 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(const struct tpm_buf *buf, off_t *offset,
Quoted 3 lines
size_tcount, void *output)
+{
+ if (*(offset + count) >=buf->length) {
I don't think you mean that; it's dereferencing a random location in the stack, which is why I see this check trip randomly when testing. I think you mean
if (*offset + count >= buf->length) {
James
Jarkko Sakkinen
maintainerRe: [PATCH v3 5/6] tpm: Add tpm_buf_read_{u8,u16,u32}
Quoted 24 lines · James Bottomley
Quoted 15 lines · Jarkko Sakkinen
+++
b/drivers/char/tpm/tpm-buf.cdrivers/char/tpm/tpm-buf.c +0
-0@@ -124,3 +124,72 @@ 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);
drivers/char/tpm/tpm-buf.c +9
-0+ +/** +* 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(const struct tpm_buf *buf, off_t *offset,Quoted 3 lines
size_tcount, void *output)
+{
+ if (*(offset + count) >=buf->length) {I don't think you mean that; it's dereferencing a random location in the stack, which is why I see this check trip randomly when testing. I think you mean
Quoted 3 lines
if (*offset + count >=
buf->length) {James
Yes, true! Thank you.
BR, Jarkko
Jarkko Sakkinen
maintainerPATCH v3 6/6 KEYS: trusted: tpm2: Use struct tpm_buf for sized buffers
In reply to Jarkko SakkinenTake 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.
---
v2:
* Use tpm_buf_read_*
--- security/keys/trusted-keys/trusted_tpm2.c | 51 +++++++++++++---------- 1 file changed, 29 insertions(+), 22 deletions(-)
security/keys/trusted-keys/trusted_tpm2.c +29 -22
@@ -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,13 @@ int tpm2_seal_trusted(struct tpm_chip *chip,return rc; } + rc =tpm_buf_init (&sized, true, true); +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,
@@ -267,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_init (&sized, false, true); +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 );
@@ -313,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 ]); + blob_len =tpm_buf_read_u32 (&buf, &offset);if (blob_len >MAX_BLOB_SIZE ) { 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 ) {
Signature · 1 line
2.42.0
Quoted 7 lines · Jarkko Sakkinen
For TPM1 I tried:
keyctl add trusted kmk "new 32" @u
This caused TPM error 18, which AFAIK means that there is not SRK (?), which is probably an issue in my swtpm configuration, which is visible in board/qemu/start-qemu.sh.in.
FYI: This would create a TPM 1.2 with an SRK with password 'sss':
swtpm_setup --tpmstate=./ --create-ek-cert --take-ownership --overwrite --srkpass sss --ownerpass ooo
Jarkko Sakkinen
maintainerQuoted 8 lines · Stefan Berger
Quoted 4 lines · Jarkko Sakkinen
For TPM1 I tried:
keyctl add trusted kmk "new 32" @u
Quoted 8 lines
Quoted 3 lines
This caused TPM error 18, which AFAIK means that there is not SRK (?), which is probably an issue in my swtpm configuration, which is visible in board/qemu/start-qemu.sh.in.
FYI: This would create a TPM 1.2 with an SRK with password 'sss':
swtpm_setup--tpmstate=./ --create-ek-cert --take-ownership --overwrite --srkpass sss --ownerpass ooo
Thanks! I'll update my scripts in my BuildRoot repository.
BR, Jarkko
Jarkko Sakkinen
maintainerQuoted 13 lines · Jarkko Sakkinen
Quoted 12 lines · Stefan Berger
Quoted 7 lines · Jarkko Sakkinen
For TPM1 I tried:
keyctl add trusted kmk "new 32" @u
This caused TPM error 18, which AFAIK means that there is not SRK (?), which is probably an issue in my swtpm configuration, which is visible in board/qemu/start-qemu.sh.in.
Quoted 6 lines
Quoted 4 lines
FYI: This would create a TPM 1.2 with an SRK with password 'sss':
swtpm_setup--tpmstate=./ --create-ek-cert --take-ownership --overwrite --srkpass sss --ownerpass oooThanks! I'll update my scripts in my BuildRoot repository.
The repository helps to verify that tpm_buf changes don't break anything. I created it because I saw it as too high risk not to verify tpm_buf changes properly, as everything uses them.
Any bug in HMAC session feature itself would be optimally only local to the feature and not something that spreads everywhere.
So both the patch set itself and also the BuildRoot repository effectively manages this risk.
BR, Jarkko