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@xxxxxxxxxx/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
