linux-kernel - Re: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to real functions

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]

Message-ID: <3e69c10c5d03ab2ccf7bda82b7ed9991dbced523.camel@kernel.org>
Date:   Sun, 05 Nov 2023 23:59:31 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     James Bottomley <James.Bottomley@...senPartnership.com>,
        Jerry Snitselaar <jsnitsel@...hat.com>
Cc:     linux-integrity@...r.kernel.org, keyrings@...r.kernel.org,
        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>,
        Mario Limonciello <mario.limonciello@....com>,
        Julien Gomes <julien@...sta.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/6] tpm: Move buffer handling from static inlines to
 real functions

On Thu, 2023-10-26 at 13:55 -0400, James Bottomley wrote:
> On Thu, 2023-10-26 at 10:10 -0700, Jerry Snitselaar wrote:
> > On Wed, Oct 25, 2023 at 08:35:55PM +0300, Jarkko Sakkinen wrote:
> > > On Wed Oct 25, 2023 at 12:03 PM EEST, Jerry Snitselaar wrote:
> > > > Reviewed-by: Jerry Snitselaar <jsnitsel@...hat.com>
> > > 
> > > On Wed, 2023-10-25 at 02:03 -0700, Jerry Snitselaar wrote:
> > > > Reviewed-by: Jerry Snitselaar <jsnitsel@...hat.com>
> > > > 
> > > 
> > > Thanks I'll add it to the next round.
> > > 
> > > For the tpm_buf_read(), I was thinking along the lines of:
> > > 
> > > /**
> > >  * tpm_buf_read() - Read from a TPM buffer
> > >  * @buf:        &tpm_buf instance
> > >  * @pos:        position within the buffer
> > >  * @count:      the number of bytes to read
> > >  * @output:     the output buffer
> > >  *
> > >  * Read bytes from a TPM buffer, and update the position. Returns
> > > false when the
> > >  * amount of bytes requested would overflow the buffer, which is
> > > expected to
> > >  * only happen in the case of hardware failure.
> > >  */
> > > static bool tpm_buf_read(const struct tpm_buf *buf, off_t *pos,
> > > size_t count, void *output)
> > > {
> > >         off_t next = *pos + count;
> > > 
> > >         if (next >= buf->length) {
> > >                 pr_warn("%s: %lu >= %lu\n", __func__, next,
> > > *offset);
> > >                 return false;
> > >         }
> > > 
> > >         memcpy(output, &buf->data[*pos], count);
> > >         *offset = next;
> > >         return true;
> > > }
> > > 
> > > BR, Jarkko
> > > 
> > 
> > Then the callers will check, and return -EIO?
> 
> Really, no, why would we do that?
> 
> The initial buffer is a page and no TPM currently can have a command
> that big, so if the buffer overflows, it's likely a programming error
> (failure to terminate loop or something) rather than a runtime one (a
> user actually induced a command that big and wanted it to be sent to
> the TPM).  The only reason you might need to check is the no-alloc case
> and you passed in a much smaller buffer, but even there, I would guess
> it will come down to a coding fault not a possible runtime error.


Yeah, this was my thinking too. So in HMAC case you anyway would not
need to check it because crypto is destined to fail anyway.

Returning boolean here does no harm so I thought that this is overally
good compromise.

BR, Jarkko