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

Update fuzzing rig, make component creation work #7028

Merged
merged 3 commits into from
Feb 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions scripts/fuzz.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#!/bin/bash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you generally disagree but asking anyway: can you please move this to a main "$@" function? Rationale in thesofproject/sof-test#740

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do, but this is your playground so no arguments here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyross your playground too !

set -e
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in most other scripts:

SOF_TOP=$(cd "$(dirname "$0")/.." && pwd)


# Simple wrapper around a libfuzzer test run, as much for
# documentation as direct use. The idea here is really simple: build
# for the Zephyr "native_posix" board (which is a just a x86
# executable for the build host, not an emulated device) and run the
# resulting zephyr.exe file. This specifies a "fuzz_corpus" directory
# to save the seeds that produce useful coverage output for use in
# repeated runs (these are not particularly large, we might consider
# curating and commiting such a seed directory to the tree).
#
# The tool will run until it finds a failure condition. You will see
# MANY errors on stdout from all the randomized input. Don't try to
# capture this, either let it output to a terminal or arrange to keep
# only the last XXX lines after the tool exits.
#
# The only prerequisite to install is a clang compiler on the host.
# Versions 12+ have all been observed to work.
#
# You will need the kconfigs specified below for correct operation,
# but can add more at the end of this script's command line to
# duplicate configurations as needed. Alternatively you can pass
# overlay files in kconfig syntax via -DOVERLAY_CONFIG=..., etc...

export SOF_TOP=$(cd "$(dirname "$0")/.." && pwd)

export ZEPHYR_BASE=$SOF_TOP/../zephyr
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used ZEPHYR_BASE for a long time, are you sure it's required in this case? Can this script be run outside a west workspace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed in the default setup. But I have like a thousand zephyr trees floating around and do dumb stuff like symlinking module directories across them. If you have ZEPHYR_BASE set to the wrong tree, you get very funny results (like, the build works, but uses the wrong SOF tree!). So I guess I'd see this as a best practices kind of thing to ensure that the tree this script is running from is the one being built.

export ZEPHYR_TOOLCHAIN_VARIANT=llvm

main()
{
west build -p -b native_posix $SOF_TOP/app/ -- \
-DCONFIG_ASSERT=y \
-DCONFIG_SYS_HEAP_BIG_ONLY=y \
-DCONFIG_ZEPHYR_NATIVE_DRIVERS=y \
-DCONFIG_ARCH_POSIX_LIBFUZZER=y \
-DCONFIG_ARCH_POSIX_FUZZ_TICKS=100 \
-DCONFIG_ASAN=y "$@"

mkdir -p ./fuzz_corpus
build/zephyr/zephyr.exe ./fuzz_corpus
}

main "$@"
2 changes: 2 additions & 0 deletions src/platform/posix/include/platform/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@

#define PLATFORM_DEFAULT_DELAY 12

struct sof;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extern?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok as a forward declaration.

Copy link
Collaborator

@lyakh lyakh Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i IIRC there is a weird (IMHO) C feature, that yes, you can use this in a header and the compiler will "guess what you actually mean" and only create one instance of the object, but I'd rather use an explicit extern the same way as we do everywhere, particularly since at least me personally I'm not sure which C standard we settled with and from which version that feature was introduced

Copy link
Collaborator

@marc-hb marc-hb Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a very good description of that portability issue at the end of the chapter "Declarations" in the Harbison & Steele (which every C developer should have IMHO). For maximum portability across various toolchains (which I think matters to SOF), they definitely agree with @lyakh .

Follow-up PR? Could also fix some other checkpatch warnings inhttps://github.com/thesofproject/sof/actions/runs/4118174248/jobs/7110383634

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humm, the use of 'struct foo' is very very common in Linux headers. I'd be surprised if you saw any 'extern' in any of the ALSA/ASoC headers...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until recently the Linux kernel supported a single compiler. I doubt ALSA is super interested in C portability either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is a type declaration, not an object definition. All it means is that "struct sof exists as a type and you can legally take a pointer to it". You see this construct in C++ a lot, because it avoids the need to pull in the very expensive headers that define types. The reason it was introduced here is that I had a spot further down where I declare a function that takes a "struct sof *" as an argument, and was too lazy to figure out where that was declared.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is a type declaration, not an object definition.

