On Wed, Aug 27, 2025 at 8:48 AM Jarkko Sakkinen <jar...@kernel.org> wrote:
>
> 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
>
I can add this in a v3.
> >
> > 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
>
The issue is that this bug is not replicable on most systems, but the way that
I've been able to test it is by running the following:
$ time cat /sys/devices/pnp0/00:0a/tpm/tpm0/ppi/tcg_operations
and
$ time cat /sys/devices/pnp0/00:0a/tpm/tpm0/ppi/vs_operations
On a system that I know is experiencing the DOS symptom.
For debugging, I've been using an unpatched kernel and running the same
commands.
> >
> > >
> > > Reported-by: Jan Stancek <jstan...@redhat.com>
> > > Signed-off-by: Denis Aleksandrov <dalek...@redhat.com>
I'll make sure to add the Suggested-by tag in the future, and the v3.
Sorry about that.
> > > ---
> > >
> > > 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*?
> >
This was the original implementation, but I can change the info message to
be more general. I can add it to the v3.
> > > + "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))
To the point of minimally changing the original code, I also noticed that the
"Not Implemented" status never gets reported due to the above conditional.
Would it make sense to change "ret > 0" to "ret >= 0" for full reporting, in
both tpm_show_tcg/vs_operations()?
Please let me know what your thoughts are about adding this to the v3.
> > > + 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 <pmen...@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
>
Thank you all for your detailed reviews and suggestions. Once the
final review is made,
I'll make sure to make the proper corrections before sending in the v3.
Cheers,
Denis
