linux-kernel - Re: [PATCH v6 10/10] tpm-buf: Enable managed and stack allocations.

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] [day] [month] [year] [list]
Message-ID: <yynqxoqux5whcbsnticikhwmupqh57xfbll5egzkn42kj7gkaf@s4kwxfmto5ia>
Date: Mon, 20 Oct 2025 11:04:51 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
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 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:

>
>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?


> {
>+	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?

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

Thanks,
Stefano