| Message ID | 20240709023337.102509-1-hao.ge@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: Move dereference after NULL check in tpm_buf_check_hmac_response | expand |
> We shouldn't dereference "auth" until after we have checked that it is > non-NULL. How do you think about to improve such a change description with imperative wordings? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n94 > Fixes: 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()") 1. Would the commit 1085b8276bb4239daa7008f0dcd5c973e4bd690f ("tpm: Add the rest of the session HMAC API") be more appropriate as a reference? 2. Would you like to add a “stable tag” accordingly? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.10-rc7#n34 3. How do you think about to append parentheses to a function name in the summary phrase? Regards, Markus
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. Sorry for some latency in responses. I'm on holiday up until week 31 and only look at emerging issues but not every day. 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. 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. Address these issues and send v2. Thank you! BR, Jarkko
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@kylinos.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
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. I missed from your patch how did you ended up to your conclusions. Please *do not* ignore the sources next time. Either explain how the bug was found or provide the reporting source. You are essentially taking credit and also blame from the work that you did not accomplish yourself, which is both wrong and dishonest. BR, Jarkko
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. Consider also the fidelity of the Fixes tag for stable: this commit needs backporting with 7ca110f2679b and Fixes should identify that James
Hi Jarkko Have a nice day. On 7/15/24 19:25, 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 Regarding this version, I don't think I should add these. I send this patch on July 9th, 2024. The following email was sent on July 13th, 2024. https://lore.kernel.org/linux-integrity/3b1755a9-b12f-42fc-b26d-de2fe4e13ec2@stanley.mountain/T/#u I think these should be included in the subsequent versions (if any). > > What is happening here is that my commit exposed pre-existing bug to > static analysis but it did not introduce a new regression. I missed > from your patch how did you ended up to your conclusions. > > Please *do not* ignore the sources next time. Either explain how the bug > was found or provide the reporting source. You are essentially taking > credit and also blame from the work that you did not accomplish > yourself, which is both wrong and dishonest. > > BR, Jarkko OK,got it,I'll pay more attention to such details in the future. I would like to clarify that I did not taking credit and dishonest. As stated earlier, the timeline indicates that my patch preceded his email. Before submitting my patch, I conducted a thorough search to ensure that there were no related submissions, in order to avoid any duplication of effort and wastage of everyone's time. I didn't expect to have wasted everyone's time because of commit message , and I sincerely apologize for that. Thanks BR Hao
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
On Tue Jul 16, 2024 at 4:04 AM EEST, Hao Ge wrote: > Hi Jarkko > > Have a nice day. > > On 7/15/24 19:25, 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 > > Regarding this version, I don't think I should add these. > > I send this patch on July 9th, 2024. > > The following email was sent on July 13th, 2024. > > https://lore.kernel.org/linux-integrity/3b1755a9-b12f-42fc-b26d-de2fe4e13ec2@stanley.mountain/T/#u > > I think these should be included in the subsequent versions (if any). OK sorry, then you are right. > > > > > What is happening here is that my commit exposed pre-existing bug to > > static analysis but it did not introduce a new regression. I missed > > from your patch how did you ended up to your conclusions. > > > > Please *do not* ignore the sources next time. Either explain how the bug > > was found or provide the reporting source. You are essentially taking > > credit and also blame from the work that you did not accomplish > > yourself, which is both wrong and dishonest. > > > > BR, Jarkko > > OK,got it,I'll pay more attention to such details in the future. > > I would like to clarify that I did not taking credit and dishonest. OK, cool, and I do agree, and I'm sorry what I said. Please just add the necessary details and send v2 then. BRR, Jarkko
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> > --- > drivers/char/tpm/tpm2-sessions.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c > index 2281d55df545..d3521aadd43e 100644 > --- a/drivers/char/tpm/tpm2-sessions.c > +++ b/drivers/char/tpm/tpm2-sessions.c > @@ -746,15 +746,16 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf, > struct tpm2_auth *auth = chip->auth; > off_t offset_s, offset_p; > u8 rphash[SHA256_DIGEST_SIZE]; > - u32 attrs; > + u32 attrs, cc; > struct sha256_state sctx; > u16 tag = be16_to_cpu(head->tag); > - u32 cc = be32_to_cpu(auth->ordinal); > int parm_len, len, i, handles; > > if (!auth) > return rc; > > + cc = be32_to_cpu(auth->ordinal); > + > if (auth->session >= TPM_HEADER_SIZE) { > WARN(1, "tpm session not filled correctly\n"); > goto out; Please check: https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=72d6e06ed101e31e943937e42053fc690dc75cfe It is exactly this except commit message is tuned. And please denote that I'm on holiday ;-) If that works for you, I can put it to my -rc PR. Thank you. BR, Jarkko
On Tue Jul 16, 2024 at 1:33 PM EEST, 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> > > --- > > drivers/char/tpm/tpm2-sessions.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c > > index 2281d55df545..d3521aadd43e 100644 > > --- a/drivers/char/tpm/tpm2-sessions.c > > +++ b/drivers/char/tpm/tpm2-sessions.c > > @@ -746,15 +746,16 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf, > > struct tpm2_auth *auth = chip->auth; > > off_t offset_s, offset_p; > > u8 rphash[SHA256_DIGEST_SIZE]; > > - u32 attrs; > > + u32 attrs, cc; > > struct sha256_state sctx; > > u16 tag = be16_to_cpu(head->tag); > > - u32 cc = be32_to_cpu(auth->ordinal); > > int parm_len, len, i, handles; > > > > if (!auth) > > return rc; > > > > + cc = be32_to_cpu(auth->ordinal); > > + > > if (auth->session >= TPM_HEADER_SIZE) { > > WARN(1, "tpm session not filled correctly\n"); > > goto out; > > Please check: > > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=72d6e06ed101e31e943937e42053fc690dc75cfe > > It is exactly this except commit message is tuned. And please denote > that I'm on holiday ;-) > > If that works for you, I can put it to my -rc PR. > > Thank you. Again because of holiday I failed to notice that my 6.11 PR's were accepted and since it is only Tue, I'm sure I squeeze one commit PR still -rc1, if a quick response. BR, Jarkko
On Tue Jul 16, 2024 at 1:33 PM EEST, 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> > > --- > > drivers/char/tpm/tpm2-sessions.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c > > index 2281d55df545..d3521aadd43e 100644 > > --- a/drivers/char/tpm/tpm2-sessions.c > > +++ b/drivers/char/tpm/tpm2-sessions.c > > @@ -746,15 +746,16 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf, > > struct tpm2_auth *auth = chip->auth; > > off_t offset_s, offset_p; > > u8 rphash[SHA256_DIGEST_SIZE]; > > - u32 attrs; > > + u32 attrs, cc; > > struct sha256_state sctx; > > u16 tag = be16_to_cpu(head->tag); > > - u32 cc = be32_to_cpu(auth->ordinal); > > int parm_len, len, i, handles; > > > > if (!auth) > > return rc; > > > > + cc = be32_to_cpu(auth->ordinal); > > + > > if (auth->session >= TPM_HEADER_SIZE) { > > WARN(1, "tpm session not filled correctly\n"); > > goto out; > > Please check: > > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=72d6e06ed101e31e943937e42053fc690dc75cfe Changed to: https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=7dc357d343f134bf59815ff6098b93503ec8a23b Just fixed some typos. BR, Jarkko
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c index 2281d55df545..d3521aadd43e 100644 --- a/drivers/char/tpm/tpm2-sessions.c +++ b/drivers/char/tpm/tpm2-sessions.c @@ -746,15 +746,16 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf, struct tpm2_auth *auth = chip->auth; off_t offset_s, offset_p; u8 rphash[SHA256_DIGEST_SIZE]; - u32 attrs; + u32 attrs, cc; struct sha256_state sctx; u16 tag = be16_to_cpu(head->tag); - u32 cc = be32_to_cpu(auth->ordinal); int parm_len, len, i, handles; if (!auth) return rc; + cc = be32_to_cpu(auth->ordinal); + if (auth->session >= TPM_HEADER_SIZE) { WARN(1, "tpm session not filled correctly\n"); goto out;