Multiple Coverity fixes for CAAM Driver, Crypto API and Core #6219

clementfaure · 2023-08-03T13:11:38Z

Hello,
Our CI reported multiple Coverity issues in optee-os. The issues fixed in this pull-request are rated High and Medium impact. As a maintainer of the crypto driver API and the CAAM, I want to push 2878ec6 b3b0567 1ba0f6d d34d5c5.

At the same time, I also fixed some issues in libutee, PTA attestation and the core. Feel free to comment regarding commits d68119a dae33b6 a6c6ab0 726f19e b98fa53

Thanks!

jforissier

For "drivers: caam: free resource upon dmaobj initialization failure":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier

For "drivers: caam: mp: fix memory on CAAM descriptor allocation failure":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier

For "crypto: rsamgf: initialize allocated buffer":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier

For "drivers: caam: remove dead code":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Use array index syntax instead of pointer arithmetic for better
readability.

This is a matter of preference, I believe @jenswi-linaro prefers the latter 😉

jforissier

For "lib: libutee: fix use after free":

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier

Comments on "lib: libutee: initialize TA property type".

Please use prefix: "lib: libutee: TEE_GetPropertyAsString():"

lib/libutee/tee_api_property.c

jforissier

For "core: tee: initialize tee_ta_param object":

Please use prefix: "core: tee: entry_open_session():".
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier

For "core: tee: initialize dirfile_entry objects":

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier

For "core: pta: attestation: check return value of crypto_bignum_bin2bn()":

One comment below, with that:
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

core/pta/attestation.c

jforissier · 2023-08-21T13:42:31Z

For "lib: libutee: initialize TA property type" + fixup:

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

clementfaure · 2023-08-22T07:51:54Z

Force push: squash + rebase

jenswi-linaro

For "lib: libutee: fix use after free":
You're fixing a false positive, I think that should be mentioned in the commit message.

lib/libutee/tee_api.c

jenswi-linaro

For "lib: libutee: initialize TA property type"

I believe this is a false positive, but it's a good idea to plug it.
Especially since our coding guidelines require local variables to be initialized.

If you agree that it's a false positive please mention that in the commit message, or point out where the error is so I can double check.

lib/libutee/tee_api_property.c

jenswi-linaro

For "core: tee: entry_open_session(): initialize tee_ta_param object"

This seems to be a false positive too, please mention that in the commit message or point out where the error is.

core/tee/entry_std.c

jenswi-linaro

For "core: tee: initialize dirfile_entry objects"

All these looks like false positives, please update the commit message accordingly or point out where the error are.

clementfaure · 2023-08-22T13:25:32Z

For "core: tee: initialize dirfile_entry objects"

All these looks like false positives, please update the commit message accordingly or point out where the error are.

Coverity does not follow variable references given as an argument to a function parameter. A function could potentially initialize a variable through its pointer, Coverity won't take that into account.

Edit: I did some test, and it looks like Coverity can keep track of variable value even through its reference, the web interface is just confusing. So, regarding Uninitialized scalar variable issues, theses are not false positive.

jenswi-linaro · 2023-08-23T08:12:41Z

For the commits:

"lib: libutee: fix use after free "
"lib: libutee: initialize TA property type"
"core: tee: entry_open_session(): initialize tee_ta_param object "
"core: tee: initialize dirfile_entry objects "
"lib: libutee: initialize variables in TEE_GetPropertyAsString()"
"core: tee: initialize variables in entry_open_session()"

please apply:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Call caam_dmaobj_free() upon caam_dmaobj_init_[input|output}() failure to free buffer allocated by allocate_private(). Signed-off-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Free the output DMA object upon CAAM descriptor allocation failure. Signed-off-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

In drvcrypt_rsa_mgf1() function, the memcpy() could potentially copy an uninitialized buffer. Allocate and initialize tmpdigest buffer with calloc() instead of malloc(). Signed-off-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Remove value check as it cannot be true and appears to be dead code. Use array index syntax instead of pointer arithmetic for better readability. Signed-off-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Make sure to call addr_is_in_no_share_heap() before the freeing buffer. This is a false positive as only the pointer value is used and not the memory freed. Signed-off-by: Clement Faure <clement.faure@nxp.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

