Re: [PATCH URGENT FIX] security: keys: trusted: fix lost handle flush

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Mon, 2019-12-16 at 11:47 +0530, Sumit Garg wrote:
> On Fri, 13 Dec 2019 at 19:19, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > On Fri, 2019-12-13 at 07:35 -0500, James Bottomley wrote:
> > > On Fri, 2019-12-13 at 11:10 +0530, Sumit Garg wrote:
> > 
> > [...]
> > > >  Also, I think we need "#else" part for this API as well.
> > > 
> > > No, we shouldn't ... the #else part is only for functions which
> > > are called when the TPM isn't compiled in.  That should never
> > > happen with tpm2_flush_context, so if it ever does we want the
> > > compile to break.
> > 
> > Just on this bit, it looks like we've given insufficient thought to
> > what it means to move TPM internals using code outside of the tpm
> > directory.  I think we really need two include/linux files, one
> > tpm.h for external code that going to do stuff which it would do
> > anyway even if a TPM weren't compiled in, like the PCR read and
> > extend.  The other tpm-internal.h for code that wants access to TPM
> > internal functions like flushing and session handling and will take
> > care itself of the case where the TPM isn't compiled in, like the
> > trusted key code does through Kconfig dependencies.  The test
> > should be easy: if it was originally in drivers/char/tpm/tpm.h it
> > belongs in include/linux/tpm-internals.h
> > 
> 
> Your approach sounds good to me. But how about just moving the APIs
> that needs to be used independently of TPM compilation to
> include/linux/tpm-externals.h from drivers/char/tpm/tpm.h. As the
> initial thoughts while I was moving contents to
> drivers/char/tpm/tpm.h was to keep a consolidated view of TPM header
> for a particular user.

If we do that, we have to change every current user of tpm.h, because
that file was originally only for the external users (mostly PCR
extends).  I'd rather avoid the churn and keep them using tpm.h, hence
the tpm-internal.h proposal.

> > > > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
> 
> I agree with you that the above API should remain as it is and should
> be moved out of the following check:
> 
> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
> ...
> #else
> ...
> #endif

That merely changes where the compile breaks.  If it's within it breaks
on the actual offending file.  If it's without, you don't find out
until kernel link time.  I'm reasonably ambivalent about this but my HA
days have given me a preference for failing fast, so in the file
itself.

James