Message-ID: <CACT4Y+ZZCBJid5vv7-rqXeXg_NvqgJNOtWmm96e0EyR=_3LLmw@mail.gmail.com>
Date:   Tue, 12 Sep 2017 19:41:14 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Andrea Parri <parri.andrea@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Potapenko <glider@...gle.com>,
        Kees Cook <keescook@...gle.com>,
        Vegard Nossum <vegard.nossum@...cle.com>,
        Quentin Casasnovas <quentin.casasnovas@...cle.com>,
        syzkaller <syzkaller@...glegroups.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH 1/3] kcov: support comparison operands collection

On Thu, Sep 7, 2017 at 6:29 PM, Andrea Parri <parri.andrea@...il.com> wrote:
> Hi Dmitry,
>
>
> On Aug 30, 2017 6:26 PM, "Dmitry Vyukov" <dvyukov@...gle.com> wrote:
>
> From: Victor Chibotaru <tchibo@...gle.com>
>
> Enables kcov to collect comparison operands from instrumented code.
> This is done by using Clang's -fsanitize=trace-cmp instrumentation
> (currently not available for GCC).
>
> The comparison operands help a lot in fuzz testing. E.g. they are
> used in Syzkaller to cover the interiors of conditional statements
> with way less attempts and thus make previously unreachable code
> reachable.
>
> To allow separate collection of coverage and comparison operands two
> different work modes are implemented. Mode selection is now done via
> a KCOV_ENABLE ioctl call with corresponding argument value.
>
> Signed-off-by: Victor Chibotaru <tchibo@...gle.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Mark Rutland <mark.rutland@....com>
> Cc: Alexander Popov <alex.popov@...ux.com>
> Cc: Andrey Ryabinin <aryabinin@...tuozzo.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Vegard Nossum <vegard.nossum@...cle.com>
> Cc: Quentin Casasnovas <quentin.casasnovas@...cle.com>
> Cc: syzkaller@...glegroups.com
> Cc: linux-mm@...ck.org
> Cc: linux-kernel@...r.kernel.org
> ---
> Clang instrumentation:
> https://clang.llvm.org/docs/SanitizerCoverage.html#tracing-data-flow
> Syzkaller:
> https://github.com/google/syzkaller
> ---
>  include/linux/kcov.h      |  12 ++-
>  include/uapi/linux/kcov.h |  32 ++++++++
>  kernel/kcov.c             | 203
> ++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 209 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> index 2883ac98c280..87e2a44f1bab 100644
> --- a/include/linux/kcov.h
> +++ b/include/linux/kcov.h
> @@ -7,19 +7,23 @@ struct task_struct;
>
>  #ifdef CONFIG_KCOV
>
> -void kcov_task_init(struct task_struct *t);
> -void kcov_task_exit(struct task_struct *t);
> -
>  enum kcov_mode {
>         /* Coverage collection is not enabled yet. */
>         KCOV_MODE_DISABLED = 0,
> +       /* KCOV was initialized, but tracing mode hasn't been chosen yet. */
> +       KCOV_MODE_INIT = 1,
>         /*
>          * Tracing coverage collection mode.
>          * Covered PCs are collected in a per-task buffer.
>          */
> -       KCOV_MODE_TRACE = 1,
> +       KCOV_MODE_TRACE_PC = 2,
> +       /* Collecting comparison operands mode. */
> +       KCOV_MODE_TRACE_CMP = 3,
>  };
>
> +void kcov_task_init(struct task_struct *t);
> +void kcov_task_exit(struct task_struct *t);
> +
>  #else
>
>  static inline void kcov_task_init(struct task_struct *t) {}
> diff --git a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h
> index 574e22ec640d..a0bc3e6a5ff7 100644
> --- a/include/uapi/linux/kcov.h
> +++ b/include/uapi/linux/kcov.h
> @@ -7,4 +7,36 @@
>  #define KCOV_ENABLE                    _IO('c', 100)
>  #define KCOV_DISABLE                   _IO('c', 101)
>
> +enum {
> +       /*
> +        * Tracing coverage collection mode.
> +        * Covered PCs are collected in a per-task buffer.
> +        * In new KCOV version the mode is chosen by calling
> +        * ioctl(fd, KCOV_ENABLE, mode). In older versions the mode argument
> +        * was supposed to be 0 in such a call. So, for reasons of backward
> +        * compatibility, we have chosen the value KCOV_TRACE_PC to be 0.
> +        */
> +       KCOV_TRACE_PC = 0,
> +       /* Collecting comparison operands mode. */
> +       KCOV_TRACE_CMP = 1,
> +};
> +
> +/*
> + * Defines the format for the types of collected comparisons.
> + */
> +enum kcov_cmp_type {
> +       /*
> +        * LSB shows whether one of the arguments is a compile-time
> constant.
> +        */
> +       KCOV_CMP_CONST = 1,
> +       /*
> +        * Second and third LSBs contain the size of arguments (1/2/4/8
> bytes).
> +        */
> +       KCOV_CMP_SIZE1 = 0,
> +       KCOV_CMP_SIZE2 = 2,
> +       KCOV_CMP_SIZE4 = 4,
> +       KCOV_CMP_SIZE8 = 6,
> +       KCOV_CMP_SIZE_MASK = 6,
> +};
> +
>  #endif /* _LINUX_KCOV_IOCTLS_H */
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index cd771993f96f..2abce5dfa2df 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -21,13 +21,21 @@
>  #include <linux/kcov.h>
>  #include <asm/setup.h>
>
> +/* Number of words written per one comparison. */
> +#define KCOV_WORDS_PER_CMP 3
> +
>  /*
>   * kcov descriptor (one per opened debugfs file).
>   * State transitions of the descriptor:
>   *  - initial state after open()
>   *  - then there must be a single ioctl(KCOV_INIT_TRACE) call
>   *  - then, mmap() call (several calls are allowed but not useful)
> - *  - then, repeated enable/disable for a task (only one task a time
> allowed)
> + *  - then, ioctl(KCOV_ENABLE, arg), where arg is
> + *     KCOV_TRACE_PC - to trace only the PCs
> + *     or
> + *     KCOV_TRACE_CMP - to trace only the comparison operands
> + *  - then, ioctl(KCOV_DISABLE) to disable the task.
> + * Enabling/disabling ioctls can be repeated (only one task a time
> allowed).
>   */
>  struct kcov {
>         /*
> @@ -47,6 +55,30 @@ struct kcov {
>         struct task_struct      *t;
>  };
>
> +static bool check_kcov_mode(enum kcov_mode needed_mode, struct task_struct
> *t)
> +{
> +       enum kcov_mode mode;
> +
> +       /*
> +        * We are interested in code coverage as a function of a syscall
> inputs,
> +        * so we ignore code executed in interrupts.
> +        */
> +       if (!t || !in_task())
> +               return false;
> +       mode = READ_ONCE(t->kcov_mode);
> +       /*
> +        * There is some code that runs in interrupts but for which
> +        * in_interrupt() returns false (e.g. preempt_schedule_irq()).
> +        * READ_ONCE()/barrier() effectively provides load-acquire wrt
> +        * interrupts, there are paired barrier()/WRITE_ONCE() in
> +        * kcov_ioctl_locked().
>
>
> Could you elaborate on this: what do you mean by "... provides load-acquire
> _wrt_ interrupts?" and why so?
>
>   Andrea


Without this barrier we can get the following situation.
Consider that ioctl sets t->kcov_mode but does not set t->kcov_area
yet (and/or __sanitizer_cov_trace_pc reads area ahead of mode), then
an interrupt on this task comes but in_task() returns true (observed
for preempt_schedule_irq()). Now the interrupt can decide that tracing
is enabled and write to area (which is NULL) which will cause panic
and crash.
We effectively need an acquire/release pair here, but only between the
task and an interrupt running on this task. So we don't need hardware
barriers, only compiler ordering.




> +        */
> +       barrier();
> +       if (mode != needed_mode)
> +               return false;
> +       return true;
> +}
> +
>  /*
>   * Entry point from instrumented code.
>   * This is called once per basic-block/edge.
> @@ -54,44 +86,136 @@ struct kcov {
>  void notrace __sanitizer_cov_trace_pc(void)
>  {
>         struct task_struct *t;
> -       enum kcov_mode mode;
> +       unsigned long *area;
> +       unsigned long ip = _RET_IP_;
> +       unsigned long pos;
>
>         t = current;
> -       /*
> -        * We are interested in code coverage as a function of a syscall
> inputs,
> -        * so we ignore code executed in interrupts.
> -        */
> -       if (!t || !in_task())
> +       if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
>                 return;
> -       mode = READ_ONCE(t->kcov_mode);
> -       if (mode == KCOV_MODE_TRACE) {
> -               unsigned long *area;
> -               unsigned long pos;
> -               unsigned long ip = _RET_IP_;
>
>  #ifdef CONFIG_RANDOMIZE_BASE
> -               ip -= kaslr_offset();
> +       ip -= kaslr_offset();
>  #endif
>
> -               /*
> -                * There is some code that runs in interrupts but for which
> -                * in_interrupt() returns false (e.g.
> preempt_schedule_irq()).
> -                * READ_ONCE()/barrier() effectively provides load-acquire
> wrt
> -                * interrupts, there are paired barrier()/WRITE_ONCE() in
> -                * kcov_ioctl_locked().
> -                */
> -               barrier();
> -               area = t->kcov_area;
> -               /* The first word is number of subsequent PCs. */
> -               pos = READ_ONCE(area[0]) + 1;
> -               if (likely(pos < t->kcov_size)) {
> -                       area[pos] = ip;
> -                       WRITE_ONCE(area[0], pos);
> -               }
> +       area = t->kcov_area;
> +       /* The first word is number of subsequent PCs. */
> +       pos = READ_ONCE(area[0]) + 1;
> +       if (likely(pos < t->kcov_size)) {
> +               area[pos] = ip;
> +               WRITE_ONCE(area[0], pos);
>         }
>  }
>  EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>
> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
> +static void write_comp_data(u64 type, u64 arg1, u64 arg2)
> +{
> +       struct task_struct *t;
> +       u64 *area;
> +       u64 count, start_index, end_pos, max_pos;
> +
> +       t = current;
> +       if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
> +               return;
> +
> +       /*
> +        * We write all comparison arguments and types as u64.
> +        * The buffer was allocated for t->kcov_size unsigned longs.
> +        */
> +       area = (u64 *)t->kcov_area;
> +       max_pos = t->kcov_size * sizeof(unsigned long);
> +
> +       count = READ_ONCE(area[0]);
> +
> +       /* Every record is KCOV_WORDS_PER_CMP words. */
> +       start_index = 1 + count * KCOV_WORDS_PER_CMP;
> +       end_pos = (start_index + KCOV_WORDS_PER_CMP) * sizeof(u64);
> +       if (likely(end_pos <= max_pos)) {
> +               area[start_index] = type;
> +               area[start_index + 1] = arg1;
> +               area[start_index + 2] = arg2;
> +               WRITE_ONCE(area[0], count + 1);
> +       }
> +}
> +
> +void notrace __sanitizer_cov_trace_cmp1(u8 arg1, u8 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE1, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_cmp1);
> +
> +void notrace __sanitizer_cov_trace_cmp2(u16 arg1, u16 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE2, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_cmp2);
> +
> +void notrace __sanitizer_cov_trace_cmp4(u16 arg1, u16 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE4, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_cmp4);
> +
> +void notrace __sanitizer_cov_trace_cmp8(u64 arg1, u64 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE8, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_cmp8);
> +
> +void notrace __sanitizer_cov_trace_const_cmp1(u8 arg1, u8 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE1 | KCOV_CMP_CONST, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp1);
> +
> +void notrace __sanitizer_cov_trace_const_cmp2(u16 arg1, u16 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE2 | KCOV_CMP_CONST, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp2);
> +
> +void notrace __sanitizer_cov_trace_const_cmp4(u16 arg1, u16 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE4 | KCOV_CMP_CONST, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp4);
> +
> +void notrace __sanitizer_cov_trace_const_cmp8(u64 arg1, u64 arg2)
> +{
> +       write_comp_data(KCOV_CMP_SIZE8 | KCOV_CMP_CONST, arg1, arg2);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp8);
> +
> +void notrace __sanitizer_cov_trace_switch(u64 val, u64 *cases)
> +{
> +       u64 i;
> +       u64 count = cases[0];
> +       u64 size = cases[1];
> +       u64 type = KCOV_CMP_CONST;
> +
> +       switch (size) {
> +       case 8:
> +               type |= KCOV_CMP_SIZE1;
> +               break;
> +       case 16:
> +               type |= KCOV_CMP_SIZE2;
> +               break;
> +       case 32:
> +               type |= KCOV_CMP_SIZE4;
> +               break;
> +       case 64:
> +               type |= KCOV_CMP_SIZE8;
> +               break;
> +       default:
> +               return;
> +       }
> +       for (i = 0; i < count; i++)
> +               write_comp_data(type, cases[i + 2], val);
> +}
> +EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
> +#endif /* ifdef CONFIG_KCOV_ENABLE_COMPARISONS */
> +
>  static void kcov_get(struct kcov *kcov)
>  {
>         atomic_inc(&kcov->refcount);
> @@ -128,6 +252,7 @@ void kcov_task_exit(struct task_struct *t)
>         /* Just to not leave dangling references behind. */
>         kcov_task_init(t);
>         kcov->t = NULL;
> +       kcov->mode = KCOV_MODE_INIT;
>         spin_unlock(&kcov->lock);
>         kcov_put(kcov);
>  }
> @@ -146,7 +271,7 @@ static int kcov_mmap(struct file *filep, struct
> vm_area_struct *vma)
>
>         spin_lock(&kcov->lock);
>         size = kcov->size * sizeof(unsigned long);
> -       if (kcov->mode == KCOV_MODE_DISABLED || vma->vm_pgoff != 0 ||
> +       if (kcov->mode != KCOV_MODE_INIT || vma->vm_pgoff != 0 ||
>             vma->vm_end - vma->vm_start != size) {
>                 res = -EINVAL;
>                 goto exit;
> @@ -175,6 +300,7 @@ static int kcov_open(struct inode *inode, struct file
> *filep)
>         kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
>         if (!kcov)
>                 return -ENOMEM;
> +       kcov->mode = KCOV_MODE_DISABLED;
>         atomic_set(&kcov->refcount, 1);
>         spin_lock_init(&kcov->lock);
>         filep->private_data = kcov;
> @@ -210,7 +336,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned
> int cmd,
>                 if (size < 2 || size > INT_MAX / sizeof(unsigned long))
>                         return -EINVAL;
>                 kcov->size = size;
> -               kcov->mode = KCOV_MODE_TRACE;
> +               kcov->mode = KCOV_MODE_INIT;
>                 return 0;
>         case KCOV_ENABLE:
>                 /*
> @@ -220,17 +346,25 @@ static int kcov_ioctl_locked(struct kcov *kcov,
> unsigned int cmd,
>                  * at task exit or voluntary by KCOV_DISABLE. After that it
> can
>                  * be enabled for another task.
>                  */
> -               unused = arg;
> -               if (unused != 0 || kcov->mode == KCOV_MODE_DISABLED ||
> -                   kcov->area == NULL)
> +               if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
>                         return -EINVAL;
>                 if (kcov->t != NULL)
>                         return -EBUSY;
> +               if (arg == KCOV_TRACE_PC)
> +                       kcov->mode = KCOV_MODE_TRACE_PC;
> +               else if (arg == KCOV_TRACE_CMP)
> +#ifdef CONFIG_KCOV_ENABLE_COMPARISONS
> +                       kcov->mode = KCOV_MODE_TRACE_CMP;
> +#else
> +               return -ENOTSUPP;
> +#endif
> +               else
> +                       return -EINVAL;
>                 t = current;
>                 /* Cache in task struct for performance. */
>                 t->kcov_size = kcov->size;
>                 t->kcov_area = kcov->area;
> -               /* See comment in __sanitizer_cov_trace_pc(). */
> +               /* See comment in check_kcov_mode(). */
>                 barrier();
>                 WRITE_ONCE(t->kcov_mode, kcov->mode);
>                 t->kcov = kcov;
> @@ -248,6 +382,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned
> int cmd,
>                         return -EINVAL;
>                 kcov_task_init(t);
>                 kcov->t = NULL;
> +               kcov->mode = KCOV_MODE_INIT;
>                 kcov_put(kcov);
>                 return 0;
>         default:
> --
> 2.14.1.581.gf28d330327-goog
>
>

Powered by blists - more mailing lists