linux-kernel - Re: [PATCH v3 0/4] Fixes for TPM interrupt handling

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, 24 Mar 2022 18:04:23 +0100
From:   Michael Niewöhner <linux@...ewoehner.de>
To:     Jarkko Sakkinen <jarkko@...nel.org>,
        Lino Sanfilippo <LinoSanfilippo@....de>
Cc:     peterhuewe@....de, jgg@...pe.ca, stefanb@...ux.vnet.ibm.com,
        stefanb@...ux.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,
        twawrzynczak@...omium.org
Subject: Re: [PATCH v3 0/4] Fixes for TPM interrupt handling

Hi guys,

On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote:
> On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote:
> > Hi,
> > 
> > On 03.05.21 at 17:50, Jarkko Sakkinen wrote:
> > > What the heck is "simplification" and what that has to do with fixing
> > > anything? I don't understand your terminology.
> > 
> > 
> > The intention for this patch is not to fix anything. Please read the cover
> > letter and the commit message.
> > This patch is about making the locality handling easier by not
> > claiming/releasing
> > it multiple times over the driver life time, but claiming it once at driver
> > startup and only releasing it at driver shutdown.
> > 
> > Right now we have locality request/release combos in
> > 
> > - probe_itpm()
> > - tpm_tis_gen_interrupt()
> > - tpm_tis_core_init()
> > - tpm_chip_start()
> > 
> > and there is still one combo missing for
> > 
> > - tpm2_get_timeouts()
> > 
> > which is the reason why we get the "TPM returned invalid status" bug in case
> > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch,
> > see
> > below).
> > 
> > And if we are going to enable interrupts, we have to introduce yet another
> > combo,
> > for accessing the status register in the interrupt handler, since TPM 2.0
> > requires holding the locality for writing to the status register. That makes
> > 6 different code places in which we take and release the locality.
> > 
> > With this patch applied we only take the locality at one place. Furthermore
> > with interrupts enabled we dont have to claim the locality for each handler
> > execution, saving us countless claim/release combinations at runtime.
> > 
> > Hence the term "simplification" which is perfectly justified IMO.
> > 
> > So again, this patch is "only" in preparation for the next patch when
> > interrupts
> > are actually enabled and we would have to take the locality in the interrupt
> > handler without this patch.
> 
> So: what problem this patch does solve?
> 
> /Jarkko
> 

first, thank you very much, Lino, for working on this! I've been debugging
issues with the tis driver in the last days and was about to start with the same
approach as yours when I luckily discovered your patch!

Jarkko, while I agree, that the commit message is not optimal, Lino tried hard
to explain what the problems with the current code are and how they are / can be
fixed. Further, I too don't see why simplification / optimization is such a bad
thing. This driver is actually a very good example. I had a hard time, too,
figuring out what's going on there. A clean rewrite is a very valid approach
here IMO. It's not "polishing for nothing", as you described it, but actually
solving problems.

Interrupt detection is broken for years now and finally a volunteer worked on a
solution. Don't you think this should be valued? Let's get this problem sorted
out :-)

Lino, I'd be happy to test the patches, when you have time and interest to work
on this again!

Thanks, Michael