linux-kernel - Re: [PATCH v5 2/5] tpm_crb: clean-up and refactor check for idle support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7c913fc0-0f2c-4327-99ee-510bdff8a537@arm.com>
Date: Thu, 20 Feb 2025 13:04:19 -0600
From: Stuart Yoder <stuart.yoder@....com>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-integrity@...r.kernel.org, peterhuewe@....de, jgg@...pe.ca,
 sudeep.holla@....com, rafael@...nel.org, lenb@...nel.org,
 linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/5] tpm_crb: clean-up and refactor check for idle
 support



On 2/20/25 3:29 AM, Jarkko Sakkinen wrote:
> On Wed, Feb 19, 2025 at 02:10:11PM -0600, Stuart Yoder wrote:
>> Refactor TPM idle check to tpm_crb_has_idle(), and reduce paraentheses
>> usage in start method checks
>>
>> Signed-off-by: Stuart Yoder <stuart.yoder@....com>
>> ---
>>   drivers/char/tpm/tpm_crb.c | 36 +++++++++++++++++++++---------------
>>   1 file changed, 21 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> index ea085b14ab7c..31db879f1324 100644
>> --- a/drivers/char/tpm/tpm_crb.c
>> +++ b/drivers/char/tpm/tpm_crb.c
>> @@ -115,6 +115,16 @@ struct tpm2_crb_pluton {
>>   	u64 reply_addr;
>>   };
>>   
>> +/*
>> + * Returns true if the start method supports idle.
>> + */
>> +static inline bool tpm_crb_has_idle(u32 start_method)
>> +{
>> +	return start_method == ACPI_TPM2_START_METHOD ||
>> +	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD ||
>> +	       start_method == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC;
>> +}
>> +
>>   static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value,
>>   				unsigned long timeout)
>>   {
>> @@ -173,9 +183,7 @@ static int __crb_go_idle(struct device *dev, struct crb_priv *priv)
>>   {
>>   	int rc;
>>   
>> -	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
>> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
>> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
>> +	if (!tpm_crb_has_idle(priv->sm))
>>   		return 0;
>>   
>>   	iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs_t->ctrl_req);
>> @@ -222,9 +230,7 @@ static int __crb_cmd_ready(struct device *dev, struct crb_priv *priv)
>>   {
>>   	int rc;
>>   
>> -	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
>> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) ||
>> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC))
>> +	if (!tpm_crb_has_idle(priv->sm))
>>   		return 0;
>>   
>>   	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req);
>> @@ -423,13 +429,13 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>   	 * report only ACPI start but in practice seems to require both
>>   	 * CRB start, hence invoking CRB start method if hid == MSFT0101.
>>   	 */
>> -	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
>> -	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED) ||
>> -	    (!strcmp(priv->hid, "MSFT0101")))
>> +	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
>> +	    priv->sm == ACPI_TPM2_MEMORY_MAPPED ||
>> +	    !strcmp(priv->hid, "MSFT0101"))
>>   		iowrite32(CRB_START_INVOKE, &priv->regs_t->ctrl_start);
>>   
>> -	if ((priv->sm == ACPI_TPM2_START_METHOD) ||
>> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD))
>> +	if (priv->sm == ACPI_TPM2_START_METHOD ||
>> +	    priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>>   		rc = crb_do_acpi_start(chip);
>>   
>>   	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
>> @@ -449,8 +455,8 @@ static void crb_cancel(struct tpm_chip *chip)
>>   
>>   	iowrite32(CRB_CANCEL_INVOKE, &priv->regs_t->ctrl_cancel);
>>   
>> -	if (((priv->sm == ACPI_TPM2_START_METHOD) ||
>> -	    (priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)) &&
>> +	if ((priv->sm == ACPI_TPM2_START_METHOD ||
>> +	     priv->sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD) &&
>>   	     crb_do_acpi_start(chip))
>>   		dev_err(&chip->dev, "ACPI Start failed\n");
>>   }
>> @@ -609,8 +615,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
>>   	 * the control area, as one nice sane region except for some older
>>   	 * stuff that puts the control area outside the ACPI IO region.
>>   	 */
>> -	if ((priv->sm == ACPI_TPM2_COMMAND_BUFFER) ||
>> -	    (priv->sm == ACPI_TPM2_MEMORY_MAPPED)) {
>> +	if (priv->sm == ACPI_TPM2_COMMAND_BUFFER ||
>> +	    priv->sm == ACPI_TPM2_MEMORY_MAPPED) {
>>   		if (iores &&
>>   		    buf->control_address == iores->start +
>>   		    sizeof(*priv->regs_h)) 
>> -- 
>> 2.34.1
>>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>

Thanks for the review.  Do you want me to respin and send out
a v6 with your Reviewed-by tags on patches 2/5 and 5/5?

Thanks,
Stuart