Re: [PATCH] tpm/eventlog: Use kvmalloc() for event log buffer

[Takashi Iwai <tiwai@suse.de>]

On Thu, 07 Nov 2024 13:17:33 +0100,
Paul Menzel wrote:
> 
> Dear Takashi,
> 
> 
> Thank you for the patch.
> 
> Am 07.11.24 um 12:18 schrieb Takashi Iwai:
> > The TPM2 ACPI table may request a large size for the event log, and it
> > may be over the max size of kmalloc().  When this happens, the driver
> 
> What is kmalloc()’s maximum size?

128kB or so, IIRC.
And according Andy, the table can be over 4MB.

> > spews the kernel WARNING at the probe time, but the error is
> > eventually ignored in the caller side, and it results in the missing
> > TPM event log exposure.
> > 
> > This patch replaces the devm_kmalloc() call with kvmalloc() to allow
> > larger sizes.  Since there is no devm variant for kvmalloc(), now it's
> > managed manually via devres_alloc() and devres_add().
> 
> As the access to the bug report is restricted, are you at liberty to
> share the system you’ve seen this on?

Likely yes, as it was reported to SLE15.  Sorry for that.

Basically the info provided there was almost what I put in the
description; the driver got the kernel WARNING and Andy tested my
patch.

If any further info is required, at best ask HPE people here in Cc.


thanks,

Takashi


> > Reported-and-tested-by: Andy Liang <andy.liang@hpe.com>
> > Cc: jenifer.golmitz@hpe.com
> > Link: https://bugzilla.suse.com/show_bug.cgi?id=1232421
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/char/tpm/eventlog/acpi.c | 21 ++++++++++++++++++---
> >   1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
> > index 69533d0bfb51..56f7d73fa6bf 100644
> > --- a/drivers/char/tpm/eventlog/acpi.c
> > +++ b/drivers/char/tpm/eventlog/acpi.c
> > @@ -63,6 +63,13 @@ static bool tpm_is_tpm2_log(void *bios_event_log, u64 len)
> >   	return n == 0;
> >   }
> >   +static void bios_event_log_release(struct device *dev, void *res)
> > +{
> > +	void **logp = res;
> > +
> > +	kvfree(*logp);
> > +}
> > +
> >   /* read binary bios log */
> >   int tpm_read_log_acpi(struct tpm_chip *chip)
> >   {
> > @@ -71,6 +78,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> >   	void __iomem *virt;
> >   	u64 len, start;
> >   	struct tpm_bios_log *log;
> > +	void **logp;
> >   	struct acpi_table_tpm2 *tbl;
> >   	struct acpi_tpm2_phy *tpm2_phy;
> >   	int format;
> > @@ -136,9 +144,16 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> >   	}
> >     	/* malloc EventLog space */
> > -	log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL);
> > -	if (!log->bios_event_log)
> > +	logp = devres_alloc(bios_event_log_release, sizeof(*logp), GFP_KERNEL);
> > +	if (!logp)
> >   		return -ENOMEM;
> > +	devres_add(&chip->dev, logp);
> > +	log->bios_event_log = kvmalloc(len, GFP_KERNEL);
> > +	if (!log->bios_event_log) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +	*logp = log->bios_event_log;
> >     	log->bios_event_log_end = log->bios_event_log + len;
> >   @@ -164,7 +179,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> >   	return format;
> >     err:
> > -	devm_kfree(&chip->dev, log->bios_event_log);
> > +	devres_release(&chip->dev, bios_event_log_release, NULL, NULL);
> >   	log->bios_event_log = NULL;
> >   	return ret;
> >   }
> 
> The diff looks good to me.
> 
> 
> Kind regards,
> 
> Paul