security: Add compiler static analysis support #64595

ceolin · 2023-10-30T20:46:52Z

Add a build option to enable GCC builtin static analysis.

To enable it an application has to set
CONFIG_COMPILER_STATIC_ANALYSIS=y

When this option is enabled GCC performs a static analysis and can point problems like:

sample.c

+	int *j; +
+	if (j != NULL) {
+		printf("j != NULL\n");

output:

${ZEPHYR_BASE}/samples/userspace/hello_world_user/src/main.c:30:12: warning: use of uninitialized value 'j' [CWE-457]
[-Wanalyzer-use-of-uninitialized-value]
   30 |         if (j != NULL) {
      |            ^
  'main': events 1-2
    |
    |   25 |         int *j;
    |      |              ^
    |      |              |
    |      |              (1) region created on stack here
    |......
    |   30 |         if (j != NULL) {
    |      |            ~
    |      |            |
    |      |            (2) use of uninitialized value 'j' here

Kconfig.zephyr

stephanosio · 2023-10-31T06:05:01Z

Kconfig.zephyr

@@ -442,6 +442,14 @@ config COMPILER_COLOR_DIAGNOSTICS
 	help
 	  Compiler diagnostic messages are colorized.

+config COMPILER_STATIC_ANALYSIS
+	bool "Compiler static analysis"
+	depends on "${ZEPHYR_TOOLCHAIN_VARIANT}" = "zephyr"


Why does this depend on Zephyr SDK?

That is GCC specific, while the host toolchain probably will work, that was the closest I got to check that it is using GCC. Is there a better way ?

zephyr is not the only ZEPHYR_TOOLCHAIN_VARIANT that is GCC-based (i.e. there is also the generic cross-compile, and crosstool-ng-built xtools; though, the latter should/will be removed soon).

The most ideal solution would be to introduce something like TOOLCHAIN_SUPPORTS_COMPILER_STATIC_ANALYSIS set by each toolchain variant, and depend on it (similar to how TOOLCHAIN_SUPPORTS_THREAD_LOCAL_STORAGE works).

But, given that we still have not established a consistent interface for toolchain capability detection (@tejlmand please feel free to correct me if some progress has been made on this front while I was away), I would just leave this an open option with a description saying this option currently only works with GCC (and also potentially a build system warning when an unsupported toolchain is used with this option).

p.s. To elaborate, the rationale for leaving this an open option, as opposed to specifying the dependencies on all upstream GCC-based toolchain variants, is to not tie down the hands of the downstream out-of-tree toolchain users.

I agree.
We should not tie this to Zephyr or gcc toolchains.

Although GCC has a specific -fanalyze then other toolchains may supports some form of static analysis during compilation.

That's why we have the compiler properties, which can be left empty if the compiler doesn't have a dedicated flag.

Suggested change

depends on "${ZEPHYR_TOOLCHAIN_VARIANT}" = "zephyr"

Kconfig.zephyr

cmake/compiler/gcc/compiler_flags.cmake

Kconfig.zephyr

tejlmand

Thanks for proposing this.

It like to open a discussion if a Kconfig is the best approach in this case.

It's nice to use Kconfig when a flag can be used directly on compiler invocation, but exactly for the SCA case, then Zephyr also supports ZEPHYR_SCA_VARIANT.

This means user now have two places / ways where they can read about SCA and how to use such tools.

The alternative would be to simply create a ZEPHYR_SCA_VARIANT=gcc, where all it does is applying the -fanalyze flag to the compiler.

That would mean there is only one place where users should read about SCA tools.

I would like to hear other opinions on this matter.
Personally I see high value in only having handling of similar feature done in a common way, but in this specific case, Kconfig is also a sane choice.

If going for the Kconfig approach, then I have a couple of specific change requests.

tejlmand · 2023-11-24T09:10:22Z

Kconfig.zephyr

@@ -442,6 +442,14 @@ config COMPILER_COLOR_DIAGNOSTICS
 	help
 	  Compiler diagnostic messages are colorized.

+config COMPILER_STATIC_ANALYSIS
+	bool "Compiler static analysis"
+	depends on "${ZEPHYR_TOOLCHAIN_VARIANT}" = "zephyr"


I agree.
We should not tie this to Zephyr or gcc toolchains.

Although GCC has a specific -fanalyze then other toolchains may supports some form of static analysis during compilation.

That's why we have the compiler properties, which can be left empty if the compiler doesn't have a dedicated flag.

Suggested change

depends on "${ZEPHYR_TOOLCHAIN_VARIANT}" = "zephyr"

Kconfig.zephyr

tejlmand · 2023-12-13T15:30:13Z

@ceolin did you push the wrong commit ?

It's only doc changes now describing the process and no implementation anymore (of -fanalyze).

ceolin · 2023-12-13T16:17:54Z

@ceolin did you push the wrong commit ?

It's only doc changes now describing the process and no implementation anymore (of -fanalyze).

lol !!! my bad. I push a different commit indeed.

tejlmand · 2024-01-03T14:26:51Z

lol !!! my bad. I push a different commit indeed.

what is status on this PR.
Are you going to push another commit containing implementation ?

ceolin · 2024-01-15T06:50:22Z

@tejlmand @stephanosio I just changed it to use Zephyr's SCA infra. It got much simpler.

Please take a look again :)

doc/develop/sca/gcc.rst

Enable GCC builtin static analysis in Zephyr's static code analysis (SCA) infra. When this option is enabled GCC performs a static analysis and can point problems like: sample.c + int *j; + + if (j != NULL) { + printf("j != NULL\n"); output: ${ZEPHYR_BASE}/samples/userspace/hello_world_user/src/main.c:30:12: warning: use of uninitialized value 'j' [CWE-457] [-Wanalyzer-use-of-uninitialized-value] 30 | if (j != NULL) { | ^ 'main': events 1-2 | | 25 | int *j; | | ^ | | | | | (1) region created on stack here |...... | 30 | if (j != NULL) { | | ~ | | | | | (2) use of uninitialized value 'j' here Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>

ceolin · 2024-01-18T06:26:22Z

@tejlmand @stephanosio ping :)

tejlmand

Nice and clean ❤️

kartben · 2024-01-19T10:42:37Z

Nice indeed, thank you! Related naive question: would it make sense to provide several SCA variants to the same build command, or are the variants always going to be mutually exclusive and possibly messing up with one another?

jfischer-no

Thanks for that. Perhaps it would be useful to have another option to set the flag for Zephyr tree only, i.e. to exclude vendor HALs from the analysis.

tejlmand · 2024-01-25T10:49:39Z

Related naive question: would it make sense to provide several SCA variants to the same build command, or are the variants always going to be mutually exclusive and possibly messing up with one another?

in most cases I assume they will be mutual exclusive, but I guess there can be cases where you technically could combine, for example the gcc with sparse.
But good luck in trying to parse the output in such cases. I assume running both tools independently would make it easier to understand the error messages provided.

Stale, and there are no longer Kconfig changes in PR

nashif reviewed Oct 30, 2023

View reviewed changes

Kconfig.zephyr Outdated Show resolved Hide resolved

ceolin force-pushed the static_analysis branch from 14b3e89 to 0c14a95 Compare October 30, 2023 23:13

ceolin marked this pull request as ready for review October 30, 2023 23:13

ceolin requested a review from tejlmand as a code owner October 30, 2023 23:13

zephyrbot added area: Kconfig area: Toolchains Toolchains area: Build System labels Oct 30, 2023

zephyrbot requested review from 57300, jeremybettis, nordicjm and stephanosio October 30, 2023 23:14

zephyrbot assigned tejlmand Oct 30, 2023

ceolin force-pushed the static_analysis branch from 0c14a95 to bbb880d Compare October 31, 2023 04:07

stephanosio previously requested changes Oct 31, 2023

View reviewed changes

ceolin force-pushed the static_analysis branch from bbb880d to 77d5664 Compare October 31, 2023 21:45

tejlmand requested changes Nov 24, 2023

View reviewed changes

ceolin force-pushed the static_analysis branch from 77d5664 to 52b8de0 Compare December 3, 2023 23:29

zephyrbot added area: Coding Guidelines Coding guidelines and style area: Documentation labels Dec 3, 2023

zephyrbot requested review from carlescufi, jfischer-no, kartben and keith-zephyr December 3, 2023 23:30

ceolin force-pushed the static_analysis branch from 52b8de0 to 9fe8026 Compare December 3, 2023 23:41

ceolin force-pushed the static_analysis branch from 9fe8026 to c04896b Compare January 15, 2024 06:49

ceolin requested a review from tejlmand January 15, 2024 06:50

ceolin requested a review from stephanosio January 15, 2024 06:50

ceolin force-pushed the static_analysis branch from c04896b to 78e2ec4 Compare January 15, 2024 06:55

pdgendt reviewed Jan 15, 2024

View reviewed changes

doc/develop/sca/gcc.rst Outdated Show resolved Hide resolved

ceolin force-pushed the static_analysis branch from 78e2ec4 to 62d20f4 Compare January 17, 2024 18:28

pdgendt reviewed Jan 17, 2024

View reviewed changes

doc/develop/sca/gcc.rst Outdated Show resolved Hide resolved

ceolin force-pushed the static_analysis branch from 62d20f4 to a9c7507 Compare January 17, 2024 18:44

pdgendt approved these changes Jan 17, 2024

View reviewed changes

tejlmand approved these changes Jan 19, 2024

View reviewed changes

kartben approved these changes Jan 19, 2024

View reviewed changes

jfischer-no approved these changes Jan 19, 2024

View reviewed changes

carlescufi merged commit 3fc5d97 into zephyrproject-rtos:main Jan 25, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security: Add compiler static analysis support #64595

security: Add compiler static analysis support #64595

ceolin commented Oct 30, 2023 •

edited

stephanosio Oct 31, 2023

ceolin Oct 31, 2023

stephanosio Nov 1, 2023 •

edited

tejlmand Nov 24, 2023

tejlmand left a comment

tejlmand Nov 24, 2023

tejlmand commented Dec 13, 2023

ceolin commented Dec 13, 2023

tejlmand commented Jan 3, 2024

ceolin commented Jan 15, 2024

ceolin commented Jan 18, 2024

tejlmand left a comment

kartben commented Jan 19, 2024

jfischer-no left a comment

tejlmand commented Jan 25, 2024

security: Add compiler static analysis support #64595

security: Add compiler static analysis support #64595

Conversation

ceolin commented Oct 30, 2023 • edited

stephanosio Oct 31, 2023

Choose a reason for hiding this comment

ceolin Oct 31, 2023

Choose a reason for hiding this comment

stephanosio Nov 1, 2023 • edited

Choose a reason for hiding this comment

tejlmand Nov 24, 2023

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand Nov 24, 2023

Choose a reason for hiding this comment

tejlmand commented Dec 13, 2023

ceolin commented Dec 13, 2023

tejlmand commented Jan 3, 2024

ceolin commented Jan 15, 2024

ceolin commented Jan 18, 2024

tejlmand left a comment

Choose a reason for hiding this comment

kartben commented Jan 19, 2024

jfischer-no left a comment

Choose a reason for hiding this comment

tejlmand commented Jan 25, 2024

ceolin commented Oct 30, 2023 •

edited

stephanosio Nov 1, 2023 •

edited