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

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

[Denis Aleksandrov]

This bug is not seen on most machines. 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 restricting unprivileged user from reading the
above-mentioned device files.

Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
---

Running scripts/checkpatch.pl suggested that the permissions be
changed to octal format. What do the maintainers think of this?
The rest of the permissions in the file are macros.

Lastly, this bug was reproduced and the fix was tested accordingly.

 drivers/char/tpm/tpm_ppi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
index bc7b1b4501b3..ac6e0aee566e 100644
--- a/drivers/char/tpm/tpm_ppi.c
+++ b/drivers/char/tpm/tpm_ppi.c
@@ -347,8 +347,8 @@ static DEVICE_ATTR(request, S_IRUGO | S_IWUSR | S_IWGRP,
 static DEVICE_ATTR(transition_action, S_IRUGO,
 		   tpm_show_ppi_transition_action, NULL);
 static DEVICE_ATTR(response, S_IRUGO, tpm_show_ppi_response, NULL);
-static DEVICE_ATTR(tcg_operations, S_IRUGO, tpm_show_ppi_tcg_operations, NULL);
-static DEVICE_ATTR(vs_operations, S_IRUGO, tpm_show_ppi_vs_operations, NULL);
+static DEVICE_ATTR(tcg_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_tcg_operations, NULL);
+static DEVICE_ATTR(vs_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_vs_operations, NULL);
 
 static struct attribute *ppi_attrs[] = {
 	&dev_attr_version.attr,
-- 
2.48.1

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

[Jarkko Sakkinen]

On Wed, Jul 02, 2025 at 04:28:51PM -0400, Denis Aleksandrov wrote:
> This bug is not seen on most machines. 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 restricting unprivileged user from reading the
> above-mentioned device files.

I suppose we can do this. I'm going to holiday for one week next
week so I'll hold for additional feedback for that period and
apply this if nothing comes up.

There's no use case for unprivileged user, or app that stops
working because of this. If you cut hairs, with patch shifting
uapi you have to we always prepared that tree falls down
somewhere but I'm willing to take risk with this :-)


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

> 
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
> ---
> 
> Running scripts/checkpatch.pl suggested that the permissions be
> changed to octal format. What do the maintainers think of this?
> The rest of the permissions in the file are macros.
> 
> Lastly, this bug was reproduced and the fix was tested accordingly.
> 
>  drivers/char/tpm/tpm_ppi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> index bc7b1b4501b3..ac6e0aee566e 100644
> --- a/drivers/char/tpm/tpm_ppi.c
> +++ b/drivers/char/tpm/tpm_ppi.c
> @@ -347,8 +347,8 @@ static DEVICE_ATTR(request, S_IRUGO | S_IWUSR | S_IWGRP,
>  static DEVICE_ATTR(transition_action, S_IRUGO,
>  		   tpm_show_ppi_transition_action, NULL);
>  static DEVICE_ATTR(response, S_IRUGO, tpm_show_ppi_response, NULL);
> -static DEVICE_ATTR(tcg_operations, S_IRUGO, tpm_show_ppi_tcg_operations, NULL);
> -static DEVICE_ATTR(vs_operations, S_IRUGO, tpm_show_ppi_vs_operations, NULL);
> +static DEVICE_ATTR(tcg_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_tcg_operations, NULL);
> +static DEVICE_ATTR(vs_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_vs_operations, NULL);
>  
>  static struct attribute *ppi_attrs[] = {
>  	&dev_attr_version.attr,
> -- 
> 2.48.1
> 

BR, Jarkko

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

[Denis Aleksandrov]

Hi Jarkko,

Thank you for the review. I'll add your Reviewed-by tag to my local commit.
Please let me know if you would like me to send a v2 version of the
patch with your tag included.

Best,
Denis


On Thu, Jul 3, 2025 at 7:56 AM Denis Aleksandrov <daleksan@redhat.com> wrote:
>
> Hi Jarkko,
>
> Thank you for the review. I'll add your Reviewed-by tag to my local commit.
> Please let me know if you would like me to send a v2 version of the patch with your tag included.
>
> Best,
> Denis
>
> On Wed, Jul 2, 2025 at 6:46 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>
>> On Wed, Jul 02, 2025 at 04:28:51PM -0400, Denis Aleksandrov wrote:
>> > This bug is not seen on most machines. 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 restricting unprivileged user from reading the
>> > above-mentioned device files.
>>
>> I suppose we can do this. I'm going to holiday for one week next
>> week so I'll hold for additional feedback for that period and
>> apply this if nothing comes up.
>>
>> There's no use case for unprivileged user, or app that stops
>> working because of this. If you cut hairs, with patch shifting
>> uapi you have to we always prepared that tree falls down
>> somewhere but I'm willing to take risk with this :-)
>>
>>
>> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>>
>> >
>> > Reported-by: Jan Stancek <jstancek@redhat.com>
>> > Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
>> > ---
>> >
>> > Running scripts/checkpatch.pl suggested that the permissions be
>> > changed to octal format. What do the maintainers think of this?
>> > The rest of the permissions in the file are macros.
>> >
>> > Lastly, this bug was reproduced and the fix was tested accordingly.
>> >
>> >  drivers/char/tpm/tpm_ppi.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
>> > index bc7b1b4501b3..ac6e0aee566e 100644
>> > --- a/drivers/char/tpm/tpm_ppi.c
>> > +++ b/drivers/char/tpm/tpm_ppi.c
>> > @@ -347,8 +347,8 @@ static DEVICE_ATTR(request, S_IRUGO | S_IWUSR | S_IWGRP,
>> >  static DEVICE_ATTR(transition_action, S_IRUGO,
>> >                  tpm_show_ppi_transition_action, NULL);
>> >  static DEVICE_ATTR(response, S_IRUGO, tpm_show_ppi_response, NULL);
>> > -static DEVICE_ATTR(tcg_operations, S_IRUGO, tpm_show_ppi_tcg_operations, NULL);
>> > -static DEVICE_ATTR(vs_operations, S_IRUGO, tpm_show_ppi_vs_operations, NULL);
>> > +static DEVICE_ATTR(tcg_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_tcg_operations, NULL);
>> > +static DEVICE_ATTR(vs_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_vs_operations, NULL);
>> >
>> >  static struct attribute *ppi_attrs[] = {
>> >       &dev_attr_version.attr,
>> > --
>> > 2.48.1
>> >
>>
>> BR, Jarkko
>>

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

[Denis Aleksandrov]

Ping.

Just checking in on this patch.
It has received a "Reviewed-by" tag, and I was wondering if there is
anything else needed from my side for it to be picked up.

Thanks,
Denis

On Thu, Jul 3, 2025 at 8:00 AM Denis Aleksandrov <daleksan@redhat.com> wrote:
>
> Hi Jarkko,
>
> Thank you for the review. I'll add your Reviewed-by tag to my local commit.
> Please let me know if you would like me to send a v2 version of the
> patch with your tag included.
>
> Best,
> Denis
>
>
> On Thu, Jul 3, 2025 at 7:56 AM Denis Aleksandrov <daleksan@redhat.com> wrote:
> >
> > Hi Jarkko,
> >
> > Thank you for the review. I'll add your Reviewed-by tag to my local commit.
> > Please let me know if you would like me to send a v2 version of the patch with your tag included.
> >
> > Best,
> > Denis
> >
> > On Wed, Jul 2, 2025 at 6:46 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >>
> >> On Wed, Jul 02, 2025 at 04:28:51PM -0400, Denis Aleksandrov wrote:
> >> > This bug is not seen on most machines. 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 restricting unprivileged user from reading the
> >> > above-mentioned device files.
> >>
> >> I suppose we can do this. I'm going to holiday for one week next
> >> week so I'll hold for additional feedback for that period and
> >> apply this if nothing comes up.
> >>
> >> There's no use case for unprivileged user, or app that stops
> >> working because of this. If you cut hairs, with patch shifting
> >> uapi you have to we always prepared that tree falls down
> >> somewhere but I'm willing to take risk with this :-)
> >>
> >>
> >> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> >>
> >> >
> >> > Reported-by: Jan Stancek <jstancek@redhat.com>
> >> > Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
> >> > ---
> >> >
> >> > Running scripts/checkpatch.pl suggested that the permissions be
> >> > changed to octal format. What do the maintainers think of this?
> >> > The rest of the permissions in the file are macros.
> >> >
> >> > Lastly, this bug was reproduced and the fix was tested accordingly.
> >> >
> >> >  drivers/char/tpm/tpm_ppi.c | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> >> > index bc7b1b4501b3..ac6e0aee566e 100644
> >> > --- a/drivers/char/tpm/tpm_ppi.c
> >> > +++ b/drivers/char/tpm/tpm_ppi.c
> >> > @@ -347,8 +347,8 @@ static DEVICE_ATTR(request, S_IRUGO | S_IWUSR | S_IWGRP,
> >> >  static DEVICE_ATTR(transition_action, S_IRUGO,
> >> >                  tpm_show_ppi_transition_action, NULL);
> >> >  static DEVICE_ATTR(response, S_IRUGO, tpm_show_ppi_response, NULL);
> >> > -static DEVICE_ATTR(tcg_operations, S_IRUGO, tpm_show_ppi_tcg_operations, NULL);
> >> > -static DEVICE_ATTR(vs_operations, S_IRUGO, tpm_show_ppi_vs_operations, NULL);
> >> > +static DEVICE_ATTR(tcg_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_tcg_operations, NULL);
> >> > +static DEVICE_ATTR(vs_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_vs_operations, NULL);
> >> >
> >> >  static struct attribute *ppi_attrs[] = {
> >> >       &dev_attr_version.attr,
> >> > --
> >> > 2.48.1
> >> >
> >>
> >> BR, Jarkko
> >>

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

[Jarkko Sakkinen]

On Fri, Aug 08, 2025 at 02:32:02PM -0400, Denis Aleksandrov wrote:
> Ping.
> 
> Just checking in on this patch.
> It has received a "Reviewed-by" tag, and I was wondering if there is
> anything else needed from my side for it to be picked up.

It's my bad I'll apply this next week!

Sorry!

> 
> Thanks,
> Denis

BR, Jarkko

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

[Jarkko Sakkinen]

On Fri, Aug 08, 2025 at 02:32:02PM -0400, Denis Aleksandrov wrote:
> Ping.
> 
> Just checking in on this patch.
> It has received a "Reviewed-by" tag, and I was wondering if there is
> anything else needed from my side for it to be picked up.
> 
> Thanks,
> Denis
> 
> On Thu, Jul 3, 2025 at 8:00 AM Denis Aleksandrov <daleksan@redhat.com> wrote:
> >
> > Hi Jarkko,
> >
> > Thank you for the review. I'll add your Reviewed-by tag to my local commit.
> > Please let me know if you would like me to send a v2 version of the
> > patch with your tag included.


Hi when I tried to apply this:

~/work/kernel.org/jarkko/linux-tpmdd master* ≡ ⇡
❯ scripts/checkpatch.pl --strict  0001-tpm-prevents-local-DOS-via-tpm-tpm0-ppi-operations.patch
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#13:
Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>

WARNING: DEVICE_ATTR unusual permissions 'S_IRUSR | S_IRGRP' used
#31: FILE: drivers/char/tpm/tpm_ppi.c:348:
+static DEVICE_ATTR(tcg_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_tcg_operations, NULL);

WARNING: Symbolic permissions 'S_IRUSR | S_IRGRP' are not preferred. Consider using octal permissions '0440'.
#31: FILE: drivers/char/tpm/tpm_ppi.c:348:
+static DEVICE_ATTR(tcg_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_tcg_operations, NULL);

WARNING: DEVICE_ATTR unusual permissions 'S_IRUSR | S_IRGRP' used
#32: FILE: drivers/char/tpm/tpm_ppi.c:349:
+static DEVICE_ATTR(vs_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_vs_operations, NULL);

WARNING: Symbolic permissions 'S_IRUSR | S_IRGRP' are not preferred. Consider using octal permissions '0440'.
#32: FILE: drivers/char/tpm/tpm_ppi.c:349:
+static DEVICE_ATTR(vs_operations, S_IRUSR | S_IRGRP, tpm_show_ppi_vs_operations, NULL);

total: 0 errors, 5 warnings, 0 checks, 10 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-tpm-prevents-local-DOS-via-tpm-tpm0-ppi-operations.patch has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

So could you fix those up?

BR, Jarkko 

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

[Jarkko Sakkinen]

On Tue, Aug 12, 2025 at 05:13:32PM -0400, Denis Aleksandrov wrote:
> Hi Jarkko,
> 
> Thanks again for taking the time to review this patch.
> 
> Regarding the `Closes:` tag warning, this issue is linked to an internal Red
> Hat page, which is why 
> it was omitted. Therefore, this warning might need to be ignored.
> 
> For the other warnings, I considered using either symbolic or octal permissions
> before submitting the
> patch. I decided on symbolic permissions because that style is used
> consistently throughout
> `drivers/char/tpm/tpm_ppi.c`.
> 
> When I tried changing the permissions to octal format, `scripts/checkpatch.pl
> --strict` still flagged those
> lines with the following warnings:
> 
> ```
> WARNING: Reported-by: should be immediately followed by Closes: with a URL to
> the report
> #13:
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>

Lemme check today, if I could give some feedback and help out with
this.

BTW could you use plain text in your emails? I know kernel people who
drop automatically HTML on this address. It's cool, I'm tolerant but
it's the standard :-)

There's a page for this too:

https://www.kernel.org/doc/html/latest/process/email-clients.html

Also, vger drops HTML mails, meaning that this response will arrive
there but your response that I'm responding most likely not (at least
vger used behave this way).

BR, Jarkko

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

[Denis Aleksandrov]

Sorry about the HTML, my reply kept getting booted yesterday due
to formatting and I wasn't sure why that was the case.

I guess it might be some automated gmail formatting.

Let me know how you would like to proceed.

Thanks again,
Denis

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

[Jarkko Sakkinen]

On Wed, Aug 13, 2025 at 09:13:19AM -0400, Denis Aleksandrov wrote:
> Sorry about the HTML, my reply kept getting booted yesterday due
> to formatting and I wasn't sure why that was the case.
> 
> I guess it might be some automated gmail formatting.
> 
> Let me know how you would like to proceed.
> 
> Thanks again,
> Denis
> 

So I need to postpone early next week because of travelling but
I'll come back then.

BR, Jarkko

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

[Denis Aleksandrov]

Sounds good, enjoy your travels!

Cheers,
Denis

On Thu, Aug 14, 2025 at 3:37 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, Aug 13, 2025 at 09:13:19AM -0400, Denis Aleksandrov wrote:
> > Sorry about the HTML, my reply kept getting booted yesterday due
> > to formatting and I wasn't sure why that was the case.
> >
> > I guess it might be some automated gmail formatting.
> >
> > Let me know how you would like to proceed.
> >
> > Thanks again,
> > Denis
> >
>
> So I need to postpone early next week because of travelling but
> I'll come back then.
>
> BR, Jarkko
>

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

[Jarkko Sakkinen]

On Fri, Aug 08, 2025 at 02:32:02PM -0400, Denis Aleksandrov wrote:
> Ping.
> 
> Just checking in on this patch.
> It has received a "Reviewed-by" tag, and I was wondering if there is
> anything else needed from my side for it to be picked up.

"This bug is not seen on most machines. 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 restricting unprivileged user from reading the
above-mentioned device files."

OK I took some time to revisit this. I think the symptom is correct but
it should not be fixed the way it is done here. I.e. the fix works as
a bug report but not as a right type of fix because it will also
change uapi behavior w/o strong enough reasons to do so.

What you should do instead would be to cache the result.

I.e. first declare this as static global:

static const char *tpm_ppi_info[] = {
	"Not implemented",
	"BIOS only",
	"Blocked for OS by BIOS",
	"User required",
	"User not required",
};


Then declare a spinlock:

static DEFINE_SPINLOCK(tpm_ppi_lock);

And finally static arrays that contain indexes to tpm_ppi_info e.g.,
tpm_ppi_tcg_operations and tpm_ppi_vs_operations.

Then on first request populate them and after that the results are
cached.


> Thanks,
> Denis

BR, Jarkko

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

[Denis Aleksandrov]

Hi Jarkko,

Thanks for the detailed review. I will update the fix, test it,
and send over a v2.

Hoping to get that over to you this week or early next.

Best,
Denis

P.S.
Would you like the v2 in a new thread, or keep it here?

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

[Jarkko Sakkinen]

On Mon, Aug 18, 2025 at 03:12:02PM -0400, Denis Aleksandrov wrote:
> Hi Jarkko,
> 
> Thanks for the detailed review. I will update the fix, test it,
> and send over a v2.
> 
> Hoping to get that over to you this week or early next.
> 
> Best,
> Denis
> 
> P.S.
> Would you like the v2 in a new thread, or keep it here?

Please try to quote responses properly.

Yeah, maybe it is better to open a new thread anyhow.

BR, Jarkko