linux-kernel - Re: [PATCH] tpm: fix a race condition tpm2_unseal

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: <20160721090245.GA7999@intel.com>
Date:	Thu, 21 Jul 2016 12:02:45 +0300
From:	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
Cc:	Peter Huewe <peterhuewe@....de>,
	linux-security-module@...r.kernel.org,
	Marcel Selhorst <tpmdd@...horst.net>,
	"moderated list:TPM DEVICE DRIVER" 
	<tpmdd-devel@...ts.sourceforge.net>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> 
> > The only use cases I see at the moment for it work this way:
> > 
> > 1. Call tpm_try_get_ops.
> > 2. Send a TPM command.
> > 3. Call tpm_put_ops.
> 
> Right, but that is just a reflection of what the in kernel users are
> doing today, not necessarily what they should be doing.
> 
> We should not break the put/get semantics..
> 
> > I did not find any other form of use. The only use is to make sure that
> > there are no transactions running before the ops are cleared. Or did I
> > overlook something perhaps?
> 
> The put/get is intended to allow a kapi user to hold a ref to tpm
> without it geting destroyed. It is not intended to be an exclusive lock.

These operations *are not* exposed to kapi. They are interal to the
driver. That's why it does not make sense speak about kapi user.

All the places where it's used it's always used in the way that you also
take the exclusive lock.

You are speaking how it could be used. I'm looking at how it's actually
used. Shouldn't implementation reflect that rather than future
prospects?

> > Trusted key unseal operation with TPM2 is broken into two operations:
> > 
> > 1. Load the given key blob.
> > 2. Unseal the data.
> > 
> > Without locking and unlocking mutex only once there is a race condition.
> 
> Well, the race condition is fundamentally because we don't have key
> virtualization in the kernel :|

I do agree and I'll share my thought about TPM2 context swapping in
LSS 2016.

> Those sorts of compound ops should hold the tpm_mutex manually, not
> through the get_ops scheme.

The next best option would be to have unlocked transmit_cmd function.

/Jarkko