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: <20170117232207.GA12127@obsidianresearch.com>
Date:   Tue, 17 Jan 2017 16:22:07 -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 03:00:53PM -0800, Andrey Pronin wrote:

> Here I was talking not about tpm_tis_spi or tpm_tis. Those can
> continue relying on the core, or register the default handler using
> .shutdown = tpm_shutdown.  I'm talking about the driver like
> tpm_i2c_infineon, which although uses the core, is created for a
> specific family of chips. So, it can assume that it needs to send
> vendor-specific commands.

But this is exactly what I'm talking about, infineon chips come in
lots of interface types, and if their legacy i2c interface need a
vendor command then likely their chips that use a common interface do
as well.

Conflating interface and bus is something I have been ripping out over
the years..

> > So, the core should detect chip XYZ and then issue the required
> > vendor-specific command in some way.
> 
> Do I get it right that you propose to create this new core tpm
> mechanism for handling shutdowns? I didn't find anything that'd
> allow to call vendor-specifc hooks from the core during shutdown,
> but may be I'm missing something.

It would be simple to add to the core path:

if (chip->id == 1234)
    tpm_vendor_1234_shutdown(...);

We don't need to involve the driver in this. If it becomes a very
complex thing down then road then we may need *bus* and *chip*
drivers, but for now the 'chip' driver(s) are just inlined in the core
code..

But if there is no actual need to do this right now then don't worry
about overdesigning things..

> > Probably the *best* thing would be to add shutdown to 'struct class'
> > in the driver core like suspend/resume?
> 
> Yes, if that could be added to struct class, and then device_shutdown()
> would call the class suspend, if the driver suspend is NULL, that'd
> solve it.

Won't know if it is possible until someone sends a patch to Greg/etc :)

> Then the core can register the default shutdown in class, and an
> individual access driver can override it by registering its own
> shutdown callback. Still, due to the ordering issues discussed
> above, it should be either/or, not first-driver-then-class, if both
> exist.

First class then driver is the usual model, which is OK for TPM.

> So, we still need to export the common tpm_shutdown().

No need to export if no driver is calling it, like I said don't
overdesign here, it is trivial to change if someone needs to do
something different later.

> to start with that: create and export the common tpm_shutdown() from
> the core, that send Shutdown(CLEAR) for 2.0 and null-ifies chip->ops
> (and fix sysfs to acquire chip->ops_sem) and let the interested drivers
> call it.

I think you should do this and use the reboot_notifier or
class->shutdown approach

I'm not completely sure why you are worrying about sending a
vendor-specific command at shutdown. Do you actually need that?

Jason