linux-kernel - Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac

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] [day] [month] [year] [list]

Message-Id: <D2HP4TX4S873.2OS2LXAWT58C4@iki.fi>
Date: Fri, 05 Jul 2024 18:04:52 +0300
From: "Jarkko Sakkinen" <jarkko.sakkinen@....fi>
To: "Jarkko Sakkinen" <jarkko@...nel.org>, "Stefan Berger"
 <stefanb@...ux.ibm.com>, <linux-integrity@...r.kernel.org>
Cc: "Thorsten Leemhuis" <regressions@...mhuis.info>, "Linus Torvalds"
 <torvalds@...ux-foundation.org>, <stable@...r.kernel.org>, "Peter Huewe"
 <peterhuewe@....de>, "Jason Gunthorpe" <jgg@...pe.ca>, "James Bottomley"
 <James.Bottomley@...senPartnership.com>, "Mimi Zohar"
 <zohar@...ux.ibm.com>, "David Howells" <dhowells@...hat.com>, "Paul Moore"
 <paul@...l-moore.com>, "James Morris" <jmorris@...ei.org>, "Serge E.
 Hallyn" <serge@...lyn.com>, "Ard Biesheuvel" <ardb@...nel.org>, "Mario
 Limonciello" <mario.limonciello@....com>, <linux-kernel@...r.kernel.org>,
 <keyrings@...r.kernel.org>, <linux-security-module@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] tpm: Address !chip->auth in
 tpm_buf_append_hmac_session*()

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