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: <20170123203901.GA13029@obsidianresearch.com>
Date:   Mon, 23 Jan 2017 13:39:01 -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 Mon, Jan 23, 2017 at 12:02:46PM -0800, Andrey Pronin wrote:

> > But if there is no actual need to do this right now then don't worry
> > about overdesigning things..
> 
> OK, I can live with chip->id specific logic in probe/shutdown, if that's
> the current approach.

It is where we are heading.. If it becomes prevelent we will need chip
and bus drivers.

> > First class then driver is the usual model, which is OK for TPM.
> 
> If "first class then driver", then the already existing
> register_reboot_notifier() can play the role of the class handler,
> since those hooks are called before drivers' shutdown handlers.

Okay.. But maybe fire off patch doing this via the device code, as if
that is accepted it is better than the reboot handler in terms of
guaranteeing ordering/etc.

> 	down_write(&chip->ops_sem);
> 	if (chip->ops) {
> 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> 		chip->ops = NULL;
> 	}
> 	up_write(&chip->ops_sem);
> on shutdown in registered "reboot notifier".

Sure, seems like a good starting point.

> I went through the places that access chip->ops to see what's
> going on at the moment with protecting them with tpm_try_get_ops().
> Here is the current state:
> 
>  - tpm_transmit/tpm_transmit_cmd. Not exported and are only called
>    by the core after tpm_try_get_ops() except for one place in
>    sysfs - pubek_show().

Most of sysfs calls functions like tpm_pcr_read_dev, tpm_getcap, etc,
etc that boil down the tpm_transmit_cmd, so many more need it than you
listed.

> But it looks a bit brittle. So, before I submit my next patch:
> Do you think it's ok to leave wait_for_tpm_stat() and
> tpm_get_timeouts() and just continue be mindful when using those
> low-level functions?

For xenfront we should someday move it to use TPM_OPS_AUTO_STARTUP and
drop the tpm_get_timeouts.

I think it is fine to leave those two low level commands as is..

Like I said, it would be more robust to acquire a lock directly in
places like transmit_cmd, but I suspect that is too hard to retrofit
at this time...

> and checking for ops == NULL inside those low-level functions:
> tpm_transmit, wait_for_tpm_stat, tpm_get_timeout. It then should
> probably be separated from get_device(), which can be kept inside
> tpm_try_get_ops().

No sense checking for ops=null if you also can't guarentee the lock is
held, it is just confusing.

Jason