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

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

[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.

Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
---

Changes in v4:
	- Removes empty lines.
	- Reorders vars to reverse christmas tree.

 drivers/char/tpm/tpm_ppi.c | 85 +++++++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 23 deletions(-)

diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
index d53fce1c9d6f..df34b215440d 100644
--- a/drivers/char/tpm/tpm_ppi.c
+++ b/drivers/char/tpm/tpm_ppi.c
@@ -33,6 +33,20 @@ 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 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 +291,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 +299,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;
@@ -324,9 +325,28 @@ static ssize_t tpm_show_ppi_tcg_operations(struct device *dev,
 					   char *buf)
 {
 	struct tpm_chip *chip = to_tpm_chip(dev);
+	ssize_t len = 0;
+	u32 ret;
+	int i;
+
+	spin_lock(&tpm_ppi_lock);
+	if (!ppi_cache_populated) {
+		len = cache_ppi_operations(chip->acpi_dev_handle, buf);
+		if (len < 0)
+			return len;
 
-	return show_ppi_operations(chip->acpi_dev_handle, buf, 0,
-				   PPI_TPM_REQ_MAX);
+		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,
@@ -334,9 +354,28 @@ static ssize_t tpm_show_ppi_vs_operations(struct device *dev,
 					  char *buf)
 {
 	struct tpm_chip *chip = to_tpm_chip(dev);
+	ssize_t len = 0;
+	u32 ret;
+	int i;
 
-	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);
-- 
2.48.1

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

[Jarkko Sakkinen]

On Tue, Sep 02, 2025 at 10:24:30AM -0400, Denis Aleksandrov wrote:
> 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.
> 
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
> 
> Changes in v4:
> 	- Removes empty lines.
> 	- Reorders vars to reverse christmas tree.
> 
>  drivers/char/tpm/tpm_ppi.c | 85 +++++++++++++++++++++++++++-----------
>  1 file changed, 62 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> index d53fce1c9d6f..df34b215440d 100644
> --- a/drivers/char/tpm/tpm_ppi.c
> +++ b/drivers/char/tpm/tpm_ppi.c
> @@ -33,6 +33,20 @@ 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 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 +291,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 +299,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;
> @@ -324,9 +325,28 @@ static ssize_t tpm_show_ppi_tcg_operations(struct device *dev,
>  					   char *buf)
>  {
>  	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t len = 0;
> +	u32 ret;
> +	int i;
> +
> +	spin_lock(&tpm_ppi_lock);
> +	if (!ppi_cache_populated) {
> +		len = cache_ppi_operations(chip->acpi_dev_handle, buf);
> +		if (len < 0)
> +			return len;
>  
> -	return show_ppi_operations(chip->acpi_dev_handle, buf, 0,
> -				   PPI_TPM_REQ_MAX);
> +		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,
> @@ -334,9 +354,28 @@ static ssize_t tpm_show_ppi_vs_operations(struct device *dev,
>  					  char *buf)
>  {
>  	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t len = 0;
> +	u32 ret;
> +	int i;
>  
> -	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);
> -- 
> 2.48.1
> 

Thank you! It's now an actual quality of service improvement whereas the
first fix more like "duct tape" alike solution :-)

I'll apply this within next few days.

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

BR, Jarkko

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

[Denis Aleksandrov]

Great, thank you Jarkko!

Could you please take a look at the most recent reply from Paul in the
v2 thread for this patch?
Should I move forward with the advised changes and send a v5?

The concerns were:
- Adding info on how to test and debug the code to the commit message.
- Reverting the "Blocked by OS ..." field in tpm_ppi_info[].
- Reverting the conditional in tpm_show_tcg/vs_operations()
and sending in a separate patch with the change. The change was
concerning displaying the "Not Implemented" status by updating
`ret > 0` to `ret >= 0` in the if statement.

Please let me know what you think.

Thanks again,
Denis

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

[Jarkko Sakkinen]

On Tue, Sep 02, 2025 at 03:12:45PM -0400, Denis Aleksandrov wrote:
> Great, thank you Jarkko!
> 
> Could you please take a look at the most recent reply from Paul in the
> v2 thread for this patch?
> Should I move forward with the advised changes and send a v5?

Mind passing me the lore link? At least quick search could not find
(sorry).

> 
> The concerns were:
> - Adding info on how to test and debug the code to the commit message.
> - Reverting the "Blocked by OS ..." field in tpm_ppi_info[].
> - Reverting the conditional in tpm_show_tcg/vs_operations()
> and sending in a separate patch with the change. The change was
> concerning displaying the "Not Implemented" status by updating
> `ret > 0` to `ret >= 0` in the if statement.
> 
> Please let me know what you think.
> 
> Thanks again,
> Denis
> 

BR, Jarkko

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

[Denis Aleksandrov]

On Wed, Sep 10, 2025 at 1:09 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Tue, Sep 02, 2025 at 03:12:45PM -0400, Denis Aleksandrov wrote:
> > Great, thank you Jarkko!
> >
> > Could you please take a look at the most recent reply from Paul in the
> > v2 thread for this patch?
> > Should I move forward with the advised changes and send a v5?
>
> Mind passing me the lore link? At least quick search could not find
> (sorry).
>

Here is the link:
https://lore.kernel.org/linux-integrity/20250827022102.17731-1-daleksan@redhat.com/t/#u
My apologies for not including it earlier.

> >
> > The concerns were:
> > - Adding info on how to test and debug the code to the commit message.
> > - Reverting the "Blocked by OS ..." field in tpm_ppi_info[].
> > - Reverting the conditional in tpm_show_tcg/vs_operations()
> > and sending in a separate patch with the change. The change was
> > concerning displaying the "Not Implemented" status by updating
> > `ret > 0` to `ret >= 0` in the if statement.

To be more clear on the last point, the suggestion was to revert this change
and send it as a separate patch.

> >
> > Please let me know what you think.
> >
> > Thanks again,
> > Denis
> >
>
> BR, Jarkko
>

Thanks,
Denis

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

[Jarkko Sakkinen]

On Wed, Sep 10, 2025 at 01:28:19PM -0400, Denis Aleksandrov wrote:
> On Wed, Sep 10, 2025 at 1:09 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Tue, Sep 02, 2025 at 03:12:45PM -0400, Denis Aleksandrov wrote:
> > > Great, thank you Jarkko!
> > >
> > > Could you please take a look at the most recent reply from Paul in the
> > > v2 thread for this patch?
> > > Should I move forward with the advised changes and send a v5?
> >
> > Mind passing me the lore link? At least quick search could not find
> > (sorry).
> >
> 
> Here is the link:
> https://lore.kernel.org/linux-integrity/20250827022102.17731-1-daleksan@redhat.com/t/#u
> My apologies for not including it earlier.

Here's what I have in my tree right now:

https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=18aada46edf7f79dba1ab89a100d0fc350d4a5c8

Let's take this is in simpler terms: is there something wrong in the
commit?

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue, Sep 02, 2025 at 10:24:30AM -0400, Denis Aleksandrov wrote:
> 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.
> 
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
> 
> Changes in v4:
> 	- Removes empty lines.
> 	- Reorders vars to reverse christmas tree.
> 
>  drivers/char/tpm/tpm_ppi.c | 85 +++++++++++++++++++++++++++-----------
>  1 file changed, 62 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> index d53fce1c9d6f..df34b215440d 100644
> --- a/drivers/char/tpm/tpm_ppi.c
> +++ b/drivers/char/tpm/tpm_ppi.c
> @@ -33,6 +33,20 @@ 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 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 +291,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 +299,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;
> @@ -324,9 +325,28 @@ static ssize_t tpm_show_ppi_tcg_operations(struct device *dev,
>  					   char *buf)
>  {
>  	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t len = 0;
> +	u32 ret;
> +	int i;
> +
> +	spin_lock(&tpm_ppi_lock);
> +	if (!ppi_cache_populated) {
> +		len = cache_ppi_operations(chip->acpi_dev_handle, buf);
> +		if (len < 0)
> +			return len;
>  
> -	return show_ppi_operations(chip->acpi_dev_handle, buf, 0,
> -				   PPI_TPM_REQ_MAX);
> +		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,
> @@ -334,9 +354,28 @@ static ssize_t tpm_show_ppi_vs_operations(struct device *dev,
>  					  char *buf)
>  {
>  	struct tpm_chip *chip = to_tpm_chip(dev);
> +	ssize_t len = 0;
> +	u32 ret;
> +	int i;
>  
> -	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);
> -- 
> 2.48.1
> 

I don't know how I messed up the patch in my Git but now it is good
(I think):

https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?h=next

Please check before I move forward with the PR.

BR, Jarkko

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

[Denis Aleksandrov]

On Mon, Sep 15, 2025 at 2:39 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Tue, Sep 02, 2025 at 10:24:30AM -0400, Denis Aleksandrov wrote:
> > 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.
> >
> > Reported-by: Jan Stancek <jstancek@redhat.com>
> > Signed-off-by: Denis Aleksandrov <daleksan@redhat.com>
> > Suggested-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > ---
> >
> > Changes in v4:
> >       - Removes empty lines.
> >       - Reorders vars to reverse christmas tree.
> >
> >  drivers/char/tpm/tpm_ppi.c | 85 +++++++++++++++++++++++++++-----------
> >  1 file changed, 62 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
> > index d53fce1c9d6f..df34b215440d 100644
> > --- a/drivers/char/tpm/tpm_ppi.c
> > +++ b/drivers/char/tpm/tpm_ppi.c
> > @@ -33,6 +33,20 @@ 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 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 +291,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 +299,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;
> > @@ -324,9 +325,28 @@ static ssize_t tpm_show_ppi_tcg_operations(struct device *dev,
> >                                          char *buf)
> >  {
> >       struct tpm_chip *chip = to_tpm_chip(dev);
> > +     ssize_t len = 0;
> > +     u32 ret;
> > +     int i;
> > +
> > +     spin_lock(&tpm_ppi_lock);
> > +     if (!ppi_cache_populated) {
> > +             len = cache_ppi_operations(chip->acpi_dev_handle, buf);
> > +             if (len < 0)
> > +                     return len;
> >
> > -     return show_ppi_operations(chip->acpi_dev_handle, buf, 0,
> > -                                PPI_TPM_REQ_MAX);
> > +             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,
> > @@ -334,9 +354,28 @@ static ssize_t tpm_show_ppi_vs_operations(struct device *dev,
> >                                         char *buf)
> >  {
> >       struct tpm_chip *chip = to_tpm_chip(dev);
> > +     ssize_t len = 0;
> > +     u32 ret;
> > +     int i;
> >
> > -     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);
> > --
> > 2.48.1
> >
>
> I don't know how I messed up the patch in my Git but now it is good
> (I think):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?h=next
>
> Please check before I move forward with the PR.

The commit looks good, besides my mistake of not freeing the lock upon an error.
I was just curious if we should follow Paul's recommendation about separating
the previously mentioned changes into another patch. I'm okay with moving
forward with the current changes without breaking it up into another patch.

I'll send over a v5 shortly. The only changes I will introduce to the
existing patch
is proper lock management.

>
> BR, Jarkko
>

Thanks,
Denis