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: <fdc24d5564b21cf554616afa94b008909ada27f0.camel@kernel.org>
Date: Thu, 20 Feb 2025 23:35:06 +0200
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Stuart Yoder <stuart.yoder@....com>
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 Thu, 2025-02-20 at 13:04 -0600, Stuart Yoder wrote:
> 
> 
> 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?

It's fine I can append them :-)

You might have to hold up until to the first work week of
March because I'm actually on holiday up until that but
yeah no need for extra noise.

Up until that tested-by's to this patch set version are
obviously welcome but it's now definitely good enough
as far as I'm concerned.

> 
> Thanks,
> Stuart

BR, Jarkko