linux-kernel - Re: [PATCH] tpm: Move dereference after NULL check in tpm_buf_check_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] [thread-next>] [day] [month] [year] [list]

Message-Id: <D2QVOMDX6BJW.3GCX7CFZL4KSF@iki.fi>
Date: Tue, 16 Jul 2024 13:06:49 +0300
From: "Jarkko Sakkinen" <jarkko.sakkinen@....fi>
To: "James Bottomley" <James.Bottomley@...senPartnership.com>, "Jarkko
 Sakkinen" <jarkko@...nel.org>, "Hao Ge" <hao.ge@...ux.dev>,
 <peterhuewe@....de>, <jgg@...pe.ca>
Cc: <linux-integrity@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Hao
 Ge" <gehao@...inos.cn>
Subject: Re: [PATCH] tpm: Move dereference after NULL check in
 tpm_buf_check_hmac_response

On Mon Jul 15, 2024 at 2:52 PM EEST, James Bottomley wrote:
> On Mon, 2024-07-15 at 14:25 +0300, Jarkko Sakkinen wrote:
> > On Tue Jul 9, 2024 at 5:33 AM EEST, Hao Ge wrote:
> > > From: Hao Ge <gehao@...inos.cn>
> > > 
> > > We shouldn't dereference "auth" until after we have checked that it
> > > is
> > > non-NULL.
> > > 
> > > Fixes: 7ca110f2679b ("tpm: Address !chip->auth in
> > > tpm_buf_append_hmac_session*()")
> > > Signed-off-by: Hao Ge <gehao@...inos.cn>
> > 
> > Also lacking:
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > Closes:
> > https://lore.kernel.org/linux-integrity/3b1755a9-b12f-42fc-b26d-de2fe4e13ec2@stanley.mountain/T/#u
> > 
> > What is happening here is that my commit exposed pre-existing bug to
> > static analysis but it did not introduce a new regression.
>
> Actually, it didn't.  The previous design was sessions were config
> determined and either auth would be non-NULL or attach would fail.  You
> chose with this series to make auth the indicator of whether sessions
> should be used, and before this auth could not be NULL so no bug
> existed.

Not on at least one driver, which does not call tpm2_sessions_init().

What do you exactly mean by design? It is first time I hear anyone to
claim that validating pointer is an alternative design.

Before my fixes:

int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
				int rc)
{
	struct tpm_header *head = (struct tpm_header *)buf->data;
	struct tpm2_auth *auth = chip->auth;
I.e.

Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")

Even in the current master there is still inline function that when HMAC
is disable:

static inline int tpm_buf_check_hmac_response(struct tpm_chip *chip,
					      struct tpm_buf *buf,
					      int rc)
{
	return rc;
}

>
> Consider also the fidelity of the Fixes tag for stable: this commit
> needs backporting with 7ca110f2679b and Fixes should identify that
>
> James

I'd suggest for you to focus fixing issue and not complaining about
irrelevant stuff.

And I'd suggest IBM to do better job next time as a company, and test
at least with your own hardware before sending anything.

BR, Jarkko