IPC4 fuzzing support #7531

andyross · 2023-04-27T18:35:15Z

Support for IPC4 protocol fuzzing with the existing rig. Just select CONFIG_IPC_MAJOR_4=y and it will adjust as needed. It seems to have pretty decent coverage as far as I've been able to check manually, has run on my system for about two hours now (running 64 instances in parallel), and has uncovered one bug (in #7528 -- that will obviously want to merge before this does).

@marc-hb I didn't adjust the existing fuzz.sh script in this PR, it's still doing IPC3 by default. There are lots of ways to handle this: you could run the fuzzer twice in CI for each protocol, we could add new "posix" platform variants for each, etc... Let me know what you'd prefer.

cujomalainey

mostly pretty good, just mostly patch cleanup

src/audio/base_fw.c

src/ipc/ipc4/helper.c

cujomalainey · 2023-04-27T20:05:11Z

src/platform/posix/ipc.c

this work should be consolidated with the commit that added it

cujomalainey · 2023-04-27T20:07:58Z

Something must be weird with the state, you pushed the PR 1h ago, the commits 2 days ago, and my review was 1min ago but yet is marked as "outdated"....

marc-hb · 2023-04-27T20:29:50Z

it's still doing IPC3 by default. There are lots of ways to handle this: you could run the fuzzer twice in CI for each protocol, we could add new "posix" platform variants for each, etc... Let me know what you'd prefer.

Can you please try something like this? (UNTESTED)
yamllint is your friend! Cause force-pushing every small indentation fix is NOT fun :-)

I didn't expect any shell script change, pretty sure I anticipated this PR. Is there any?

--- a/.github/workflows/ipc_fuzzer.yml
+++ b/.github/workflows/ipc_fuzzer.yml
@@ -42,15 +42,14 @@ jobs:
           fuzz-seconds: 5
 
 
-  # TODO, to add IPC4 support fix compilation of:
-  #
-  #    ./scripts/fuzz.sh -t 1 -- -DCONFIG_IPC_MAJOR_4=y
-  #
-  # ... or of some other _IPC4_ -DOVERLAY_CONFIG=
-  #
-  # Then use a simple IPC3/IPC4 matrix like the one in zephyr.yml
   simple-IPC3-fuzz_sh:
     runs-on: ubuntu-22.04
+    strategy:
+      fail-fast: false
+      matrix:
+        # Keep these names short due to questionable Github UI choices
+        IPC: [ IPC3, IPC4 ]
+
     steps:
       - name: add i386 arch
         run: |
@@ -78,4 +77,8 @@ jobs:
           cd workspace
           clang --verbose
           set -x
-          sof/scripts/fuzz.sh -o _.log -t 300
+          case '${{ matrix.IPC }}' in
+            IPC3) cmake_arg='-DCONFIG_IPC_MAJOR_3=y'
+            IPC4) cmake_arg='-DCONFIG_IPC_MAJOR_4=y'
+          esac
+          sof/scripts/fuzz.sh -o _.log -t 300 -- "$cmake_arg"

__packed and __aligned are defined in compiler_attributes.h, which wasn't included, and when building for Zephyr that needs to include the zephyr/toolchain.h header which is the original source of those macros. Transitive headers were saving this, but building for posix/fuzzing works differently. Signed-off-by: Andy Ross <andyross@google.com>

CLK_MAX_CPU_HZ is defined in platform/lib/clk.h, unmasked by posix work that lacks the same transitive headers. Signed-off-by: Andy Ross <andyross@google.com>

This calls mailbox APIs when building for IPC4, but was missing the header Signed-off-by: Andy Ross <andyross@google.com>

andyross · 2023-05-02T19:56:22Z

Update, merge two patches which were vestigial versions of the same fix (this sat around in my tree for a while). Clean up the checkpatch warnings. Also filed bug #7549 to track the platform-dependent code discussion.

I'm too timid to muck with CI files in the same PR, so no change to the workflow yaml here. But I did validate that fuzz.sh works in place if you just pass the kconfig (sorry, didn't realize you'd left the "--" syntax trick in the command line there)

andyross · 2023-05-02T19:58:16Z

Remaining checkpatch issue is a false positive. It's not possible to non-trivially use memcpy_s in that circumstance. The comp_data destination buffer is just a void* on the main IPC object, it doesn't have a build-time (or runtime) checkable size, we just assume it can hold one message.

cujomalainey

just my request about the TODO, rest looks good

src/audio/base_fw.c

This was trying to read hardware registers, which obviously doesn't work on other platforms. Stub with zeros for now. Longer term: this whole message ("basefw_mem_state_info") might want some rework. In addition to being heavily hardware-specific, it seems like a comparatively large block without self-describing data (e.g. id/val pairs), nor with a struct definition to specify it, nor with any documentation that I can find. Signed-off-by: Andy Ross <andyross@google.com>

marc-hb · 2023-05-02T22:10:45Z

I'm too timid to muck with CI files in the same PR, so no change to the workflow yaml here. But I did validate that fuzz.sh works in place if you just pass the kconfig

Thanks for manually testing this, good enough for now.

src/audio/base_fw.c

src/ipc/ipc4/helper.c

lyakh · 2023-05-03T07:44:28Z

src/ipc/ipc4/helper.c

superfluous initialisation

No, it's to handle the case below where it may or may not be initialized to IMR depending on RIMAGE_MANIFEST.

