[PATCH] Add user-defined callback on write() calls to labeled data.

[Sam Kerner]

On Wed, Apr 2, 2014 at 7:20 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
>
>
>
> ================
> Comment at: lib/dfsan/dfsan.cc:261
> @@ +260,3 @@
> +
> +  dfsan_labeled_write_callback = labeled_write_callback;
> +
> ----------------
> I don't think this approach is correct. We can't store a pointer to the callback function (which uses the instrumented ABI) and call it from the dfsan runtime library using the uninstrumented ABI, as the labels received by the callback function will be wrong (nor is this guaranteed to work at all).
>
> We have a trampoline mechanism for safely calling instrumented ABI functions from the dfsan runtime library. Please take a look at how the custom functions pthread_create and dl_iterate_phdr are implemented.

Those methods take a function pointer as an argument, and I see that
the presence of a function pointer argument causes a trampoline
argument to be added to the call to __dfsw_* by the DataFlowSanitizer
class.  write() has no such callback.

I could make the function that sets the callback a custom method, so
that its custom implementation gets a trampoline argument.  Is that a
reasonable approach, or is there a better way to obtain the
trampoline?

>
> When you have done this, you should be able to improve your test to show that the values
> passed to the write callback have the same labels as those passed to the write function.
>
> ================
> Comment at: lib/dfsan/dfsan_custom.cc:808
> @@ +807,3 @@
> +SANITIZER_INTERFACE_ATTRIBUTE int
> +__dfsw_write(int fd, const void *buf, size_t count) {
> +  if (dfsan_labeled_write_callback != NULL) {
> ----------------
> The signature of this function is incorrect. It should accept labels for each of the parameters and a pointer to the return label.
>
> ================
> Comment at: lib/dfsan/dfsan_custom.cc:810
> @@ +809,3 @@
> +  if (dfsan_labeled_write_callback != NULL) {
> +    // If any label is set in |buf|, invoke the callback.
> +    int found_label = 0;
> ----------------
> It would probably be simpler to call the callback unconditionally and let it check buf for labels. This would also allow the client to observe unlabelled writes if it needs to for whatever reason.
>
> ================
> Comment at: lib/dfsan/dfsan_custom.cc:816
> @@ +815,3 @@
> +      if (dfsan_read_label(addr, 1) != 0) {
> +        found_label = 1;
> +      }
> ----------------
> You could exit early here.
>
> ================
> Comment at: test/dfsan/write_callback.c:1
> @@ +1,2 @@
> +// RUN: %clang_dfsan                        -m64 %s -o %t && %t %T/file1.txt
> +// RUN: %clang_dfsan -mllvm -dfsan-args-abi -m64 %s -o %t && %t %T/file2.txt
> ----------------
> Maybe we could check that these files have the contents we expect?
>
>
> http://llvm-reviews.chandlerc.com/D3268