linux-kernel - Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm

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: <CSU4NMPD9WK6.YQ329KQG7PSF@suppilovahvero>
Date:   Wed, 24 May 2023 04:46:57 +0300
From:   "Jarkko Sakkinen" <jarkko@...nel.org>
To:     "Lukas Wunner" <lukas@...ner.de>
Cc:     "Lino Sanfilippo" <LinoSanfilippo@....de>, <peterhuewe@....de>,
        <jgg@...pe.ca>, <jsnitsel@...hat.com>, <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>,
        <p.rosenberger@...bus.com>
Subject: Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm

On Tue May 23, 2023 at 10:37 PM EEST, Lukas Wunner wrote:
> > > +unhandled:
> > > +	tpm_tis_process_unhandled_interrupt(chip);
> > > +	return IRQ_HANDLED;
> > 
> > Shouldn't the return value be IRQ_NONE?
>
> No, absolutely not.  If you return IRQ_NONE here then genirq code
> will increase the spurious interrupt counter.  That's bad because
> the IRQ storm detection tpm_tis_core.c would race with the IRQ storm
> detection in genirq code:
>
> Note that disablement of the interrupt must happen in a work_struct
> here to avoid a deadlock. (The deadlock would occur because
> devm_free_irq() waits for the interrupt handler to finish.)
>
> Now, let's say the 1000 unhandled interrupts limit has been reached
> and the work_struct is scheduled.  If the work_struct isn't run
> quickly enough, you may reach the 99900 limit in note_interrupt()
> (see kernel/irq/spurious.c) and then genirq code will force the
> interrupt off completely.
>
> To avoid that you *have* to return IRQ_HANDLED here and thus pretend
> towards genirq code that the interrupt was not spurious.

This would deserve an inline comment.

> > >  struct tpm_tis_data {
> > > +	struct tpm_chip *chip;
> > >  	u16 manufacturer_id;
> > >  	struct mutex locality_count_mutex;
> > >  	unsigned int locality_count;
> > >  	int locality;
> > > +	/* Interrupts */
> > 
> > Not relevant change for a bug fix.
>
> But not harmful either, is it?

No but it is still spurious change in this context.

> Thanks,
>
> Lukas

BR, Jarkko