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: <b69b55e8-eba8-3f3f-e65f-cc5e6cf3eb84@linux.dev>
Date: Mon, 15 Jul 2024 16:29:42 +0800
From: Hao Ge <hao.ge@...ux.dev>
To: Jarkko Sakkinen <jarkko@...nel.org>, 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

Hi Jarkko

On 7/14/24 23:43, 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.
> Sorry for some latency in responses. I'm on holiday up until week 31 and
> only look at emerging issues but not every day.
Understand,I hope you enjoy your holiday.
> I do agree with the code change but the description contains no information
> of the bug and how the fix resolves it. Could you please rewrite the
> description?
>
> I can only think this realizing with tpm_ibmvtpm and TCG_TPM2_HMAC
> enabled because it was according to recent learnings a platform which
> does not end up calling tpm2_sessions_init().
>
> Since you bug report contains no bug report, I need to ask that did you
> realize a regression in some platform? Fix will get eventually accepted
> even if the bug was found by "looking at code" but the gist here is that
> your bug description contains no description how you found the bug,
> which it should.
Actually It's reported by smatch and Dan Carpenter also noticed this 
warning.

This is the email he sent

https://lore.kernel.org/all/3b1755a9-b12f-42fc-b26d-de2fe4e13ec2@stanley.mountain/ 

> When TCG_TPM2_HMAC is disable it should be safe because we have:
>
> static inline int tpm_buf_check_hmac_response(struct tpm_chip *chip,
> 					      struct tpm_buf *buf,
> 					      int rc)
> {
> 	return rc;
> }
>
> I also noticed that your fixes tag is incorrect as that null dereference
> pre-existed on tpm_ibmvtpm before my fixes. It is not a new regression
> introduced by my patches. So use git blame and check which commit
> introduced that one.
I'm a bit confused about fixes tag. Sometimes tool often fail to truly 
understand the context.

Form the logical perspective of the code,null deference indeed existed 
before your fixes.

But for smatch, It simply conducted a static code analysis and flagged a 
line with a warnning.

Its warning seems to be related to Commit 7ca110f2679b("tpm: Address 
!chip->auth in tpm_buf_append_hmac_session*()")

That's why I used the 'fixes' tag for Commit 7ca110f2679b.(Please 
forgive me for not being clear earlier. I discovered it through smatch.)

So, should I use the fix tag for Commit 7ca110f2679b ('tpm: Address 
!chip->auth in tpm_buf_append_hmac_session*()') or for Commit 
1085b8276bb4 ('tpm: Add the rest of the session HMAC API')?

Even though I have already send V2 which fixes tag using Commit 
1085b8276bb4 ('tpm: Add the rest of the session HMAC API'),

I still want to gain knowledge in this area so that I can more 
accurately use fxies tag and send patches in the future.
>
> Address these issues and send v2. Thank you!
>
> BR, Jarkko

Thanks

BR

Hao