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]

Date:   Tue, 30 May 2023 12:31:47 +0200
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Jarkko Sakkinen <jarkko@...nel.org>, peterhuewe@....de,
        jgg@...pe.ca
Cc:     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, lukas@...ner.de, p.rosenberger@...bus.com
Subject: Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm

On 24.05.23 at 17:30, Jarkko Sakkinen wrote:
> On Wed May 24, 2023 at 6:58 AM EEST, Jarkko Sakkinen wrote:
>>>  	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;
>>>  }
>
> Some minor glitches I noticed.
>
> You could simplify the flow by making the helper to return IRQ_NONE.
>
> E.g.
>
> 	tpm_tis_relinquish_locality(chip, 0);
> 	if (rc < 0)
> 		return tpm_tis_process_unhandled_interrupt(chip);
>
> I'd recommend changing the function name simply tpm_tis_rollback_interrupt().
>

IMHO this name is worse, since this function does actually _not_ rollback interrupts
most of the times it is called. Only after an interrupt storm is detected (so currently
after it has been called at least 1000 times without rollback) it actually
rolls back interrupts and falls back to polling.

Maybe rather tpm_tis_check_for_interrupt_storm()?

Regards,
Lino