linux-kernel - Re: [PATCH v3 5/6] tpm: Add tpm_buf_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Oct 2023 13:52:51 +0300
From:   "Jarkko Sakkinen" <jarkko@...nel.org>
To:     "Mario Limonciello" <mario.limonciello@....com>,
        <linux-integrity@...r.kernel.org>
Cc:     <keyrings@...r.kernel.org>,
        "James Bottomley" <James.Bottomley@...senPartnership.com>,
        "William Roberts" <bill.c.roberts@...il.com>,
        "Stefan Berger" <stefanb@...ux.ibm.com>,
        "David Howells" <dhowells@...hat.com>,
        "Jason Gunthorpe" <jgg@...pe.ca>,
        "Mimi Zohar" <zohar@...ux.ibm.com>,
        "Peter Huewe" <peterhuewe@....de>,
        "Julien Gomes" <julien@...sta.com>,
        "Jerry Snitselaar" <jsnitsel@...hat.com>,
        "open list" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 5/6] tpm: Add tpm_buf_read_{u8,u16,u32}

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@...nel.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