Re: [PATCH v5] tpm: Managed allocations for tpm_buf instances

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Jul 03, 2025 at 04:21:05PM -0400, Stefan Berger wrote:
> 
> 
> On 7/3/25 2:17 PM, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > 
> > Repeal and replace tpm_buf_init() and tpm_buf_init_sized() with
> > tpm_buf_alloc(), which returns a buffer of  memory with the struct tpm_buf
> > header at the beginning of the returned buffer. This leaves 4092 bytes of
> > free space for the payload.
> > 
> > Given that kfree() becomes the destructor for struct tpm_buf instances,
> > tpm_buf_destroy() is now obsolete, and can be removed.
> > 
> > The actual gist is that a struct tpm_buf instance can be declared using
> > __free(kfree) from linux/slab.h:
> > 
> > 	struct tpm_buf *buf __free(kfree) buf = tpm_buf_alloc();
> > 
> > Doing this has two-folded benefits associated with struct tpm_buf:
> > 
> > 1. New features will not introduce memory leaks.
> > 2. It addresses undiscovered memory leaks.
> > 
> > In addition, the barrier to contribute is lowered given that managing
> > memory is a factor easier.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> 
> > @@ -374,20 +362,18 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
> >    */
> >   void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
> >   {
> > -	struct tpm_buf buf;
> > -	int rc;
> > +	struct tpm_buf *buf __free(kfree) = tpm_buf_alloc();
> >
> 
> Remove empty line?

I recalled from the past that checkpatch.pl would complain if there was
no empty line after the declarations.

Now that I tested removing that line, it did not so I guess I can remove
that empty line. The presumed checkpatch error was the only reason for
having it.
> 
> > -	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
> > -	if (rc) {
> > +	if (!buf) {
> >   		dev_warn(&chip->dev, "0x%08x was not flushed, out of memory\n",
> >   			 handle);
> >   		return;
> >   	}
> > -	tpm_buf_append_u32(&buf, handle);
> > +	tpm_buf_reset(buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
> > +	tpm_buf_append_u32(buf, handle);
> > -	tpm_transmit_cmd(chip, &buf, 0, "flushing context");
> > -	tpm_buf_destroy(&buf);
> > +	tpm_transmit_cmd(chip, buf, 0, "flushing context");
> >   }
> >   EXPORT_SYMBOL_GPL(tpm2_flush_context);
> > @@ -414,19 +400,20 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
> >   			const char *desc)
> >   {
> >   	struct tpm2_get_cap_out *out;
> > -	struct tpm_buf buf;
> >   	int rc;
> > -	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
> > -	if (rc)
> > -		return rc;
> > -	tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
> > -	tpm_buf_append_u32(&buf, property_id);
> > -	tpm_buf_append_u32(&buf, 1);
> > -	rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
> > +	struct tpm_buf *buf __free(kfree) = tpm_buf_alloc();
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	tpm_buf_reset(buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
> > +	tpm_buf_append_u32(buf, TPM2_CAP_TPM_PROPERTIES);
> > +	tpm_buf_append_u32(buf, property_id);
> > +	tpm_buf_append_u32(buf, 1);
> > +	rc = tpm_transmit_cmd(chip, buf, 0, NULL);
> >   	if (!rc) {
> >   		out = (struct tpm2_get_cap_out *)
> > -			&buf.data[TPM_HEADER_SIZE];
> > +			&buf->data[TPM_HEADER_SIZE];
> >   		/*
> >   		 * To prevent failing boot up of some systems, Infineon TPM2.0
> >   		 * returns SUCCESS on TPM2_Startup in field upgrade mode. Also
> > @@ -438,7 +425,6 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
> >   		else
> >   			rc = -ENODATA;
> >   	}
> > -	tpm_buf_destroy(&buf);
> >   	return rc;
> >   }
> >   EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt);
> > @@ -455,15 +441,14 @@ EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt);
> >    */
> >   void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
> >   {
> > -	struct tpm_buf buf;
> > -	int rc;
> > +	struct tpm_buf *buf __free(kfree) = tpm_buf_alloc();
> 
> Remove empty line here.
> 
> With this nit fixed:
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 

Thanks and I'm happy to fixup those. They did look also silly to me :-)

BR, Jarkko