Re: [RFC PATCH v2 1/6] kcov: add per-task dataflow tracking for function arguments/return values

[Yunseong Kim]

Hi Alexander,

>> - Per-task buffer: task->kcov_df_area with atomic xadd reservation
> 
> I don't understand this line...
> 
>> - Recursion-safe: notrace __no_sanitize_coverage noinline
>> - ERR_PTR aware: skips struct expansion for error pointers
> 
> ... and this.

I updated this text at v2 patch.

>>
>> The callbacks (__sanitizer_cov_trace_args/ret) are inserted by the
>> compiler when -fsanitize-coverage=dataflow-args,dataflow-ret is used.
>> The Kconfig options depend on cc-option to verify compiler support.
>>
>> Buffer format (TLV records, all u64):
>>   area[0]: atomic word count
>>   [pos+0]: type_and_seq (0xE=entry, 0xF=return in upper 4 bits)
>>   [pos+1]: PC
>>   [pos+2]: meta (arg_idx | arg_size | ptr)
>>   [pos+3..N]: field values read via copy_from_kernel_nofault()
>>
>> This is completely independent from legacy /sys/kernel/debug/kcov.
>> Existing users (syzkaller, oss-fuzz) are unaffected.
> 
> Does oss-fuzz even use kcov?

Also, I removed this text at v2 patch. I mistakenly confused it with another
usage of KCOV with a other fuzzer.

  https://security.googleblog.com/2024/06/hacking-for-defenders-approaches-to.html

>>
>> Signed-off-by: Yunseong Kim <yunseong.kim@xxxxxxxx>
>> ---
>>  include/linux/sched.h |   8 ++
>>  kernel/kcov.c         | 291 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/Kconfig.debug     |  22 ++++
>>  3 files changed, 321 insertions(+)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index c4433c185ad8..03be4b495f70 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1533,6 +1533,14 @@ struct task_struct {
>>         /* KCOV sequence number: */
>>         int                             kcov_sequence;
>>
>> +       /* KCOV dataflow per-task sequence counter for TLV records: */
>> +       u32                             kcov_dataflow_seq;
>> +
>> +       /* KCOV dataflow: separate buffer for trace-args/trace-ret */
>> +       unsigned int                    kcov_df_size;
>> +       void                            *kcov_df_area;
>> +       bool                            kcov_df_enabled;
>> +
>>         /* Collect coverage from softirq context: */
>>         unsigned int                    kcov_softirq;
>>  #endif
>> diff --git a/kernel/kcov.c b/kernel/kcov.c
>> index 1df373fb562b..d3c9c0efe961 100644
>> --- a/kernel/kcov.c
>> +++ b/kernel/kcov.c
>> @@ -353,6 +353,288 @@ void notrace __sanitizer_cov_trace_switch(kcov_u64 val, void *arg)
>>  EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
>>  #endif /* ifdef CONFIG_KCOV_ENABLE_COMPARISONS */
>>
>> +#if defined(CONFIG_KCOV_DATAFLOW_ARGS) || defined(CONFIG_KCOV_DATAFLOW_RET)
>> +/*
>> + * KCOV Dataflow: /sys/kernel/debug/kcov_dataflow
>> + *
>> + * Completely separate from legacy /sys/kernel/debug/kcov.
> 
> Since this code is completely separate, could it be put into a separate file?
> I think kcov.c is too big already.

Thank you again for your guide, I updated it at v2.

>> + * Own buffer, own ioctl, own mmap. No printk — buffer only.
> 
> Can you please not use these long dashes in C code?

I removed all a the v2.

>> +/*
>> + * Core write function — no printk, no locks, just atomic buffer write.
> 
> I think it's okay to omit what this function is not doing.
> 
> 
>> +
>> +       /* Atomic reservation */
>> +       pos = 1 + xadd((unsigned long *)&area[0], record_len);
>> +       if (unlikely(pos + record_len > max_pos)) {
>> +               xadd((unsigned long *)&area[0], -(long)record_len);
>> +               return;
>> +       }
> 
> Have you tried compiling this code on ARM64?
> I am pretty sure they don't have xadd(), so it won't work.
> But why do we need an atomic increment here at all? write_comp_data()
> performs the same job, and does not need it.
> Or am I missing something?

Thank you again for pointing out. After updating to the READ_ONCE/WRITE_ONCE
atomic pattern, Testing results based on v2 on arm64 for the Rust for Linux
kernel module(eight_struct_args_rust) are as follows:

 do_el0_svc({0xffffffffffffff9c, 0xffffffffffffff9c, 0xffffffff, 0x0, 0x0, 0x0})
   invoke_syscall({0xffffffffffffff9c, 0xffffffffffffff9c, 0x38, 0x0, 0x0, 0x0}, 0x38)
     __arm64_sys_openat({0xffffffffffffff9c, 0xffffffffffffff9c, 0x38, 0x0, 0x0, 0x0})
       ksys_write(0xffff9a031231, 0x1)
         fdget_pos(0x4)
         0xffff000004421cc0 = fdget_pos()
       0x0 = vfs_write()
       vfs_write(0xffff9a031231, 0x1, 0x0)
       0x0 = _RNvCsdfZGIOKgjaD_22eight_struct_args_rust13write_handler [eight_struct_args_rust]()
       _RNvCsdfZGIOKgjaD_22eight_struct_args_rust13write_handler [eight_struct_args_rust](0xffff9a031231, 0x1, 0x0)
         rsf_1 [eight_struct_args_rust](0x11)
         0x11 = rsf_1 [eight_struct_args_rust]()
         rsf_2 [eight_struct_args_rust](0x11, {0x11, 0x22})
         0x33 = rsf_2 [eight_struct_args_rust]()
         rsf_4 [eight_struct_args_rust](0x11, {0x11, 0x22}, {0x11, 0x22, 0x33}, {0x11, 0x22, 0x33, 0x44})
         0xaa = rsf_4 [eight_struct_args_rust]()

 ...

Latest test results from Github CI:

  https://github.com/yskzalloc/kcov-dataflow/actions/runs/27397351811/job/80967927283

Best regards,
Yunseong