Thanks @andyross , I read too fast and stand corrected (and I think you meant: "not an object... declaration" :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is a type declaration, not an object definition.

ouch... /me looking at those "computer glasses" and contemplates if he should wear them more frequently... Then hides away

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, of course it's a perfectly fine forward type declaration. And I myself frequently suggest it when just a pointer to that type is needed - exactly as @andyross explained, and I think that's usually preferred over including a header just for that. And that's also why it's used in Linux, in SOF, etc. I just somehow misread it as struct sof sof;...


void posix_dma_init(struct sof *sof);
void posix_dai_init(struct sof *sof);

Expand Down
71 changes: 64 additions & 7 deletions src/platform/posix/ipc.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <sof/ipc/common.h>
#include <sof/ipc/schedule.h>
#include <sof/schedule/edf_schedule.h>
#include <sof/audio/component_ext.h>

// 6c8f0d53-ff77-4ca1-b825-c0c4e1b0d322
DECLARE_SOF_UUID("posix-ipc-task", ipc_task_uuid,
Expand Down Expand Up @@ -46,6 +47,8 @@ static uint8_t fuzz_in_sz;
static void fuzz_isr(const void *arg)
{
size_t rem, i, n = MIN(posix_fuzz_sz, sizeof(fuzz_in) - fuzz_in_sz);
bool comp_new = false;
int comp_idx = 0;

for (i = 0; i < n; i++)
fuzz_in[fuzz_in_sz++] = posix_fuzz_buf[i];
Expand All @@ -58,20 +61,74 @@ static void fuzz_isr(const void *arg)

size_t maxsz = SOF_IPC_MSG_MAX_SIZE - 4, msgsz = fuzz_in[0] * 2;

memset(global_ipc->comp_data, 0, maxsz);
n = MIN(msgsz, MIN(fuzz_in_sz - 1, maxsz));
rem = fuzz_in_sz - (n + 1);

memset(global_ipc->comp_data, 0, maxsz);
memcpy(global_ipc->comp_data, &fuzz_in[1], n);
memmove(&fuzz_in[0], &fuzz_in[n + 1], rem);
fuzz_in_sz = rem;

// One special case: a first byte of 0xff (which is in the
// otherwise-ignored size value at the front of the command --
// we rewrite those) is interpreted as a "component new"
// command, which we format specially, with a driver index
// specified by the second byte (modulo the number of
// registered drivers). This command involves matching
// against a UUID value, which even fuzzing isn't able to
// discover at runtime. So unless we whitebox this, no
// components will be created.
if (n > 2 && ((uint8_t *)global_ipc->comp_data)[0] == 0xff) {
comp_new = true;
comp_idx = ((uint8_t *)global_ipc->comp_data)[1];
}

// The first dword is a size value which fuzzing will stumble
// on only one time in 24M, fill it in manually.
// on only rarely, fill it in manually.
*(uint32_t *)global_ipc->comp_data = msgsz;
for (i = 0; i < n; i++) {
uint8_t *cmd = global_ipc->comp_data; // why is it a void*?

cmd[i + 4] = fuzz_in[i + 1];
struct sof_ipc_comp *cc = global_ipc->comp_data;
Copy link
Collaborator

@marc-hb marc-hb Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/platform/posix/ipc.c:90:30: warning: unused variable ‘cc’ [-Wunused-variable]
   90 |         struct sof_ipc_comp *cc = global_ipc->comp_data;
      |                              ^~

... in some -DCONFIG... combination which I forgot, sorry.


// "Adjust" the command to represent a "comp new" command per
// above. Basically we want to copy in the UUID value for one
// of the runtime-enumerated drivers based on data already
// randomized in the fuzz command.
if (comp_new) {
struct {
struct sof_ipc_comp comp;
struct sof_ipc_comp_ext ext;
} *cmd = global_ipc->comp_data;

// Set global/command type fields to TPLG_MSG/TPLG_COMP_NEW
cmd->comp.hdr.cmd &= 0x0000ffff;
cmd->comp.hdr.cmd |= SOF_IPC_GLB_TPLG_MSG;
cmd->comp.hdr.cmd |= SOF_IPC_TPLG_COMP_NEW;

// We have only one core available in native_posix
cmd->comp.core = 0;

// Fix up cmd size and ext_data_length to match
if (cmd->comp.hdr.size < sizeof(*cmd))
cmd->comp.hdr.size = sizeof(*cmd);
cmd->comp.ext_data_length = cmd->comp.hdr.size - sizeof(cmd->comp);

// Extract the list of available component drivers (do
// it every time; in practice I don't think this
// changes at runtime but in principle it might in the
// future)
int ndrvs = 0;
static struct comp_driver_info *drvs[256];
struct list_item *iter;
struct comp_driver_list *dlist = comp_drivers_get();
list_for_item(iter, &dlist->list) {
struct comp_driver_info *inf =
container_of(iter, struct comp_driver_info, list);
drvs[ndrvs++] = inf;
}

struct comp_driver_info *di = drvs[comp_idx % ndrvs];
memcpy(cmd->ext.uuid, di->drv->uid, sizeof(cmd->ext.uuid));
}
memmove(&fuzz_in[0], &fuzz_in[n + 1], rem);
fuzz_in_sz = rem;

posix_ipc_isr(NULL);
}
Expand Down