RE: [PATCH] kcov: add KCOV_PC_RANGE to limit pc range

From: JianGen Jiao (QUIC)
Date: Thu Nov 18 2021 - 22:17:33 EST

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 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?

``` 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.


``` 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?
But for sure I can use the bitmap from syzkaller.

THX
Joey
-----Original Message-----
From: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Sent: Thursday, November 18, 2021 10:00 PM
To: JianGen Jiao (QUIC) <quic_jiangenj@xxxxxxxxxxx>
Cc: andreyknvl@xxxxxxxxx; kasan-dev@xxxxxxxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>; Alexander Lochmann <info@xxxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
> 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
>