Android Binder fuzzer #564
Conversation
rodionov
force-pushed
the
binder_fuzzer
branch
4 times, most recently
from
485d1cc to
154aa2b
Compare
tools/lkl/Targets
Outdated
|
|
||
| # The same list of absl dependencies as in libprotobuf-mutator cmake config: | ||
| # https://github.com/google/libprotobuf-mutator/blob/master/cmake/external/protobuf.cmake | ||
| LDLIBS_/binder-fuzzer-$(LKL_HOST_CONFIG_MMU) += \ |
There was a problem hiding this comment.
This looks like it could be easy to break as libprotobuf-mutator evolves. Is there a better way to do it? Or perhaps do some validation in the "autoconf" part?
There was a problem hiding this comment.
Thanks for flagging this, Tavi! Yeah, I see your concerns. I'm also not quite happy with libprotobuf-mutator integration. Tried to clean up things as much as possible but there is still some opportunity for the improvement, I guess. In this implementation we rely on a specific layout of libprotobuf-mutator build directory which is fragile. Some potential options in the order preference:
-
Use as-is but pin libprotobuf-mutator to a specific commit. We will update the documentation accordingly and will use the exact libprotobuf-mutator commit in all the automation scripts. Updates to libprotobuf-mutator will be manually tested to ensure the currently implemented build config works fine or needs some changes. This way we would avoid potential breakages to builds due to the evolved libprotobuf-mutator at a cost of not following tip of the tree of libprotobuf-mutator (which I think is not a big deal here compared to added stability). Happy to add validation to autoconf part to enforce the libprotobuf-mutator is using the right commit
-
Ship libprotobuf-mutator prebuilts inside LKL source tree. To build the fuzzer we would need (a) libprotobuf headers, (b) libprotobuf-mutator headers, (c) libprotobuf-mutator and librpotobuf static libc, (d) protoc protobuf compiler (needs to match the exact version of libprotobuf static libs and headers). We can potentially put the prebuilt objects somewhere under tools/lkl/fuzzers/prebuilts. The needed headers can be stored somewhere in LKL source tree too. Not 100% sure what license these headers should be under (currently they are not GPLv2).
-
Completely get rid of libprotobuf-mutator :) Would make source tree cleaner but would require major refactoring of the fuzzer and probably reduce its performance (libprotobuf-mutator enables structure-aware fuzzing what improves code coverage). Using libprotobuf-mutator brings good benefits for fuzzing and I would like to use for future LKL-based fuzzing projects if possible.
@tavip @thehajime what do you think?
There was a problem hiding this comment.
@rodionov although I don't think it's clean enough with the current state, we have similar dependent library, dpdk, which we put a detection under Makefile.autoconf with a supplemental script to prepare the library. that might be another option 4) for the protobuf integration.
https://github.com/lkl/linux/blob/master/tools/lkl/Makefile.autoconf#L46-L59
https://github.com/lkl/linux/blob/master/tools/lkl/scripts/dpdk-sdk-build.sh
There was a problem hiding this comment.
Thank you for pointing this out! I see, libprotobuf-mutator is quite similar -- let me change the PR according to dpdk example.
There was a problem hiding this comment.
@tavip, @thehajime I did some changes to address your recommendations:
- Added tools/lkl/scripts/libprotobuf-mutator-build.sh supplemental script which downloads and builds libprotobuf-mutator project pinning it to v1.4 version.
- Added additional configuration and checks to tools/lkl/Makefile.autoconf file which verifies that libprotobuf-mutator has all the necessary dependencies built and located at the expected paths.
both 1 and 2 expect PROTOBUF_MUTATOR_DIR which provides path to the directory with libprotobuf-mutator. I originally wanted to provide a default option for this variable somewhere inside LKL source tree but make clean would remove all the built static *.a libs regardless of the location of folder inside LKL source tree. Thus, it might be inconvenient to download libprotobuf-mutator inside LKL source tree and the caller needs to explicitly provide location of the libprotobuf-mutator located somewhere else via PROTOBUF_MUTATOR_DIR.
Let me know what you think. Happy to address any further comments or suggestions regarding this topic.
There was a problem hiding this comment.
Thanks @rodionov, it looks like the PR is missing some files and some some temporary / debug commits. I am fine with not setting a default value for PROTOBUF_MUTATOR_DIR. I think we should also update the tests to build the binder fuzzer to make sure we don't break it and maybe even run the CVE repro if possible.
There was a problem hiding this comment.
Thanks @tavip ! I'm sorry for pushing the wrong branch, just reuploaded the right one.
I think we should also update the tests to build the binder fuzzer to make sure we don't break it and maybe even run the CVE repro if possible.
+1 for adding a test to CI -- will be adding a new commit with the test shortly.
Current implementation of install_headers.py doesn't handle enums which
contain complex expressions such as macro invocations. For example, in
include/uapi/linux/android/binder.h the following enums are incorrectly
parsed due to commas inside macro _IOW('c', 0, ...) in:
```
enum binder_driver_command_protocol {
BC_TRANSACTION = _IOW('c', 0, struct binder_transaction_data),
BC_REPLY = _IOW('c', 1, struct binder_transaction_data),
...
}
```
This CL implements a workaround which detect presence of macros with
brackets and ignores commas in it.
As a limitation the proposed solution isn't generic engough to handle
nested macro invocations (i.e. MACRO1(MACRO2(...))) with unlimited depth
of brackets. However it offers an improvement over existing approach.
Signed-off-by: Zi Fan Tan <[email protected]>
Signed-off-by: Eugene Rodionov <[email protected]>
rodionov
force-pushed
the
binder_fuzzer
branch
2 times, most recently
from
638ed2e to
c8fd6f7
Compare
The instructions on how to build the fuzzer are provided in tools/lkl/fuzzers/binder/README.md Signed-off-by: Zi Fan Tan <[email protected]> Signed-off-by: Eugene Rodionov <[email protected]>
Currently Makefile.conf is included in tools/lkl/Makefile using `-include` construct which won't generate an error if Makefile.conf is not present which might happen due to an error while executing Makefile.autoconf. As Makefile.conf contains important LKL configuration options `make` should not proceed with building targets. Do the same for inclusion of Target and ../scripts/Makefile.include files. Signed-off-by: Eugene Rodionov <[email protected]>
rodionov
force-pushed
the
binder_fuzzer
branch
from
c8fd6f7 to
3a8bc33
Compare
| @@ -42,15 +42,15 @@ conf: $(OUTPUT)Makefile.conf | |||
| $(OUTPUT)Makefile.conf $(OUTPUT)include/kernel_config.h $(OUTPUT)/kernel.config: Makefile.autoconf | |||
| $(call QUIET_AUTOCONF, headers)$(MAKE) -f Makefile.autoconf -s | |||
|
|
|||
| -include $(OUTPUT)Makefile.conf | |||
| include $(OUTPUT)Makefile.conf | |||
There was a problem hiding this comment.
TIL: https://www.gnu.org/software/make/manual/html_node/Include.html
If an included makefile cannot be found in any of these directories it is not an immediately fatal error; processing of the makefile containing the include continues. Once it has finished reading makefiles, make will try to remake any that are out of date or don’t exist.
Nice, thank you @rodionov !
| @@ -48,6 +48,8 @@ export CFLAGS += -I$(OUTPUT)/include -Iinclude -Wall -g -O2 -Wextra \ | |||
| -Wno-unused-parameter \ | |||
| -Wno-missing-field-initializers -fno-strict-aliasing | |||
|
|
|||
| export CXXFLAGS += $(CFLAGS) -Wno-address-of-temporary | |||
There was a problem hiding this comment.
Can this be moved to fuzzers/binder/Build?
| $(OUTPUT)fuzzers/binder%$(EXESUF): $(OUTPUT)fuzzers/binder%-in.o $(OUTPUT)liblkl.a | ||
| $(QUIET_LINK)$(CXX) $(LDFLAGS) $(LDFLAGS_$*-y) -o $@ $^ $(LDLIBS) $(LDLIBS_$*-y) | ||
|
|
||
| $(OUTPUT)fuzzers/binder/binder-fuzzer-in.o: gen_proto_files |
There was a problem hiding this comment.
Why not define a rule for for building proto files here instead of running compile_proto.sh?
$(OUTPUT)%.pb.cc: %.proto
$(QUIET_PROTOC)$(PROTOC) ...
Hi @tavip @thehajime,
Here is a couple of commits implementing @abcSup's Binder fuzzer.