Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General lack of .config control exposed by fuzzer .config divergence #9386

Open
marc-hb opened this issue Aug 22, 2024 · 6 comments
Open

General lack of .config control exposed by fuzzer .config divergence #9386

marc-hb opened this issue Aug 22, 2024 · 6 comments
Labels
bug Something isn't working as expected

Comments

@marc-hb
Copy link
Collaborator

marc-hb commented Aug 22, 2024

Originally posted by @marc-hb in #9343 (comment)

Even if #9356 is perfect, it does not change anything to the apparently messy Kconfig situation... @andyross could you help there? Should CONFIG_INCOHERENT really be gated by CONFIG_XTENSA? Can we align build-fuzz/.config with build/.config more? Some differences are unavoidable but they should be as small as possible: fuzzing something different from the product is both dangerous and a bit of a waste.


Let's compare the default build-mtl/zephyr/.config with build-fuzz/zephyr/.config as of today's SOF commit 1009ba7

./scripts/xtensa-build-zephyr.py -p mtl
./scripts/fuzz.sh -p -b -- -DCONFIG_IPC_MAJOR_4=y
diff  ../build-mtl/zephyr/.config build-fuzz/zephyr/.config | diffstat
 unknown |  617 ++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------------
 1 file changed, 186 insertions(+), 431 deletions(-)

That's a lot. Some of these differences are unavoidable because they are "hardware"-dependent, for instance: CONFIG_DT_HAS_CDNS_TENSILICA_XTENSA_LX7_ENABLED or CONFIG_ZEPHYR_POSIX_FUZZ_IRQ. Other diffferences are wrong because they are "pure software" like CONFIG_AMS or CONFIG_SOF_TELEMETRY. Can these last two software features affect fuzzing? Maybe, maybe not: we don't want to know. Pure software differences should be as small as possible, period. That is: if we want to fuzz and find security issues in code used in production.

So where do CONFIG_AMS and CONFIG_SOF_TELEMETRY come from?

sof]$ git grep CONFIG_AMS
app/boards/intel_adsp_ace15_mtpm.conf:CONFIG_AMS=y
app/boards/intel_adsp_ace20_lnl.conf:CONFIG_AMS=y
app/boards/intel_adsp_cavs25.conf:CONFIG_AMS=y

Enjoy the typical duplication.

As a quick fix, maybe we can bring the fuzzed .config closer to the real thing with this simple addition?

./scripts/fuzz.sh -p -b -- -DCONFIG_IPC_MAJOR_4=y -DEXTRA_CONF_FILE=boards/intel_adsp_ace15_mtpm.conf

Unfortunately not. First we get a gazillion of "hardware" warnings:

warning: POWER_DOMAIN_INTEL_ADSP (defined at drivers/power_domain/Kconfig:40) was assigned the value
'y' but got the value 'n'. Check these unsatisfied dependencies:
DT_HAS_INTEL_ADSP_POWER_DOMAIN_ENABLED (=n). 

warning: DEBUG_COREDUMP (defined at subsys/debug/coredump/Kconfig:4) was assigned the value 'y' but
got the value 'n'. Check these unsatisfied dependencies: ...

Then:

CMake Error at /var/home/mherber2/SOF/zephyr/cmake/modules/extensions.cmake:5340 (message):
  add_llext_target: only one source file is supported for ELF object file

cc:

@marc-hb
Copy link
Collaborator Author

marc-hb commented Aug 22, 2024

maybe we can bring the fuzzed .config closer to the real thing with this simple boards/intel_adsp_ace15_mtpm.conf addition? Unfortunately not.

That's because boards/intel_adsp_ace15_mtpm.conf is a "grab bag" of items totally unrelated to each other, including some actually ace15-specific stuff mixed up with pure software configuration like CONFIG_AMS. The latter being naturally duplicated all over the place.

The fix is to study https://docs.zephyrproject.org/latest/build/kconfig/setting.html#initial-conf and implement some proper "overlays" where (at least) hardware and software are separated.

Note how this comment is totally unrelated to fuzzing; fuzzing is just the messenger here.

@marc-hb marc-hb added the bug Something isn't working as expected label Aug 22, 2024
marc-hb added a commit to marc-hb/sof that referenced this issue Aug 22, 2024
Extract -DCONFIG_* definitions hardcoded inside the script and move them
to a new .conf file where they belong.

This is a first, baby-step towards addressing the more general lack of
.config control described in thesofproject#9386

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
lgirdwood pushed a commit that referenced this issue Aug 23, 2024
Extract -DCONFIG_* definitions hardcoded inside the script and move them
to a new .conf file where they belong.

This is a first, baby-step towards addressing the more general lack of
.config control described in #9386

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@lgirdwood
Copy link
Member

@marc-hb can this be closed now with #9389 merged ?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Aug 24, 2024

No, there are .config issues pretty much everywhere we look. Examples:

I will also submit some changes to reduce a bit the gap between build-fuzz/zephyr/.config and products.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Aug 27, 2024

I will also submit some changes to reduce a bit the gap between build-fuzz/zephyr/.config and products.

Done:

There are probably more CONFIG_ that should be added (and cleaned up). #9409 sets the stage/adds the "framework" showing how to do that.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 4, 2024

The CONFIG_ gap between fuzzing and production is still too big and there are still weird things in the production config (like b2f79a0) but I don't have any time left for this.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 18, 2024

Another problem noticed: commit 87e973d "downgrades" m to n instead of y. Maybe CONFIG_LLEXT=n would be better instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
None yet
Development

No branches or pull requests

2 participants