| 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: <ur6ype3ki6sxmpbzh53vfoewhq5oiqucioz3nwbdcda2vkaut7@sjt2hr33j3xk>
Date: Mon, 22 May 2023 15:44:54 -0700
From: Jerry Snitselaar <jsnitsel@...hat.com>
To: Lino Sanfilippo <LinoSanfilippo@....de>
Cc: peterhuewe@....de, jarkko@...nel.org, jgg@...pe.ca,
hdegoede@...hat.com, oe-lkp@...ts.linux.dev, lkp@...el.com,
peter.ujfalusi@...ux.intel.com, peterz@...radead.org,
linux@...ewoehner.de, linux-integrity@...r.kernel.org,
linux-kernel@...r.kernel.org, l.sanfilippo@...bus.com,
lukas@...ner.de, p.rosenberger@...bus.com
Subject: Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm
On Mon, May 22, 2023 at 04:31:04PM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@...bus.com>
>
> Commit e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") enabled
> interrupts instead of polling on all capable TPMs. Unfortunately, on some
> products the interrupt line is either never asserted or never deasserted.
>
> The former causes interrupt timeouts and is detected by
> tpm_tis_core_init(). The latter results in interrupt storms.
>
> Recent reports concern the Lenovo ThinkStation P360 Tiny, Lenovo ThinkPad
> L490 and Inspur NF5180M6:
>
> https://lore.kernel.org/linux-integrity/20230511005403.24689-1-jsnitsel@redhat.com/
> https://lore.kernel.org/linux-integrity/d80b180a569a9f068d3a2614f062cfa3a78af5a6.camel@kernel.org/
>
> The current approach to avoid those storms is to disable interrupts by
> adding a DMI quirk for the concerned device.
>
> However this is a maintenance burden in the long run, so use a generic
> approach:
>
> Detect an interrupt storm by counting the number of unhandled interrupts
> within a 10 ms time interval. In case that more than 1000 were unhandled
> deactivate interrupts, deregister the handler and fall back to polling.
>
> This equals the implementation that handles interrupt storms in
> note_interrupt() by means of timestamps and counters in struct irq_desc.
> However the function to access this structure is private so the logic has
> to be reimplemented in the TPM TIS core.
>
> Since handler deregistration would deadlock from within the interrupt
> routine trigger a worker thread that executes the unregistration.
>
> Suggested-by: Lukas Wunner <lukas@...ner.de>
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@...bus.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@...hat.com>
> ---
> drivers/char/tpm/tpm_tis_core.c | 71 +++++++++++++++++++++++++++++++--
> drivers/char/tpm/tpm_tis_core.h | 6 +++
> 2 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 558144fa707a..458ebf8c2f16 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -752,6 +752,55 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
> return status == TPM_STS_COMMAND_READY;
> }
>
> +static void tpm_tis_handle_irq_storm(struct tpm_chip *chip)
> +{
> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> + int intmask = 0;
> +
> + dev_err(&chip->dev, HW_ERR
> + "TPM interrupt storm detected, polling instead\n");
> +
> + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> +
> + intmask &= ~TPM_GLOBAL_INT_ENABLE;
> +
> + tpm_tis_request_locality(chip, 0);
> + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> + tpm_tis_relinquish_locality(chip, 0);
> +
> + chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +
> + /*
> + * We must not call devm_free_irq() from within the interrupt handler,
> + * since this function waits for running interrupt handlers to finish
> + * and thus it would deadlock. Instead trigger a worker that does the
> + * unregistration.
> + */
> + schedule_work(&priv->free_irq_work);
> +}
> +
> +static void tpm_tis_process_unhandled_interrupt(struct tpm_chip *chip)
> +{
> + const unsigned int MAX_UNHANDLED_IRQS = 1000;
> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> + /*
> + * The worker to free the TPM interrupt (free_irq_work) may already
> + * be scheduled, so make sure it is not scheduled again.
> + */
> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> + return;
> +
> + if (time_after(jiffies, priv->last_unhandled_irq + HZ/10))
> + priv->unhandled_irqs = 1;
> + else
> + priv->unhandled_irqs++;
> +
> + priv->last_unhandled_irq = jiffies;
> +
> + if (priv->unhandled_irqs > MAX_UNHANDLED_IRQS)
> + tpm_tis_handle_irq_storm(chip);
> +}
> +
> static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> {
> struct tpm_chip *chip = dev_id;
> @@ -761,10 +810,10 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
>
> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
> if (rc < 0)
> - return IRQ_NONE;
> + goto unhandled;
>
> if (interrupt == 0)
> - return IRQ_NONE;
> + goto unhandled;
>
> set_bit(TPM_TIS_IRQ_TESTED, &priv->flags);
> if (interrupt & TPM_INTF_DATA_AVAIL_INT)
> @@ -780,10 +829,14 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), interrupt);
> tpm_tis_relinquish_locality(chip, 0);
> if (rc < 0)
> - return IRQ_NONE;
> + goto unhandled;
>
> tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
> return IRQ_HANDLED;
> +
> +unhandled:
> + tpm_tis_process_unhandled_interrupt(chip);
> + return IRQ_HANDLED;
> }
>
> static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
> @@ -804,6 +857,15 @@ static void tpm_tis_gen_interrupt(struct tpm_chip *chip)
> chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> }
>
> +static void tpm_tis_free_irq_func(struct work_struct *work)
> +{
> + struct tpm_tis_data *priv = container_of(work, typeof(*priv), free_irq_work);
> + struct tpm_chip *chip = priv->chip;
> +
> + devm_free_irq(chip->dev.parent, priv->irq, chip);
> + priv->irq = 0;
> +}
> +
> /* Register the IRQ and issue a command that will cause an interrupt. If an
> * irq is seen then leave the chip setup for IRQ operation, otherwise reverse
> * everything and leave in polling mode. Returns 0 on success.
> @@ -816,6 +878,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> int rc;
> u32 int_status;
>
> + INIT_WORK(&priv->free_irq_work, tpm_tis_free_irq_func);
>
> rc = devm_request_threaded_irq(chip->dev.parent, irq, NULL,
> tis_int_handler, IRQF_ONESHOT | flags,
> @@ -918,6 +981,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
> interrupt = 0;
>
> tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
> + flush_work(&priv->free_irq_work);
>
> tpm_tis_clkrun_enable(chip, false);
>
> @@ -1021,6 +1085,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> chip->timeout_b = msecs_to_jiffies(TIS_TIMEOUT_B_MAX);
> chip->timeout_c = msecs_to_jiffies(TIS_TIMEOUT_C_MAX);
> chip->timeout_d = msecs_to_jiffies(TIS_TIMEOUT_D_MAX);
> + priv->chip = chip;
> priv->timeout_min = TPM_TIMEOUT_USECS_MIN;
> priv->timeout_max = TPM_TIMEOUT_USECS_MAX;
> priv->phy_ops = phy_ops;
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index e978f457fd4d..6fc86baa4398 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -91,12 +91,18 @@ enum tpm_tis_flags {
> };
>
> struct tpm_tis_data {
> + struct tpm_chip *chip;
> u16 manufacturer_id;
> struct mutex locality_count_mutex;
> unsigned int locality_count;
> int locality;
> + /* Interrupts */
> int irq;
> + struct work_struct free_irq_work;
> + unsigned long last_unhandled_irq;
> + unsigned int unhandled_irqs;
> unsigned int int_mask;
> +
> unsigned long flags;
> void __iomem *ilb_base_addr;
> u16 clkrun_enabled;
>
> base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
> --
> 2.40.1
>
Powered by blists - more mailing lists