| 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: <be1f990324293fd09ae29b07a9d1bdcbf6a21bd5.camel@mniewoehner.de>
Date: Wed, 18 May 2022 18:08:30 +0200
From: Michael Niewöhner <linux@...ewoehner.de>
To: Jarkko Sakkinen <jarkko@...nel.org>,
Lino Sanfilippo <LinoSanfilippo@....de>
Cc: peterhuewe@....de, jgg@...pe.ca, stefanb@...ux.vnet.ibm.com,
linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org,
lukas@...ner.de, p.rosenberger@...bus.com,
Lino Sanfilippo <l.sanfilippo@...bus.com>
Subject: Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send()
to tpm_tis_probe_irq_single()
On Wed, 2022-05-18 at 04:26 +0300, Jarkko Sakkinen wrote:
> On Tue, 2022-05-17 at 20:19 +0200, Michael Niewöhner wrote:
> > Hi guys,
> >
> >
> > On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote:
> > > On 16.05.22 at 19:51, Jarkko Sakkinen wrote:
> > > > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote:
> > > > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
> > > > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
> > > > > > > From: Lino Sanfilippo <l.sanfilippo@...bus.com>
> > > > > > >
> > > > > > > There is no need to check for the irq test completion at each data
> > > > > > > transmission during the driver livetime. Instead do the check only
> > > > > > > once at
> > > > > > > driver startup.
> > > > > > >
> > > > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@...bus.com>
> > > > > > > ---
> > > > > > > drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++-----------------
> > > > > > > ----
> > > > > > > -
> > > > > > > 1 file changed, 22 insertions(+), 46 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > > > > > b/drivers/char/tpm/tpm_tis_core.c
> > > > > > > index bdfde1cd71fe..4c65718feb7d 100644
> > > > > > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip
> > > > > > > *chip)
> > > > > > > * tpm.c can skip polling for the data to be available as the
> > > > > > > interrupt is
> > > > > > > * waited for here
> > > > > > > */
> > > > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8
> > > > > > > *buf,
> > > > > > > size_t len)
> > > > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t
> > > > > > > len)
> > > > > > > {
> > > > > > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > > > int rc;
> > > > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip
> > > > > > > *chip, const u8 *buf, size_t len)
> > > > > > > return rc;
> > > > > > > }
> > > > > > >
> > > > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t
> > > > > > > len)
> > > > > > > -{
> > > > > > > - int rc, irq;
> > > > > > > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > > > -
> > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> > > > > > > - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > - return tpm_tis_send_main(chip, buf, len);
> > > > > > > -
> > > > > > > - /* Verify receipt of the expected IRQ */
> > > > > > > - irq = priv->irq;
> > > > > > > - priv->irq = 0;
> > > > > > > - chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> > > > > > > - rc = tpm_tis_send_main(chip, buf, len);
> > > > > > > - priv->irq = irq;
> > > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > - tpm_msleep(1);
> > > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > - disable_interrupts(chip);
> > > > > > > - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > > > - return rc;
> > > > > > > -}
> > > > > > > -
> > > > > > > struct tis_vendor_durations_override {
> > > > > > > u32 did_vid;
> > > > > > > struct tpm1_version version;
> > > > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct
> > > > > > > tpm_chip *chip, u32 intmask,
> > > > > > >
> > > > > > > rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > > > &original_int_vec);
> > > > > > > - if (rc < 0)
> > > > > > > + if (rc < 0) {
> > > > > > > + disable_interrupts(chip);
> > > > > > > return rc;
> > > > > > > + }
> > > > > > >
> > > > > > > rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > > > irq);
> > > > > > > if (rc < 0)
> > > > > > > - return rc;
> > > > > > > + goto out_err;
> > > > > > >
> > > > > > > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality),
> > > > > > > &int_status);
> > > > > > > if (rc < 0)
> > > > > > > - return rc;
> > > > > > > + goto out_err;
> > > > > > >
> > > > > > > /* Clear all existing */
> > > > > > > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality),
> > > > > > > int_status);
> > > > > > > if (rc < 0)
> > > > > > > - return rc;
> > > > > > > + goto out_err;
> > > > > > >
> > > > > > > /* Turn on */
> > > > > > > rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> > > > > > > intmask | TPM_GLOBAL_INT_ENABLE);
> > > > > > > if (rc < 0)
> > > > > > > - return rc;
> > > > > > > + goto out_err;
> > > > > > >
> > > > > > > clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > >
> > > > > > > /* Generate an interrupt by having the core call through
> > > > > > > to
> > > > > > > * tpm_tis_send
> > > > > > > */
> > > > > > > rc = tpm_tis_gen_interrupt(chip);
> > > > > > > if (rc < 0)
> > > > > > > - return rc;
> > > > > > > + goto out_err;
> > > > > > >
> > > > > > > - /* tpm_tis_send will either confirm the interrupt is
> > > > > > > working
> > > > > > > or it
> > > > > > > - * will call disable_irq which undoes all of the above.
> > > > > > > - */
> > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > > > - rc = tpm_tis_write8(priv, original_int_vec,
> > > > > > > - TPM_INT_VECTOR(priv->locality));
> > > > > > > - if (rc < 0)
> > > > > > > - return rc;
> > > > > > > + tpm_msleep(1);
> > > > > > >
> > > > > > > - return 1;
> > > > > > > - }
> > > > > > > + /* Verify receipt of the expected IRQ */
> > > > > > > + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > > > + goto out_err;
> > > > > > > +
> > > > > > > + chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > > >
> > > > > > > return 0;
> > > > > > > +
> > > > > > > +out_err:
> > > >
> > > > Rename this as just 'err'.
> > > >
> > > > > > > + disable_interrupts(chip);
> > > > > > > + tpm_tis_write8(priv, original_int_vec,
> > > > > > > TPM_INT_VECTOR(priv-
> > > > > > > > locality));
> > > > > > > +
> > > > > > > + return rc;
> > > > > > > }
> > > > > > >
> > > > > > > /* Try to find the IRQ the TPM is using. This is for legacy x86
> > > > > > > systems that
> > > > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev,
> > > > > > > struct tpm_tis_data *priv, int irq,
> > > > > > > if (irq) {
> > > > > > > tpm_tis_probe_irq_single(chip, intmask,
> > > > > > > IRQF_SHARED,
> > > > > > > irq);
> > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> > > > > > > dev_err(&chip->dev, FW_BUG
> > > > > > > "TPM interrupt not
> > > > > > > working,
> > > > > > > polling instead\n");
> > > > > > > -
> > > > > > > - disable_interrupts(chip);
> > > > > > > - }
> > > > > > > } else {
> > > > > > > tpm_tis_probe_irq(chip, intmask);
> > > > > > > }
> > > > > > > --
> > > > > > > 2.36.0
> > > > > > >
> > > > > >
> > > > > > For me this looks just code shuffling.
> > > > > >
> > > > > > I don't disagree but changing working code without actual semantical
> > > > > > reasons neither makes sense.
> > > > > >
> > > > > > BR, Jarkko
> > > > > >
> > > > >
> > > > > Well the semantical reason for this change is that the check for irq
> > > > > test
> > > > > completion
> > > > > only has to be done once for the driver livetime. There is no point in
> > > > > doing it
> > > > > over and over again for each transmission.
> > > > > So the code is not simply shuffled around, it is shifted to a place
> > > > > where
> > > > > it is only
> > > > > executed once.
> > > > >
> > > > > This is not a bugfix but it is clearly an improvement/cleanup. As far
> > > > > as I
> > > > > understood
> > > > > from your comments on the earlier versions of this patch set cleanups
> > > > > are
> > > > > also ok as
> > > > > long as they are not intermixed with bugfixes.
> > > >
> > > > The patch does not do anything particulary useful IMHO. There's no
> > > > stimulus to do this change.
> > > >
> >
> > I don't agree. IMHO preventing useless actions (like checking the interrupt
> > again and again) *is* useful and I think it's reason enough.
>
> Show me the test data to back this up.
Why do you need test data as an argument for not doing useless actions? o.O
>
> BR, Jarkko
Powered by blists - more mailing lists