Re: [PATCH v2] tpm: prevents local DOS via tpm/tpm0/ppi/*operations

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Aug 27, 2025 at 07:55:23AM +0200, Paul Menzel wrote:
> Dear Denis,
> 
> 
> Thank you for your patch. In the summary, I’d use imperative mood:

+1

> 
> tpm: Prevent local DOS …
> 
> Am 27.08.25 um 04:21 schrieb Denis Aleksandrov:
> > Reads on tpm/tpm0/ppi/*operations can become very long on
> > misconfigured systems. Reading the TPM is a blocking operation,
> > thus a user could effectively trigger a DOS.
> > 
> > Resolve this by caching the results and avoiding the blocking
> > operations after the first read.
> 
> If you could elaborate, how to test this, and in possible error cases, how
> to debug this – for example, how to disable the cache–, that’d be great.

+1

> 
> > 
> > Reported-by: Jan Stancek <jstancek@redhat.com>
> > Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
> > ---
> > 
> > Changes in v2:
> >   - Replaced file permission change with a caching mechanism as
> >     suggested by Jarkko.
> > 
> >   drivers/char/tpm/tpm_ppi.c | 88 ++++++++++++++++++++++++++++----------
> >   1 file changed, 65 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> > index d53fce1c9d6f..e0212893748e 100644
> > --- a/drivers/char/tpm/tpm_ppi.c
> > +++ b/drivers/char/tpm/tpm_ppi.c
> > @@ -33,6 +33,21 @@ static const guid_t tpm_ppi_guid =
> >   	GUID_INIT(0x3DDDFAA6, 0x361B, 0x4EB4,
> >   		  0xA4, 0x24, 0x8D, 0x10, 0x08, 0x9D, 0x16, 0x53);
> > +static const char * const tpm_ppi_info[] = {
> > +	"Not implemented",
> > +	"BIOS only",
> > +	"Blocked for OS by BIOS",
> 
> Is this x86 specific? If not maybe use *system firmware*?
> 
> > +	"User required",
> > +	"User not required",
> > +};
> > +
> > +/* A spinlock to protect access to the cache from concurrent reads */
> > +static DEFINE_SPINLOCK(tpm_ppi_lock);
> > +
> > +static u32 ppi_operations_cache[PPI_VS_REQ_END + 1];
> > +
> > +static bool ppi_cache_populated;
> > +
> >   static bool tpm_ppi_req_has_parameter(u64 req)
> >   {
> >   	return req == 23;
> > @@ -277,8 +292,7 @@ static ssize_t tpm_show_ppi_response(struct device *dev,
> >   	return status;
> >   }
> > -static ssize_t show_ppi_operations(acpi_handle dev_handle, char *buf, u32 start,
> > -				   u32 end)
> > +static ssize_t cache_ppi_operations(acpi_handle dev_handle, char *buf)
> >   {
> >   	int i;
> >   	u32 ret;
> > @@ -286,34 +300,22 @@ static ssize_t show_ppi_operations(acpi_handle dev_handle, char *buf, u32 start,
> >   	union acpi_object *obj, tmp;
> >   	union acpi_object argv = ACPI_INIT_DSM_ARGV4(1, &tmp);
> > -	static char *info[] = {
> > -		"Not implemented",
> > -		"BIOS only",
> > -		"Blocked for OS by BIOS",
> > -		"User required",
> > -		"User not required",
> > -	};
> > -
> >   	if (!acpi_check_dsm(dev_handle, &tpm_ppi_guid, TPM_PPI_REVISION_ID_1,
> >   			    1 << TPM_PPI_FN_GETOPR))
> >   		return -EPERM;
> >   	tmp.integer.type = ACPI_TYPE_INTEGER;
> > -	for (i = start; i <= end; i++) {
> > +	for (i = 0; i <= PPI_VS_REQ_END; i++) {
> >   		tmp.integer.value = i;
> >   		obj = tpm_eval_dsm(dev_handle, TPM_PPI_FN_GETOPR,
> >   				   ACPI_TYPE_INTEGER, &argv,
> >   				   TPM_PPI_REVISION_ID_1);
> > -		if (!obj) {
> > +		if (!obj)
> >   			return -ENOMEM;
> > -		} else {
> > -			ret = obj->integer.value;
> > -			ACPI_FREE(obj);
> > -		}
> > -		if (ret > 0 && ret < ARRAY_SIZE(info))
> > -			len += sysfs_emit_at(buf, len, "%d %d: %s\n",
> > -					     i, ret, info[ret]);
> > +		ret = obj->integer.value;
> > +		ppi_operations_cache[i] = ret;
> > +		ACPI_FREE(obj);
> >   	}
> >   	return len;
> > @@ -323,20 +325,60 @@ static ssize_t tpm_show_ppi_tcg_operations(struct device *dev,
> >   					   struct device_attribute *attr,
> >   					   char *buf)
> >   {
> > +	int i;
> > +	ssize_t len = 0;
> > +	u32 ret;
> >   	struct tpm_chip *chip = to_tpm_chip(dev);
> > -	return show_ppi_operations(chip->acpi_dev_handle, buf, 0,
> > -				   PPI_TPM_REQ_MAX);
> > +	spin_lock(&tpm_ppi_lock);
> > +	if (!ppi_cache_populated) {
> > +		len = cache_ppi_operations(chip->acpi_dev_handle, buf);
> > +
> > +		if (len < 0)
> > +			return len;
> > +
> > +		ppi_cache_populated = true;
> > +	}
> > +
> > +	for (i = 0; i <= PPI_TPM_REQ_MAX; i++) {
> > +		ret = ppi_operations_cache[i];
> > +		if (ret > 0 && ret < ARRAY_SIZE(tpm_ppi_info))
> > +			len += sysfs_emit_at(buf, len, "%d %d: %s\n",
> > +							i, ret, tpm_ppi_info[ret]);
> > +	}
> > +	spin_unlock(&tpm_ppi_lock);
> > +
> > +	return len;
> >   }
> >   static ssize_t tpm_show_ppi_vs_operations(struct device *dev,
> >   					  struct device_attribute *attr,
> >   					  char *buf)
> >   {
> > +	int i;
> > +	ssize_t len = 0;
> > +	u32 ret;
> >   	struct tpm_chip *chip = to_tpm_chip(dev);
> > -	return show_ppi_operations(chip->acpi_dev_handle, buf, PPI_VS_REQ_START,
> > -				   PPI_VS_REQ_END);
> > +	spin_lock(&tpm_ppi_lock);
> > +	if (!ppi_cache_populated) {
> > +		len = cache_ppi_operations(chip->acpi_dev_handle, buf);
> > +
> > +		if (len < 0)
> > +			return len;
> > +
> > +		ppi_cache_populated = true;
> > +	}
> > +
> > +	for (i = PPI_VS_REQ_START; i <= PPI_VS_REQ_END; i++) {
> > +		ret = ppi_operations_cache[i];
> > +		if (ret > 0 && ret < ARRAY_SIZE(tpm_ppi_info))
> > +			len += sysfs_emit_at(buf, len, "%d %d: %s\n",
> > +							i, ret, tpm_ppi_info[ret]);
> > +	}
> > +	spin_unlock(&tpm_ppi_lock);
> > +
> > +	return len;
> >   }
> >   static DEVICE_ATTR(version, S_IRUGO, tpm_show_ppi_version, NULL);
> 
> The diff looks good. Feel free to carry:
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Could you look at the next patch as a sanity check for the issues that
you addressed? I highly appreciate your great comments on details like
the ones you put out, thank you.

> 
> 
> Kind regards,
> 
> Paul Menzel

BR, Jarkko