| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aF0nis6M5ddf668S@kernel.org>
Date: Thu, 26 Jun 2025 13:57:14 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Prachotan Bathi <prachotan.bathi@....com>
Cc: Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
Stuart Yoder <stuart.yoder@....com>,
linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/2] tpm_crb_ffa: handle tpm busy return code
On Wed, Jun 25, 2025 at 02:19:43PM -0500, Prachotan Bathi wrote:
> Platforms supporting direct message request v2 [1] can support secure
> partitions that support multiple services. For CRB over FF-A interface,
> if the firmware TPM or TPM service [1] shares its Secure Partition (SP)
> with another service, message requests may fail with a -EBUSY error.
>
> To handle this, replace the single check and call with a retry loop
> that attempts the TPM message send operation until it succeeds or a
> configurable timeout is reached. Implement a _try_send_receive function
> to do a single send/receive and modify the existing send_receive to
> add this retry loop.
> The retry mechanism introduces a module parameter (`busy_timeout_ms`,
> default: 2000ms) to control how long to keep retrying on -EBUSY
> responses. Between retries, the code waits briefly (50-100 microseconds)
> to avoid busy-waiting and handling TPM BUSY conditions more gracefully.
>
> The parameter can be modified at run-time as such:
> echo 3000 | tee /sys/module/tpm_crb_ffa/parameters/busy_timeout_ms
> This changes the timeout from the default 2000ms to 3000ms.
>
> [1] TPM Service Command Response Buffer Interface Over FF-A
> https://developer.arm.com/documentation/den0138/latest/
>
> Signed-off-by: Prachotan Bathi <prachotan.bathi@....com>
> ---
> .../admin-guide/kernel-parameters.txt | 8 +++
> drivers/char/tpm/tpm_crb_ffa.c | 65 +++++++++++++++----
> 2 files changed, 60 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f1f2c0874da9..5cb5dbfd761f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -7214,6 +7214,14 @@
> causing a major performance hit, and the space where
> machines are deployed is by other means guarded.
>
> + tpm_crb_ffa.busy_timeout_ms= [ARM64,TPM]
> + Maximum time in milliseconds to retry sending a message
> + to the TPM service before giving up. This parameter controls
> + how long the system will continue retrying when the TPM
> + service is busy.
> + Format: <unsigned int>
> + Default: 2000 (2 seconds)
> +
> tpm_suspend_pcr=[HW,TPM]
> Format: integer pcr id
> Specify that at suspend time, the tpm driver
> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> index 089d1e54bb46..1a51c966e42c 100644
> --- a/drivers/char/tpm/tpm_crb_ffa.c
> +++ b/drivers/char/tpm/tpm_crb_ffa.c
> @@ -10,8 +10,12 @@
> #define pr_fmt(fmt) "CRB_FFA: " fmt
>
> #include <linux/arm_ffa.h>
> +#include <linux/delay.h>
> +#include <linux/moduleparam.h>
> #include "tpm_crb_ffa.h"
>
> +#define memzero(s, n) memset((s), 0, (n))
This is a spurious change i.e., out of scope of the topic.
If you wanted to do this, you'd need to have a separate patch for it.
Otherwise, stick to memset(). This is the "split to logical changes"
part of SubmittingPatches document.
> +
> /* TPM service function status codes */
> #define CRB_FFA_OK 0x05000001
> #define CRB_FFA_OK_RESULTS_RETURNED 0x05000002
> @@ -178,22 +182,18 @@ int tpm_crb_ffa_init(void)
> }
> EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
>
> -static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> - unsigned long a0,
> - unsigned long a1,
> - unsigned long a2)
> +static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> + unsigned long a0, unsigned long a1,
> + unsigned long a2)
> {
> const struct ffa_msg_ops *msg_ops;
> int ret;
>
> - if (!tpm_crb_ffa)
> - return -ENOENT;
> -
> msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
>
> if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> - memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
> - sizeof(struct ffa_send_direct_data2));
> + memzero(&tpm_crb_ffa->direct_msg_data2,
> + sizeof(struct ffa_send_direct_data2));
>
> tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> @@ -201,12 +201,12 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> tpm_crb_ffa->direct_msg_data2.data[3] = a2;
>
> ret = msg_ops->sync_send_receive2(tpm_crb_ffa->ffa_dev,
> - &tpm_crb_ffa->direct_msg_data2);
> + &tpm_crb_ffa->direct_msg_data2);
> if (!ret)
> ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
> } else {
> - memset(&tpm_crb_ffa->direct_msg_data, 0x00,
> - sizeof(struct ffa_send_direct_data));
> + memzero(&tpm_crb_ffa->direct_msg_data,
> + sizeof(struct ffa_send_direct_data));
>
> tpm_crb_ffa->direct_msg_data.data1 = func_id;
> tpm_crb_ffa->direct_msg_data.data2 = a0;
> @@ -214,11 +214,50 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> tpm_crb_ffa->direct_msg_data.data4 = a2;
>
> ret = msg_ops->sync_send_receive(tpm_crb_ffa->ffa_dev,
> - &tpm_crb_ffa->direct_msg_data);
> + &tpm_crb_ffa->direct_msg_data);
> if (!ret)
> ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1);
> }
>
> + return ret;
> +}
> +
> +static unsigned int busy_timeout_ms = 2000;
> +/**
> + * busy_timeout_ms - Maximum time to retry before giving up on busy
> + *
> + * This parameter defines the maximum time in milliseconds to retry
> + * sending a message to the TPM service before giving up.
I'd keep this single line and save the effort to kernel-parameters.rst.
Otherwise, this will be pain to maintain.
> + */
> +module_param(busy_timeout_ms, uint, 0644);
> +MODULE_PARM_DESC(busy_timeout_ms,
> + "Maximum time(in ms) to retry before giving up on busy");
"Maximum time (in ms)", or rather "Maximum time in ms" because you
really need to know the unit every single time you use the parameter.
> +
> +static int __tpm_crb_ffa_send_receive(unsigned long func_id, unsigned long a0,
> + unsigned long a1, unsigned long a2)
> +{
> + ktime_t start, stop;
> + int ret;
> +
> + if (!tpm_crb_ffa)
> + return -ENOENT;
> +
> + start = ktime_get();
> + stop = ktime_add(start, ms_to_ktime(busy_timeout_ms));
> +
> + for (;;) {
> + ret = __tpm_crb_ffa_try_send_receive(func_id, a0, a1, a2);
> +
> + if (ret != -EBUSY)
> + break;
> +
> + usleep_range(50, 100);
> + if (ktime_after(ktime_get(), stop)) {
> + dev_warn(&tpm_crb_ffa->ffa_dev->dev,
> + "Busy retry timed out\n");
> + break;
> + }
> + }
>
> return ret;
> }
> --
> 2.43.0
>
BR, Jarkko
Powered by blists - more mailing lists