Warnings when building release tag version 1.2

[adeaarm]

Hello,

while using arm-none-eabi-gcc (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release) and building with options -Wmaybe-uninitialized these warnings appear:

Building C object tf-m-tests/test/secure_fw/suites/qcbor/CMakeFiles/tfm_qcbor_ns.dir/__/__/__/__/__/lib/ext/qcbor-src/src/qcbor_decode.o
lib/ext/qcbor-src/src/qcbor_decode.c: In function 'QCBORDecode_EnterBstrWrappedFromMapN':
lib/ext/qcbor-src/src/qcbor_decode.c:3347:14: warning: 'Item.uDataType' may be used uninitialized in this function [-Wmaybe-uninitialized]
 3347 |    const int nItemType = pItem->uDataType;
      |              ^~~~~~~~~
lib/ext/qcbor-src/src/qcbor_decode.c:3853:14: note: 'Item.uDataType' was declared here
 3853 |    QCBORItem Item;
      |              ^~~~
lib/ext/qcbor-src/src/qcbor_decode.c:3351:80: warning: 'Item.uTags[0]' may be used uninitialized in this function [-Wmaybe-uninitialized]
 3351 |    if(!(TagSpec.uTagRequirement & QCBOR_TAG_REQUIREMENT_ALLOW_ADDITIONAL_TAGS) &&
lib/ext/qcbor-src/src/qcbor_decode.c:3853:14: note: 'Item.uTags[0]' was declared here
 3853 |    QCBORItem Item;
      |              ^~~~
lib/ext/qcbor-src/src/qcbor_decode.c: In function 'QCBORDecode_EnterBstrWrappedFromMapSZ':
lib/ext/qcbor-src/src/qcbor_decode.c:3311:9: warning: 'Item.uDataType' may be used uninitialized in this function [-Wmaybe-uninitialized]
 3311 |       if(uDataType == puTypeList[i]) {
      |         ^
lib/ext/qcbor-src/src/qcbor_decode.c:3871:14: note: 'Item.uDataType' was declared here
 3871 |    QCBORItem Item;
      |              ^~~~
lib/ext/qcbor-src/src/qcbor_decode.c:3351:80: warning: 'Item.uTags[0]' may be used uninitialized in this function [-Wmaybe-uninitialized]
 3351 |    if(!(TagSpec.uTagRequirement & QCBOR_TAG_REQUIREMENT_ALLOW_ADDITIONAL_TAGS) &&
lib/ext/qcbor-src/src/qcbor_decode.c:3871:14: note: 'Item.uTags[0]' was declared here
 3871 |    QCBORItem Item;
      |              ^~~~
lib/ext/qcbor-src/src/qcbor_decode.c: In function 'QCBORDecode_GetUInt64ConvertAll':
lib/ext/qcbor-src/src/qcbor_decode.c:5192:4: warning: 'Item.uDataType' may be used uninitialized in this function [-Wmaybe-uninitialized]
 5192 |    switch(pItem->uDataType) {
      |    ^~~~~~
lib/ext/qcbor-src/src/qcbor_decode.c:5295:14: note: 'Item.uDataType' was declared here
 5295 |    QCBORItem Item;

One way of fixing them would be by always initialising QCBORItem Item = {0}; on those declarations.

[laurencelundblade]

I'm 99% sure there is no real problem with the code. QCBOR has been analyzed be real thorough static analyzers many times and they don't flag this error. I also looked briefly at the code and don't see a problem.

So the issue is how to stop the false warnings for not-very-good static analysis like that provided by -Wmaybe-uninitialized. I do want QCBOR to compile well in all environments.

I'm hesitant about adding an unnecessary initialization because it will probably increase code size somewhere. I want to see if there's another way to solve this or see that the code size increase is very small.

Will take me a little while to figure this out, but wanted to provide a response right away. Thanks for reporting.

[laurencelundblade]

I plan on finding a fix here, but any comment on why you are using -Wmaybe-uninitialized (rather than a better static analyzer)?

[adeaarm]

Hi @laurencelundblade , thanks for the feedback. We simply build TF-M using -Wall by default with GCC, and I believe that -Wmaybe-uninitialized is enabled by default in that case (at least starting from gcc 10). Note that this affects only optimized builds. Regarding the specifics of this warning, we did not re-run any other static analyzer so I can't really comment on how genuine this issue is. It looks connected to the early return available in the function that populates the Item object. Might get back to this at some point and update this thread with additional findings, in the meantime you have your evaluation from your side as well.

[laurencelundblade]

See #179

[adeaarm]

While I understand the point about the performance hit by initialising, I don't see why that would be a problem regarding security, especially compared to the current strategy which is to leave them uninitialised, i.e. initialise with casual data found on the stack. In other words, if the security of the QCBORItem might be adversely affected by its initialisation values, then this would be even a stronger point to provide initialization functions for those items.

[laurencelundblade]

Hi,

Did you read the whole comment in the source file?

• -Wmaybe-uninitialized has "maybe" in its name because it is inconclusive and produces false positives. (and -Wall turns it on for some versions of gcc and not for others)
• It's true that using a value that is left as random stack data is very bad for security, but that's not happening here.
• There are no cases where a variable is uninitialized (and contains random stack data) before it is used (-Wmaybe-uninitialized's complaint is a false positive).
• 0 is not necessarily the right value if they were uninitialized (which they are not). The code that is there sets them to the right value before they are used (but -Wmaybe-uninitialized is too half-baked to know).

A number of big tech companies (US and Chinese) have run formal static analysis on this code that is much more thorough than -Wmaybe-uninitialized because they are using it for critical things.

[adeaarm]

I misinterpreted your comment about security. I agree with the points above.

[adeaarm]

Just to add a bit more of context: The fixed value init proposed above was meant just to silence the warning, not to fix an issue assuming that it was genuine.

[laurencelundblade]

Fixed by #179