linux-kernel - Re: [PATCH v2 1/4] tpm: Use a threaded interrupt handler

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]

Date:   Thu, 29 Apr 2021 09:58:39 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Lino Sanfilippo <LinoSanfilippo@....de>
Cc:     peterhuewe@....de, jgg@...pe.ca, stefanb@...ux.vnet.ibm.com,
        James.Bottomley@...senpartnership.com, keescook@...omium.org,
        jsnitsel@...hat.com, ml.linux@...oe.vision,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] tpm: Use a threaded interrupt handler

On Thu, Apr 29, 2021 at 12:37:40AM +0200, Lino Sanfilippo wrote:
> 
> Hi,
> 
> On 28.04.21 at 01:53, Jarkko Sakkinen wrote:
> > On Mon, Apr 26, 2021 at 01:47:17AM +0200, Lino Sanfilippo wrote:
> >> Interrupt handling at least includes reading and writing the interrupt
> >> status register from the interrupt routine. However over SPI those accesses
> >> require a sleepable context, since a mutex is used in the concerning
> >> functions.
> >> For this reason request a threaded interrupt handler which is running in
> >> (sleepable) process context.
> >>
> >> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@....de>
> >> ---
> >>  drivers/char/tpm/tpm_tis_core.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> index e7d1eab..0959559 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -781,8 +781,10 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
> >>  	int rc;
> >>  	u32 int_status;
> >>
> >> -	if (devm_request_irq(chip->dev.parent, irq, tis_int_handler, flags,
> >> -			     dev_name(&chip->dev), chip) != 0) {
> >> +
> >> +	if (devm_request_threaded_irq(chip->dev.parent, irq, NULL,
> >> +				      tis_int_handler, IRQF_ONESHOT | flags,
> >> +				      dev_name(&chip->dev), chip) != 0) {
> >>  		dev_info(&chip->dev, "Unable to request irq: %d for probe\n",
> >>  			 irq);
> >>  		return -1;
> >> --
> >> 2.7.4
> >>
> >
> > Why?
> >
> > https://elixir.bootlin.com/linux/v5.12/source/drivers/char/tpm/tpm_tis_core.c#L670
> >
> > I don't see anything that sleeps there.
> >
> > /Jarkko1
> >
> 
> The problem are the register read/write functions which we use to access the status register in
> the interrupt handler. In case of SPI they result in taking the spi_bus_lock which is a mutex.
> 
> E.g tpm_tis_spi_read32: tpm_tis_spi_read_bytes->tpm_tis_spi_transfer->spi_bus_lock->mutex_lock
> 
> Using a threaded interrupt handler seemed to me the easiest way to avoid this issue.
> 
> Regards,
> Lino
> 
> 
> 

This is a sentence that you should delete:

"However over SPI those accesses require a sleepable context, since a
mutex is used in the concerning functions.  "

It neither explains anything who and why sort of stuff.

Why don't you put intead something like

"Inside tpm_int_handler(), tpm_tis_read32() and tpm_tis_write32() are
invoked. The SPI subsystem requires mutex for I/O, which means that the
calls ought not to be used inside interrupt context."

(I did not check typos). Generally speaking, commit message is as, if not
more important than the code change.

/Jarkko