Re: [PATCH] tpm: Move dereference after NULL check in tpm_buf_check_hmac_response

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

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@kylinos.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@kylinos.cn>
> > 
> > Also lacking:
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.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