Messages in this thread | | | Date | Mon, 23 Oct 2023 20:38:30 -0500 | Subject | Re: [PATCH v3 5/6] tpm: Add tpm_buf_read_{u8,u16,u32} | From | Mario Limonciello <> |
| |
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.
| |