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 <[email protected]> > > Signed-off-by: Denis Aleksandrov <[email protected]> > > --- > > > > 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 <[email protected]> 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
