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@xxxxxxxxxx>
> > ---
> > 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
