Re: [PATCH v3 5/6] tpm: Add tpm_buf_read_{u8,u16,u32}

[Jarkko Sakkinen]

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