Message-ID: <DM8PR02MB824798E699AC9F4B2510E293F89C9@DM8PR02MB8247.namprd02.prod.outlook.com>
Date:   Fri, 19 Nov 2021 11:09:00 +0000
From:   "JianGen Jiao (Joey)" <jiangenj@....qualcomm.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>,
        "JianGen Jiao (QUIC)" <quic_jiangenj@...cinc.com>
CC:     "andreyknvl@...il.com" <andreyknvl@...il.com>,
        "kasan-dev@...glegroups.com" <kasan-dev@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Alexander Lochmann <info@...xander-lochmann.de>,
        "Likai Ding (QUIC)" <quic_likaid@...cinc.com>
Subject: RE: [PATCH] kcov: add KCOV_PC_RANGE to limit pc range

Yes, on x86_64, module address space is after kernel. But like below on arm64, it's different.

# grep stext /proc/kallsyms
ffffffc010010000 T _stext
# cat /proc/modules |sort -k 6 | tail -2
Some_module_1 552960 0 - Live 0xffffffc00ca05000 (O)
Some_module_1 360448 0 - Live 0xffffffc00cb8f000 (O)
# cat /proc/modules |sort -k 6 | head -2
Some_module_3 16384 1 - Live 0xffffffc009430000

-----Original Message-----
From: Dmitry Vyukov <dvyukov@...gle.com> 
Sent: Friday, November 19, 2021 6:38 PM
To: JianGen Jiao (QUIC) <quic_jiangenj@...cinc.com>
Cc: andreyknvl@...il.com; kasan-dev@...glegroups.com; LKML <linux-kernel@...r.kernel.org>; Alexander Lochmann <info@...xander-lochmann.de>; Likai Ding (QUIC) <quic_likaid@...cinc.com>
Subject: Re: [PATCH] kcov: add KCOV_PC_RANGE to limit pc range

WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.

On Fri, 19 Nov 2021 at 04:17, JianGen Jiao (QUIC) <quic_jiangenj@...cinc.com> wrote:
>
> Hi Dmitry,
> I'm using the start, end pc from cover filter, which currently is the fast way compared to the big bitmap passing from syzkaller solution, as I only set the cover filter to dirs/files I care about.

I see.
But if we are unlucky and our functions of interest are at the very low and high addresses, start/end will cover almost all kernel code...

> I checked 
> https://groups.google.com/g/kasan-dev/c/oVz3ZSWaK1Q/m/9ASztdzCAAAJ,
> The bitmap seems not the same as syzkaller one, which one will be used finally?

I don't know yet. We need to decide.
In syzkaller we are more flexible and can change code faster, while kernel interfaces are stable and need to be kept forever. So I think we need to concentrate more on the good kernel interface and then support it in syzkaller.

> ``` Alexander's one
> + pos = (ip - canonicalize_ip((unsigned long)&_stext)) / 4; idx = pos 
> + % BITS_PER_LONG; pos /= BITS_PER_LONG; if (likely(pos < 
> + t->kcov_size)) WRITE_ONCE(area[pos], READ_ONCE(area[pos]) | 1L << 
> + idx);
> ```
> Pc offset is divided by 4 and start is _stext. But for some arch, pc is less than _stext.

You mean that modules can have PC < _stext?

> ``` https://github.com/google/syzkaller/blob/master/syz-manager/covfilter.go#L139-L154
>         data := make([]byte, 8+((size>>4)/8+1))
>         order := binary.ByteOrder(binary.BigEndian)
>         if target.LittleEndian {
>                 order = binary.LittleEndian
>         }
>         order.PutUint32(data, start)
>         order.PutUint32(data[4:], size)
>
>         bitmap := data[8:]
>         for pc := range pcs {
>                 // The lowest 4-bit is dropped.
>                 pc = uint32(backend.NextInstructionPC(target, uint64(pc)))
>                 pc = (pc - start) >> 4
>                 bitmap[pc/8] |= (1 << (pc % 8))
>         }
>         return data
> ```
> Pc offset is divided by 16 and start is cover filter start pc.
>
> I think divided by 8 is more reasonable? Because there is at least one instruction before each __sanitizer_cov_trace_pc call.
> 0000000000000160 R_AARCH64_CALL26  __sanitizer_cov_trace_pc
> 0000000000000168 R_AARCH64_CALL26  __sanitizer_cov_trace_pc
>
> I think we still need my patch because we still need a way to keep the trace_pc call and post-filter in syzkaller doesn't solve trace_pc dropping, right?