Yes, if RIMAGE_MANIFEST is defined, then desc is assigned immediately and there's no need for initialisation. If it isn't defined, then the function returns immediately and desc is never used, so there's again no need to initialise. BTW, if it isn't defined and there's a return statement right at the beginning, then compilers / analysers might complain about a whole bunch of unused variables and unreachable code.

src/platform/posix/ipc.c

The component model for IPC4 seems to be based on an array of manifest headers at the start of the firmware image. Those are rimage data, and not all platforms (in particular the fuzzer) are packing their output with rimage. Just stub this for now. In the source tree as it exists, there don't seem to be any extra components to build, nor is there any support I can see at the linker/rimage level to get them included anyway. Signed-off-by: Andy Ross <andyross@google.com>

Add new APIs needed for CONFIG_IPC_MAJOR_4=y builds. Note that many of these are fairly Intel-specific, meaning they probably don't belong in the platform layer. But in this case (fuzzing) it's probably worthwhile to enable as much code as possible vs. refactoring for purity. Future IPC4 hardware platforms will need to do that refactoring for us anyway. Signed-off-by: Andy Ross <andyross@google.com>

Don't use the IPC3 protocol whiteboxing for IPC4, the formats are different. Also IPC4 seems to send a LOT of zero-length empty replies. Those are causing spurious errors (memcmp_s fails when you pass it a null destination pointer, even when the length is zero), where the Intel IPC driver will just drop them. Suppress handling to match behavior, though this is probably a buglet worth investigating. Signed-off-by: Andy Ross <andyross@google.com>

Fuzzing produces (by design) an endless flood of protocol errors. Quiet the top level ones when in use, because otherwise the log output takes up most of the CPU time available that we'd rather spend on fuzzing. Same trick from IPC3. Signed-off-by: Andy Ross <andyross@google.com>

kv2019i · 2023-05-09T09:17:01Z

One known fail in https://sof-ci.01.org/sofpr/PR7531/build6911/devicetest/index.html , otherwise looks good.

In review comments, @andyross @lyakh there's the one superfluous variable init that is not addressed. I do think this is valid, but I'm inclined to proceed with merge as the rest of the PR is good and we have tests passing.

Hot on the heels of PR thesofproject#7531. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb · 2023-05-09T19:47:24Z

Small #7579 adds IPC4 fuzzing to Github Actions, please review.

Hot on the heels of PR #7531. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

andyross requested review from cujomalainey and marc-hb April 27, 2023 18:35

andyross requested review from RanderWang, aborisovich, dbaluta, lbetlej, lgirdwood, marcinszkudlinski, mmaka1, pblaszko and plbossart as code owners April 27, 2023 18:35

cujomalainey reviewed Apr 27, 2023

View reviewed changes

andyross added 3 commits May 2, 2023 08:50

base_fw: Add missing header

3b578ce

CLK_MAX_CPU_HZ is defined in platform/lib/clk.h, unmasked by posix work that lacks the same transitive headers. Signed-off-by: Andy Ross <andyross@google.com>

init.c: Missing header

dd9d36c

This calls mailbox APIs when building for IPC4, but was missing the header Signed-off-by: Andy Ross <andyross@google.com>

andyross mentioned this pull request May 2, 2023

[BUG] Platform-specific IPC4 code in platform-independent source files #7549

Closed

andyross force-pushed the ipc4-fuzz branch from 8814bdd to 1cd1427 Compare May 2, 2023 19:54

cujomalainey reviewed May 2, 2023

View reviewed changes

src/audio/base_fw.c Show resolved Hide resolved

andyross force-pushed the ipc4-fuzz branch from 1cd1427 to 42a2056 Compare May 2, 2023 21:41

cujomalainey approved these changes May 2, 2023

View reviewed changes

src/audio/base_fw.c Show resolved Hide resolved

lyakh reviewed May 3, 2023

View reviewed changes

andyross added 3 commits May 3, 2023 06:33

andyross force-pushed the ipc4-fuzz branch from 42a2056 to f2e782a Compare May 3, 2023 13:39

RanderWang approved these changes May 9, 2023

View reviewed changes

kv2019i approved these changes May 9, 2023

View reviewed changes

kv2019i merged commit 7f02b0b into thesofproject:main May 9, 2023

marc-hb added a commit to marc-hb/sof that referenced this pull request May 9, 2023

.github/fuzzer: add IPC matrix and IPC4 to new fuzzer

96859dd

Hot on the heels of PR thesofproject#7531. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

kv2019i pushed a commit that referenced this pull request May 11, 2023

.github/fuzzer: add IPC matrix and IPC4 to new fuzzer

45c3f13

Hot on the heels of PR #7531. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

IPC4 fuzzing support #7531

IPC4 fuzzing support #7531

Uh oh!

Conversation

andyross commented Apr 27, 2023

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cujomalainey Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

cujomalainey commented Apr 27, 2023

Uh oh!

marc-hb commented Apr 27, 2023

Uh oh!

andyross commented May 2, 2023

Uh oh!

andyross commented May 2, 2023

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-hb commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh May 3, 2023

Choose a reason for hiding this comment

Uh oh!

andyross May 3, 2023

Choose a reason for hiding this comment

Uh oh!

lyakh May 4, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kv2019i commented May 9, 2023

Uh oh!

marc-hb commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marc-hb commented May 2, 2023 •

edited

Loading