TA property type `type` is declared without being initialized and might be used in the if statement uninitialized. Signed-off-by: Clement Faure <clement.faure@nxp.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Initialize tee_ta_param{} to zero in entry_open_session() so it can be used initialized in cleanup_shm_refs() without Coverity error. Signed-off-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Coverity reports many errors where dirfile_entry{} is used un-initialized. Resolve these errors by setting these objects to zero on declaration. Signed-off-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Check the return value of crypto_bignum_bin2bn(). Signed-off-by: Clement Faure <clement.faure@nxp.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Initialize local variables at declaration as specified by the coding guidelines. Signed-off-by: Clement Faure <clement.faure@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

clementfaure · 2023-08-23T09:52:21Z

Force-push : tag applied + re-word commits since uninitialized scalar value issues are not false positive

jforissier reviewed Aug 4, 2023

View reviewed changes

lib/libutee/tee_api_property.c Outdated Show resolved Hide resolved

jforissier reviewed Aug 4, 2023

View reviewed changes

core/pta/attestation.c Show resolved Hide resolved

clementfaure force-pushed the upstream/coverity_2 branch 2 times, most recently from dea6549 to f81cf08 Compare August 18, 2023 12:10

clementfaure force-pushed the upstream/coverity_2 branch from f81cf08 to 37246d9 Compare August 22, 2023 07:51

jenswi-linaro reviewed Aug 22, 2023

View reviewed changes

lib/libutee/tee_api.c Show resolved Hide resolved

jenswi-linaro reviewed Aug 22, 2023

View reviewed changes

lib/libutee/tee_api_property.c Outdated Show resolved Hide resolved

jenswi-linaro reviewed Aug 22, 2023

View reviewed changes

core/tee/entry_std.c Outdated Show resolved Hide resolved

jenswi-linaro reviewed Aug 22, 2023

View reviewed changes

clementfaure force-pushed the upstream/coverity_2 branch from 37246d9 to 667592d Compare August 22, 2023 13:51

clementfaure added 9 commits August 23, 2023 10:30

drivers: caam: mp: fix memory on CAAM descriptor allocation failure

71b6526

Free the output DMA object upon CAAM descriptor allocation failure. Signed-off-by: Clement Faure <clement.faure@nxp.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

core: pta: attestation: check return value of crypto_bignum_bin2bn()

60d63fe

Check the return value of crypto_bignum_bin2bn(). Signed-off-by: Clement Faure <clement.faure@nxp.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

clementfaure added 2 commits August 23, 2023 10:32

lib: libutee: initialize variables in TEE_GetPropertyAsString()

578d609

Initialize local variables at declaration as specified by the coding guidelines. Signed-off-by: Clement Faure <clement.faure@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

core: tee: initialize variables in entry_open_session()

e84bffc

Initialize local variables at declaration as specified by the coding guidelines. Signed-off-by: Clement Faure <clement.faure@nxp.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

clementfaure force-pushed the upstream/coverity_2 branch from 667592d to e84bffc Compare August 23, 2023 08:33

jforissier merged commit d24c41e into OP-TEE:master Aug 24, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple Coverity fixes for CAAM Driver, Crypto API and Core #6219

Multiple Coverity fixes for CAAM Driver, Crypto API and Core #6219

clementfaure commented Aug 3, 2023 •

edited

Loading

jforissier left a comment

jforissier left a comment

jforissier left a comment

jforissier left a comment

jforissier left a comment

jforissier left a comment

jforissier left a comment

jforissier left a comment

jforissier left a comment

jforissier commented Aug 21, 2023

clementfaure commented Aug 22, 2023

jenswi-linaro left a comment

jenswi-linaro left a comment

jenswi-linaro left a comment

jenswi-linaro left a comment

clementfaure commented Aug 22, 2023 •

edited

Loading

jenswi-linaro commented Aug 23, 2023

clementfaure commented Aug 23, 2023

Multiple Coverity fixes for CAAM Driver, Crypto API and Core #6219

Multiple Coverity fixes for CAAM Driver, Crypto API and Core #6219

Conversation

clementfaure commented Aug 3, 2023 • edited Loading

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier left a comment

Choose a reason for hiding this comment

jforissier commented Aug 21, 2023

clementfaure commented Aug 22, 2023

jenswi-linaro left a comment

Choose a reason for hiding this comment

jenswi-linaro left a comment

Choose a reason for hiding this comment

jenswi-linaro left a comment

Choose a reason for hiding this comment

jenswi-linaro left a comment

Choose a reason for hiding this comment

clementfaure commented Aug 22, 2023 • edited Loading

jenswi-linaro commented Aug 23, 2023

clementfaure commented Aug 23, 2023

clementfaure commented Aug 3, 2023 •

edited

Loading

clementfaure commented Aug 22, 2023 •

edited

Loading