Yes, the in-kernel filter solves the problem of trace capacity/overflows.


> But for sure I can use the bitmap from syzkaller.
>
> THX
> Joey
> -----Original Message-----
> From: Dmitry Vyukov <dvyukov@...gle.com>
> Sent: Thursday, November 18, 2021 10:00 PM
> To: JianGen Jiao (QUIC) <quic_jiangenj@...cinc.com>
> Cc: andreyknvl@...il.com; kasan-dev@...glegroups.com; LKML 
> <linux-kernel@...r.kernel.org>; Alexander Lochmann 
> <info@...xander-lochmann.de>
> Subject: Re: [PATCH] kcov: add KCOV_PC_RANGE to limit pc range
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
>
> ,On Wed, 17 Nov 2021 at 07:24, Joey Jiao <quic_jiangenj@...cinc.com> wrote:
> >
> > Sometimes we only interested in the pcs within some range, while 
> > there are cases these pcs are dropped by kernel due to `pos >=
> > t->kcov_size`, and by increasing the map area size doesn't help.
> >
> > To avoid disabling KCOV for these not intereseted pcs during build 
> > time, adding this new KCOV_PC_RANGE cmd.
>
> Hi Joey,
>
> How do you use this? I am concerned that a single range of PCs is too restrictive. I can only see how this can work for single module (continuous in memory) or a single function. But for anything else (something in the main kernel, or several modules), it won't work as PCs are not continuous.
>
> Maybe we should use a compressed bitmap of interesting PCs? It allows to support all cases and we already have it in syz-executor, then syz-executor could simply pass the bitmap to the kernel rather than post-filter.
> It's also overlaps with the KCOV_MODE_UNIQUE mode that +Alexander proposed here:
> https://groups.google.com/g/kasan-dev/c/oVz3ZSWaK1Q/m/9ASztdzCAAAJ
> It would be reasonable if kernel uses the same bitmap format for these
> 2 features.
>
>
>
> > An example usage is to use together syzkaller's cov filter.
> >
> > Change-Id: I954f6efe1bca604f5ce31f8f2b6f689e34a2981d
> > Signed-off-by: Joey Jiao <quic_jiangenj@...cinc.com>
> > ---
> >  Documentation/dev-tools/kcov.rst | 10 ++++++++++
> >  include/uapi/linux/kcov.h        |  7 +++++++
> >  kernel/kcov.c                    | 18 ++++++++++++++++++
> >  3 files changed, 35 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/kcov.rst
> > b/Documentation/dev-tools/kcov.rst
> > index d83c9ab..fbcd422 100644
> > --- a/Documentation/dev-tools/kcov.rst
> > +++ b/Documentation/dev-tools/kcov.rst
> > @@ -52,9 +52,15 @@ program using kcov:
> >      #include <fcntl.h>
> >      #include <linux/types.h>
> >
> > +    struct kcov_pc_range {
> > +      uint32 start;
> > +      uint32 end;
> > +    };
> > +
> >      #define KCOV_INIT_TRACE                    _IOR('c', 1, unsigned long)
> >      #define KCOV_ENABLE                        _IO('c', 100)
> >      #define KCOV_DISABLE                       _IO('c', 101)
> > +    #define KCOV_TRACE_RANGE                   _IOW('c', 103, struct kcov_pc_range)
> >      #define COVER_SIZE                 (64<<10)
> >
> >      #define KCOV_TRACE_PC  0
> > @@ -64,6 +70,8 @@ program using kcov:
> >      {
> >         int fd;
> >         unsigned long *cover, n, i;
> > +        /* Change start and/or end to your interested pc range. */
> > +        struct kcov_pc_range pc_range = {.start = 0, .end = 
> > + (uint32)(~((uint32)0))};
> >
> >         /* A single fd descriptor allows coverage collection on a single
> >          * thread.
> > @@ -79,6 +87,8 @@ program using kcov:
> >                                      PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> >         if ((void*)cover == MAP_FAILED)
> >                 perror("mmap"), exit(1);
> > +        if (ioctl(fd, KCOV_PC_RANGE, pc_range))
> > +               dprintf(2, "ignore KCOV_PC_RANGE error.\n");
> >         /* Enable coverage collection on the current thread. */
> >         if (ioctl(fd, KCOV_ENABLE, KCOV_TRACE_PC))
> >                 perror("ioctl"), exit(1); diff --git 
> > a/include/uapi/linux/kcov.h b/include/uapi/linux/kcov.h index 
> > 1d0350e..353ff0a 100644
> > --- a/include/uapi/linux/kcov.h
> > +++ b/include/uapi/linux/kcov.h
> > @@ -16,12 +16,19 @@ struct kcov_remote_arg {
> >         __aligned_u64   handles[0];
> >  };
> >
> > +#define PC_RANGE_MASK ((__u32)(~((u32) 0))) struct kcov_pc_range {
> > +       __u32           start;          /* start pc & 0xFFFFFFFF */
> > +       __u32           end;            /* end pc & 0xFFFFFFFF */
> > +};
> > +
> >  #define KCOV_REMOTE_MAX_HANDLES                0x100
> >
> >  #define KCOV_INIT_TRACE                        _IOR('c', 1, unsigned long)
> >  #define KCOV_ENABLE                    _IO('c', 100)
> >  #define KCOV_DISABLE                   _IO('c', 101)
> >  #define KCOV_REMOTE_ENABLE             _IOW('c', 102, struct kcov_remote_arg)
> > +#define KCOV_PC_RANGE                  _IOW('c', 103, struct kcov_pc_range)
> >
> >  enum {
> >         /*
> > diff --git a/kernel/kcov.c b/kernel/kcov.c index 36ca640..59550450
> > 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -36,6 +36,7 @@
> >   *  - 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, optional to set trace pc range
> >   *  - then, ioctl(KCOV_ENABLE, arg), where arg is
> >   *     KCOV_TRACE_PC - to trace only the PCs
> >   *     or
> > @@ -69,6 +70,8 @@ struct kcov {
> >          * kcov_remote_stop(), see the comment there.
> >          */
> >         int                     sequence;
> > +       /* u32 Trace PC range from start to end. */
> > +       struct kcov_pc_range    pc_range;
> >  };
> >
> >  struct kcov_remote_area {
> > @@ -192,6 +195,7 @@ static notrace unsigned long 
> > canonicalize_ip(unsigned long ip)  void notrace
> > __sanitizer_cov_trace_pc(void)  {
> >         struct task_struct *t;
> > +       struct kcov_pc_range pc_range;
> >         unsigned long *area;
> >         unsigned long ip = canonicalize_ip(_RET_IP_);
> >         unsigned long pos;
> > @@ -199,6 +203,11 @@ void notrace __sanitizer_cov_trace_pc(void)
> >         t = current;
> >         if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> >                 return;
> > +       pc_range = t->kcov->pc_range;
> > +       if (pc_range.start < pc_range.end &&
> > +               ((ip & PC_RANGE_MASK) < pc_range.start ||
> > +               (ip & PC_RANGE_MASK) > pc_range.end))
> > +               return;
> >
> >         area = t->kcov_area;
> >         /* The first 64-bit word is the number of subsequent PCs. */ 
> > @@ -568,6 +577,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
> >         int mode, i;
> >         struct kcov_remote_arg *remote_arg;
> >         struct kcov_remote *remote;
> > +       struct kcov_pc_range *pc_range;
> >         unsigned long flags;
> >
> >         switch (cmd) {
> > @@ -589,6 +599,14 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
> >                 kcov->size = size;
> >                 kcov->mode = KCOV_MODE_INIT;
> >                 return 0;
> > +       case KCOV_PC_RANGE:
> > +               /* Limit trace pc range. */
> > +               pc_range = (struct kcov_pc_range *)arg;
> > +               if (copy_from_user(&kcov->pc_range, pc_range, sizeof(kcov->pc_range)))
> > +                       return -EINVAL;
> > +               if (kcov->pc_range.start >= kcov->pc_range.end)
> > +                       return -EINVAL;
> > +               return 0;
> >         case KCOV_ENABLE:
> >                 /*
> >                  * Enable coverage for the current task.
> > --
> > 2.7.4
> >

Powered by blists - more mailing lists