Message-ID: <aPvKLYMtXZeQnA5D@kernel.org>
Date: Fri, 24 Oct 2025 21:49:17 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: linux-integrity@...r.kernel.org, keyring@...r.kernel.org,
	dpsmith@...rtussolutions.com, ross.philipson@...cle.com,
	Jonathan McDowell <noodles@...th.li>,
	Roberto Sassu <roberto.sassu@...wei.com>,
	Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>,
	Stefan Berger <stefanb@...ux.ibm.com>,
	Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
	James Bottomley <James.Bottomley@...senpartnership.com>,
	Mimi Zohar <zohar@...ux.ibm.com>,
	David Howells <dhowells@...hat.com>,
	Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
	"Serge E. Hallyn" <serge@...lyn.com>, linux-kernel@...r.kernel.org,
	keyrings@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v6 10/10] tpm-buf: Enable managed and stack allocations.

On Mon, Oct 20, 2025 at 11:04:51AM +0200, Stefano Garzarella wrote:
> On Sat, Oct 18, 2025 at 02:17:25PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>
> > 
> > Decouple kzalloc from buffer creation, so that a managed allocation can be
> > done trivially:
> > 
> > 	struct tpm_buf *buf __free(kfree) buf = kzalloc(TPM_BUFSIZE,
> > 							GFP_KERNEL);
> > 	if (!buf)
> > 		return -ENOMEM;
> > 	tpm_buf_init(buf, TPM_BUFSIZE);
> > 
> > Alternatively, stack allocations are also possible:
> > 
> > 	u8 buf_data[512];
> > 	struct tpm_buf *buf = (struct tpm_buf *)buf_data;
> > 	tpm_buf_init(buf, sizeof(buf_data));
> > 
> > Given that every single tpm_transmit_cmd() call site needs to be changed,
> > place command names from TCG 1.2 and 2.0 specifications to the @dest
> > parameter, which will e.g., help tracing bugs.
> 
> Perhaps my previous message fell through the cracks, but I still have a
> couple of comments (perhaps trivial, sorry in that case) that have not been
> answered about this patch:

I think what happened is that there was enough time that I forgot what
I had or hadn't done :-) I'll address your comments.

> 
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>
> > Reviewed-by: Stefan Berger <stefanb@...ux.ibm.com>
> > ---
> > v6
> > - Update commit message.
> > v5:
> > - There was a spurious change in tpm2_seal_trusted() error
> >  code handling introduce by this patch.
> > v4:
> > - Since every single tpm_transmit_cmd() call site needs to be
> >  changed anyhow, use 'dest' parameter more structured and
> >  actually useful way, and pick the string TCG 1.2 and 2.0
> >  specifications.
> > - tpm1-cmd: Remove useless rc declarations and repliace them
> >  with trivial "return tpm_transmit_cmd" statement.
> > - Reverted spurious changes in include/linux/tpm.h.
> > - Use concisely TPM_BUFSIZE instead of PAGE_SIZE.
> > v3:
> > - A new patch from the earlier series with more scoped changes and
> >  less abstract commit message.
> > ---
> > drivers/char/tpm/tpm-buf.c                | 122 +++++----
> > drivers/char/tpm/tpm-sysfs.c              |  21 +-
> > drivers/char/tpm/tpm.h                    |   1 -
> > drivers/char/tpm/tpm1-cmd.c               | 162 +++++-------
> > drivers/char/tpm/tpm2-cmd.c               | 299 ++++++++++------------
> > drivers/char/tpm/tpm2-sessions.c          | 122 +++++----
> > drivers/char/tpm/tpm2-space.c             |  44 ++--
> > drivers/char/tpm/tpm_vtpm_proxy.c         |  30 +--
> > include/linux/tpm.h                       |  18 +-
> > security/keys/trusted-keys/trusted_tpm1.c |  34 ++-
> > security/keys/trusted-keys/trusted_tpm2.c | 175 ++++++-------
> > 11 files changed, 484 insertions(+), 544 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index 1b9dee0d0681..a3bf3c3d0c48 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> 
> [...]
> 
> > @@ -92,6 +119,9 @@ EXPORT_SYMBOL_GPL(tpm_buf_destroy);
> >  */
> > u32 tpm_buf_length(struct tpm_buf *buf)
> 
> Should we update the return value to u16?

Ack.

> 
> 
> > {
> > +	if (buf->flags & TPM_BUF_INVALID)
> > +		return 0;
> > +
> > 	return buf->length;
> > }
> > EXPORT_SYMBOL_GPL(tpm_buf_length);
> 
> [...]
> 
> > diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> > index 636acb66a4f6..3ac204a902de 100644
> > --- a/security/keys/trusted-keys/trusted_tpm1.c
> > +++ b/security/keys/trusted-keys/trusted_tpm1.c
> > @@ -310,9 +310,8 @@ static int TSS_checkhmac2(unsigned char *buffer,
> >  * For key specific tpm requests, we will generate and send our
> >  * own TPM command packets using the drivers send function.
> >  */
> > -static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> > +static int trusted_tpm_send(void *cmd, size_t buflen)
> > {
> > -	struct tpm_buf buf;
> > 	int rc;
> > 
> > 	if (!chip)
> > @@ -322,15 +321,12 @@ static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> > 	if (rc)
> > 		return rc;
> > 
> > -	buf.flags = 0;
> > -	buf.length = buflen;
> > -	buf.data = cmd;
> > 	dump_tpm_buf(cmd);
> > -	rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
> > +	rc = tpm_transmit_cmd(chip, cmd, 4, "sending data");
> 
> Is it fine here to remove the intermediate tpm_buf ?
> 
> IIUC tpm_transmit_cmd() needs a tpm_buf, while here we are passing just
> the "data", or in some way it's a nested tpm_buf?

This does not look right at all. I'll fix it and add automated test
suite for TPM 1.x to my standard test shenanigans so that I can
continuously test its correctness [1]. It anyhow should have TPM 1.2
tests (especially for trusted keys). As it becomes more rare
accidental bugs could easily hover in.

Thanks, this was really good catch. I'll address both bug and make
sure that I don't do the same mistake twice :-)

> 
> > 	dump_tpm_buf(cmd);
> > 
> > +	/* Convert TPM error to -EPERM. */
> > 	if (rc > 0)
> > -		/* TPM error */
> > 		rc = -EPERM;
> > 
> > 	tpm_put_ops(chip);
> 
> Thanks,
> Stefano
> 

[1] https://codeberg.org/jarkko/linux-tpmdd-test

BR, Jarkko

Powered by blists - more mailing lists