linux-kernel - Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

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: <b909aaee-3fff-4dca-40f4-4c5348474426@redhat.com>
Date:   Mon, 13 Apr 2020 12:04:25 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        linux-integrity@...r.kernel.org
Cc:     stable@...r.kernel.org, Peter Huewe <peterhuewe@....de>,
        Jason Gunthorpe <jgg@...pe.ca>, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tpm/tpm_tis: Free IRQ if probing fails

Hi Jarkko,

On 4/12/20 7:04 PM, Jarkko Sakkinen wrote:
> Call devm_free_irq() if we have to revert to polling in order not to
> unnecessarily reserve the IRQ for the life-cycle of the driver.
> 
> Cc: stable@...r.kernel.org # 4.5.x
> Reported-by: Hans de Goede <hdegoede@...hat.com>
> Fixes: e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> ---
>   drivers/char/tpm/tpm_tis_core.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 27c6ca031e23..ae6868e7b696 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1062,9 +1062,12 @@ 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");
> +				devm_free_irq(chip->dev.parent, priv->irq,
> +					      chip);
> +			}

My initial plan was actually to do something similar, but if the probe code
is actually ever fixed to work as intended again then this will lead to a
double free as then the IRQ-test path of tpm_tis_send() will have called
disable_interrupts() which already calls devm_free_irq().

You could check for chip->irq != 0 here to avoid that.

But it all is rather messy, which is why I went with the "#if 0" approach
in my patch.

Also we will currently ALWAYS hit the "TPM interrupt not working, polling instead"
error because the TPM_CHIP_FLAG_IRQ never gets set. So if we are going to do an
interim fix (and we should) we should really also silence that error.

Regards,

Hans

p.s.

I'm currently in contact with Lenovo trying to figure out what is going on
here with the always firing IRQ on the X1 8th gen, I guess the fix for that
might also help with the T490 issue.