Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

["Jarkko Sakkinen" <jarkko.sakkinen@iki.fi>]

On Fri Jul 5, 2024 at 5:35 PM EEST, Jarkko Sakkinen wrote:
> On Fri Jul 5, 2024 at 5:05 PM EEST, Stefan Berger wrote:
> > The original thread here
> >
> > https://lore.kernel.org/linux-integrity/656b319fc58683e399323b880722434467cf20f2.camel@kernel.org/T/#t
> >
> > identified the fact that tpm2_session_init() was missing for the ibmvtpm 
> > driver. It is a non-zero problem for the respective platforms where this 
> > driver is being used. The patched fixed the reported issue.
>
> All bugs needs to be fixed always before features are added. You are
> free now to submit your change as a feature patch, which will be
> reviewed and applied later on.
>
> > Now that you fixed it in v4 are you going to accept my original patch 
> > with the Fixes tag since we will (likely) have an enabled feature in 
> > 6.10 that is not actually working when the ibmvtpm driver is being used?
>
> There's no bug in tpm_ibmvtpm driver as it functions as well as in 6.9.
>
> I can review it earliest in the week 31, as feature patch. This was my
> holiday week, and I came back only to fix the bug in the authentication
> session patch set.
>
> > I do no think that this is true and its only tpm_ibmvtpm.c that need the 
> > call to tpm2_session_init. All drivers that use TPM_OPS_AUTO_STARTUP 
> > will run tpm_chip_register -> tpm_chip_bootstrap -> tpm_auto_startup -> 
> > tpm2_auto_startup -> tpm2_sessions_init
>
> Right my bad. I overlooked the call sites and you're correct in that
> for anything with that flag on, it will be called.
>
> It still changes nothing, as the commit you were pointing out in the
> fixes tag does not implement initialization code, and we would not have
> that flag in the first place, if it was mandatory [1].
>
> [1] It could be that it is mandatory perhaps, but that is a different
> story. Then we would render the whole flag out. I think this was anyway
> good insight, even if by unintentionally, and we can reconsider removing
> it some day.

I should have rejected the patch set based on not validating chip->auth
in opaque API that tpm2-sessions is, and it should not fail caller like
that no matter how world outside of it is structured. It's a time-bomb
like it is in the mainline because of this.  I missed that detail
and your transcript exposed the bug.

Working around an *identified* bug in the caller *is not* a bug fix.

BR, Jarkko