Re: [PATCH 4/4] tpm: introduce tpm_extend_pcr_digests()

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Fri, Apr 07, 2017 at 11:10:37PM +0300, Jarkko Sakkinen wrote:
> On Fri, Apr 07, 2017 at 10:31:56PM +0300, Jarkko Sakkinen wrote:
> > On Fri, Apr 07, 2017 at 11:50:49AM +0200, Roberto Sassu wrote:
> > > On 4/5/2017 4:36 PM, Roberto Sassu wrote:
> > > > I have a question. As you can see in the IMA patch, I'm calling
> > > > tpm_is_tpm2() to determine if I should invoke tpm_pcr_extend(),
> > > > for TPM 1.2, or tpm_pcr_extend_digests(), for TPM 2.0.
> > > > 
> > > > Should the new function work with TPM 1.2? If a tpm2_digest
> > > > structure with a SHA1 digest is provided, I could call
> > > > tpm_pcr_extend() instead of returning an error.
> > > 
> > > Hi Jarkko
> > > 
> > > would you have any objection if the new functions work
> > > regardless of the TPM version?
> > > 
> > > Thanks
> > > 
> > > Roberto
> > 
> > Yes, you should not add multiple functions that do the same thing
> > essentially. Please rework tpm_pcr_extend instead.
> > 
> > And while you are doing it, please also rework it to use tpm_buf
> > for everything.
> > 
> > /Jarkko
> 
> Some prework is required before you implement your new things.
> 
> 1. tpm1_pcr_extend() to tpm-interface.c that is called by
> tpm_pcr_extend() and make it use tpm_buf. (1 commit)
> 
> 2. There's a race condition bug in the way Nayna has implemented the
> digest list extension. It takes and releases tpm_mutex multiple times.
> This bug needs to be fixed before any other changes are justified
> (1 commit). Please add the Fixes line to the commit message.
> 
> For (2) you should probably rename the existing tpm2_pcr_extend() as
> tpm2_pcr_extend_bank() and change it as a static function. That
> functio should take tpm_transmit flags as the last parameter. Then
> implement tpm2_pcr_extend() that does the same thing as is done now
> inside tpm_pcr_extend(). Call tpm2_pcr_extend_bank() inside that
> function with TPM_TRANSMIT_UNLOCKED.
> 
> You should make this as its own patch set without any of the new
> additions. Only after these fixes are landed I'm ready to review
> any new extensions to tpm_pcr_extend().
> 
> PS I *purposely* have not read any of the IMA links that you have sent
> to me. You should be able to explain these changes without requiring
> such action.
> 
> /Jarkko

And there was one big problem in your first patches you did not have the
RFC tag and still you did not include the kernel mailing list. I won't
apply or give reviewed-by to any patches that do not linux-kernel and
for non-trivial changes by defacto also include linux-security-module.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot