linux-kernel - Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on shutdown

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: <20170117192728.GF27528@obsidianresearch.com>
Date:   Tue, 17 Jan 2017 12:27:28 -0700
From:   Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:     Andrey Pronin <apronin@...omium.org>
Cc:     Peter Huewe <peterhuewe@....de>,
        Marcel Selhorst <tpmdd@...horst.net>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        tpmdd-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        semenzato@...omium.org, groeck@...omium.org
Subject: Re: [PATCH] tpm/tpm_i2c_infineon: ensure no ongoing commands on
 shutdown

On Tue, Jan 17, 2017 at 09:58:27AM -0800, Andrey Pronin wrote:
> > Yes, sorry, I should have mentioned that.. Maybe that is too much to
> > fix..
> 
> If we fix sysfs to go through tpm_try_get_ops, then all we can do for
> shutdown is indeed something like

Maybe yes, I also had at one point a thought to push the read side of
the ops_sem all the way down to the transmit_cmd level... But that
complicates calling shutdown.

> 	down_write(&chip->ops_sem);
> 	if (chip->ops && chip->flags & TPM_CHIP_FLAG_TPM2)
> 		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 	chip->ops = NULL;
> 	up_write(&chip->ops_sem);
> 
> Does that sound like a good plan?
> If we don't want sysfs to increment/decrement the reference count for
> the device, we can still make it go through

Grabbing the extra kref is harmless..

> > I'm confused - doesn't your system reset the TPM when it reboots?
> > Isn't that required so the firmware starts with known PCRs? Doesn't
> > reset trump unorderly shutdown?
> > 
> 
> That's right, the TPM is reset when the system reboots. However, for
> TPM 2.0, if it resets w/o Shutdown(CLEAR) first, it will detect it
> during Startup, and mark as unorderly shutdown. Shutdown(CLEAR) is
> the signal to the TPM to save its state to nvram and prepare to reset.
> If it was not done, it is possible that something was not saved (e.g.
> the DA counter), and the chip correctly marks it as a potential DA
> problem.

Okay, that makes sense, and needs to go in a comment someplace!

> > > All these things are handled by tpm_chip_unregister(). I thought about
> > > creating a tpm_chip_shutdown routine that could be called from shutdown
> > > handlers of the drivers that need it (and I'd do it for every driver,
> > > especially in 2.0 case). But decided that it's better to reuse the
> > > existing tpm_chip_unregister() that already does what's needed.
> > 
> > If for some reason we need this for every driver then this is probably
> > a better approach - but that seems very, very strange to me.
> 
> As described above, we can do a smaller tpm_chip_shutdown() that the
> drivers that need it (2.0 or susceptible to issues if reset in the
> middle of command) can call.
> I'll do it, if it sounds like the right plan to you.

Yes please..

Is there some way we can have the TPM core do this without requiring
the driver to add a shutdown the struct driver?

Maybe we could put something in chip->dev->driver? Not sure..

Jason