| Message ID | 20231024011531.442587-6-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Extend struct tpm_buf to support sized buffers (TPM2B) | expand |
On 10/23/2023 20:15, Jarkko Sakkinen wrote: > Add tpm_buf_read_u8(), tpm_buf_read_u16() and tpm_read_u32() for the sake > of more convenient parsing of TPM responses. > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > drivers/char/tpm/tpm-buf.c | 69 ++++++++++++++++++++++++++++++++++++++ > include/linux/tpm.h | 3 ++ > 2 files changed, 72 insertions(+) > > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c > index f1d92d7e758d..bcd3cbcd9dd9 100644 > --- a/drivers/char/tpm/tpm-buf.c > +++ b/drivers/char/tpm/tpm-buf.c > @@ -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; > + } 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. > + > + 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); > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 687b5173bdab..6590bd1f0a0e 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -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.
On Tue Oct 24, 2023 at 4:38 AM EEST, Mario Limonciello wrote: > On 10/23/2023 20:15, Jarkko Sakkinen wrote: > > Add tpm_buf_read_u8(), tpm_buf_read_u16() and tpm_read_u32() for the sake > > of more convenient parsing of TPM responses. > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > drivers/char/tpm/tpm-buf.c | 69 ++++++++++++++++++++++++++++++++++++++ > > include/linux/tpm.h | 3 ++ > > 2 files changed, 72 insertions(+) > > > > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c > > index f1d92d7e758d..bcd3cbcd9dd9 100644 > > --- a/drivers/char/tpm/tpm-buf.c > > +++ b/drivers/char/tpm/tpm-buf.c > > @@ -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; > > + } > > 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. 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
On Tue, 2023-10-24 at 04:15 +0300, Jarkko Sakkinen wrote: > +++ b/drivers/char/tpm/tpm-buf.c > @@ -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) { 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
On Fri, 2023-10-27 at 08:24 -0400, James Bottomley wrote: > On Tue, 2023-10-24 at 04:15 +0300, Jarkko Sakkinen wrote: > > +++ b/drivers/char/tpm/tpm-buf.c > > @@ -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) { > > 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 Yes, true! Thank you. BR, Jarkko
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c index f1d92d7e758d..bcd3cbcd9dd9 100644 --- a/drivers/char/tpm/tpm-buf.c +++ b/drivers/char/tpm/tpm-buf.c @@ -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); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 687b5173bdab..6590bd1f0a0e 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -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.
Add tpm_buf_read_u8(), tpm_buf_read_u16() and tpm_read_u32() for the sake of more convenient parsing of TPM responses. Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- drivers/char/tpm/tpm-buf.c | 69 ++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 3 ++ 2 files changed, 72 insertions(+)