Skip to contentSkip to conversation

kcov: collect coverage from usb soft interrupts

PATCH Superseded Andrey Konovalov 2 participants 6 messages Feb 26, 2020 – Feb 27, 2020

Subsystems (6)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover kcov: collect coverage from usb soft interrupts

This patchset extends kcov to allow collecting coverage from soft interrupts and then uses the new functionality to collect coverage from USB code.

This has allowed to find at least one new HID bug [1], which was recently fixed by Alan [2].

[1] https://syzkaller.appspot.com/bug?extid=09ef48aa58261464b621
[2] https://patchwork.kernel.org/patch/11283319/

This patchset has been pushed to the public Linux kernel Gerrit instance:

https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/2225

Changes RFC -> v1:
- Don't support hardirq or nmi, only softirq, to avoid issues with nested interrupts.
- Combined multiple per-cpu variables into one.
- Used plain accesses and kcov_start/stop() instead of xchg()'s.
- Simplified handling of per-cpu variables.
- Avoid disabling interrupts for the whole kcov_remote_start/stop() region.
- Avoid overwriting t->kcov_sequence when saving/restoring state.
- Move kcov_remote_start/stop_usb() annotations into __usb_hcd_giveback_urb() to cover all urb complete() callbacks at once.
- Drop unneeded Dummy HCD changes.
- Split out a patch that removed debug messages.

Andrey Konovalov (3):
kcov: cleanup debug messages
kcov: collect coverage from interrupts
usb: core: kcov: collect coverage from usb complete callback

 Documentation/dev-tools/kcov.rst |  17 +--
 drivers/usb/core/hcd.c           |   3 +
 include/linux/sched.h            |   3 +
 kernel/kcov.c                    | 187 ++++++++++++++++++++-----------
 lib/Kconfig.debug                |   9 ++
 5 files changed, 147 insertions(+), 72 deletions(-)
Signature · 1 line
2.25.1.481.gfbce0eb801-goog
#1 lore raw

PATCH 1/3 kcov: cleanup debug messages

Previous commit left a lot of excessive debug messages, clean them up.

---
 kernel/kcov.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)
kernel/kcov.c +2 -20
index f50354202dbe..f6bd119c9419 100644
@@ -98,6 +98,7 @@ static struct kcov_remote *kcov_remote_find(u64 handle)
 	return NULL;
 }
 
+/* Must be called with kcov_remote_lock locked. */
 static struct kcov_remote *kcov_remote_add(struct kcov *kcov, u64 handle)
 {
 	struct kcov_remote *remote;
@@ -119,16 +120,13 @@ static struct kcov_remote_area *kcov_remote_area_get(unsigned int size)
 	struct kcov_remote_area *area;
 	struct list_head *pos;
 
-	kcov_debug("size = %u\n", size);
 	list_for_each(pos, &kcov_remote_areas) {
 		area = list_entry(pos, struct kcov_remote_area, list);
 		if (area->size == size) {
 			list_del(&area->list);
-			kcov_debug("rv = %px\n", area);
 			return area;
 		}
 	}
-	kcov_debug("rv = NULL\n");
 	return NULL;
 }
 
@@ -136,7 +134,6 @@ static struct kcov_remote_area *kcov_remote_area_get(unsigned int size)
 static void kcov_remote_area_put(struct kcov_remote_area *area,
 					unsigned int size)
 {
-	kcov_debug("area = %px, size = %u\n", area, size);
 	INIT_LIST_HEAD(&area->list);
 	area->size = size;
 	list_add(&area->list, &kcov_remote_areas);
@@ -366,7 +363,6 @@ static void kcov_remote_reset(struct kcov *kcov)
 	hash_for_each_safe(kcov_remote_map, bkt, tmp, remote, hnode) {
 		if (remote->kcov != kcov)
 			continue;
-		kcov_debug("removing handle %llx\n", remote->handle);
 		hash_del(&remote->hnode);
 		kfree(remote);
 	}
@@ -553,7 +549,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 
 	switch (cmd) {
 	case KCOV_INIT_TRACE:
-		kcov_debug("KCOV_INIT_TRACE\n");
 		/*
 		 * Enable kcov in trace mode and setup buffer size.
 		 * Must happen before anything else.
@@ -572,7 +567,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		kcov->mode = KCOV_MODE_INIT;
 		return 0;
 	case KCOV_ENABLE:
-		kcov_debug("KCOV_ENABLE\n");
 		/*
 		 * Enable coverage for the current task.
 		 * At this point user must have been enabled trace mode,
@@ -598,7 +592,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		kcov_get(kcov);
 		return 0;
 	case KCOV_DISABLE:
-		kcov_debug("KCOV_DISABLE\n");
 		/* Disable coverage for the current task. */
 		unused = arg;
 		if (unused != 0 || current->kcov != kcov)
@@ -610,7 +603,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		kcov_put(kcov);
 		return 0;
 	case KCOV_REMOTE_ENABLE:
-		kcov_debug("KCOV_REMOTE_ENABLE\n");
 		if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
 			return -EINVAL;
 		t = current;
@@ -629,7 +621,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		kcov->remote_size = remote_arg->area_size;
 		spin_lock(&kcov_remote_lock);
 		for (i = 0; i < remote_arg->num_handles; i++) {
-			kcov_debug("handle %llx\n", remote_arg->handles[i]);
 			if (!kcov_check_handle(remote_arg->handles[i],
 						false, true, false)) {
 				spin_unlock(&kcov_remote_lock);
@@ -644,8 +635,6 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 			}
 		}
 		if (remote_arg->common_handle) {
-			kcov_debug("common handle %llx\n",
-					remote_arg->common_handle);
 			if (!kcov_check_handle(remote_arg->common_handle,
 						true, false, false)) {
 				spin_unlock(&kcov_remote_lock);
@@ -782,7 +771,6 @@ void kcov_remote_start(u64 handle)
 	spin_lock(&kcov_remote_lock);
 	remote = kcov_remote_find(handle);
 	if (!remote) {
-		kcov_debug("no remote found");
 		spin_unlock(&kcov_remote_lock);
 		return;
 	}
@@ -810,8 +798,6 @@ void kcov_remote_start(u64 handle)
 	/* Reset coverage size. */
 	*(u64 *)area = 0;
 
-	kcov_debug("area = %px, size = %u", area, size);
-
 	kcov_start(t, size, area, mode, sequence);
 
 }
@@ -881,10 +867,8 @@ void kcov_remote_stop(void)
 	unsigned int size = t->kcov_size;
 	int sequence = t->kcov_sequence;
 
-	if (!kcov) {
-		kcov_debug("no kcov found\n");
+	if (!kcov)
 		return;
-	}
 
 	kcov_stop(t);
 	t->kcov = NULL;
@@ -894,8 +878,6 @@ void kcov_remote_stop(void)
 	 * KCOV_DISABLE could have been called between kcov_remote_start()
 	 * and kcov_remote_stop(), hence the check.
 	 */
-	kcov_debug("move if: %d == %d && %d\n",
-		sequence, kcov->sequence, (int)kcov->remote);
 	if (sequence == kcov->sequence && kcov->remote)
 		kcov_move_area(kcov->mode, kcov->area, kcov->size, area);
 	spin_unlock(&kcov->lock);
Signature · 1 line
2.25.1.481.gfbce0eb801-goog
#2 lore raw

PATCH 2/3 kcov: collect coverage from interrupts

In reply to Andrey Konovalov

This change extends kcov remote coverage support to allow collecting coverage from soft interrupts in addition to kernel background threads.

To collect coverage from code that is executed in softirq context, a part of that code has to be annotated with kcov_remote_start/stop() in a similar way as how it is done for global kernel background threads. Then the handle used for the annotations has to be passed to the KCOV_REMOTE_ENABLE ioctl.

Internally this patch adjusts the __sanitizer_cov_trace_pc() compiler inserted callback to not bail out when called from softirq context. kcov_remote_start/stop() are updated to save/restore the current per task kcov state in a per-cpu area (in case the softirq came when the kernel was already collecting coverage in task context). Coverage from softirqs is collected into pre-allocated per-cpu areas, whose size is controlled by the new CONFIG_KCOV_IRQ_AREA_SIZE.

---
 Documentation/dev-tools/kcov.rst |  17 ++--
 include/linux/sched.h            |   3 +
 kernel/kcov.c                    | 165 ++++++++++++++++++++++---------
 lib/Kconfig.debug                |   9 ++
 4 files changed, 142 insertions(+), 52 deletions(-)
Documentation/dev-tools/kcov.rst +9 -8
index 1c4e1825d769..8548b0b04e43 100644
@@ -217,14 +217,15 @@ This allows to collect coverage from two types of kernel background
 threads: the global ones, that are spawned during kernel boot in a limited
 number of instances (e.g. one USB hub_event() worker thread is spawned per
 USB HCD); and the local ones, that are spawned when a user interacts with
-some kernel interface (e.g. vhost workers).
+some kernel interface (e.g. vhost workers); as well as from soft
+interrupts.
 
-To enable collecting coverage from a global background thread, a unique
-global handle must be assigned and passed to the corresponding
-kcov_remote_start() call. Then a userspace process can pass a list of such
-handles to the KCOV_REMOTE_ENABLE ioctl in the handles array field of the
-kcov_remote_arg struct. This will attach the used kcov device to the code
-sections, that are referenced by those handles.
+To enable collecting coverage from a global background thread or from a
+softirq, a unique global handle must be assigned and passed to the
+corresponding kcov_remote_start() call. Then a userspace process can pass
+a list of such handles to the KCOV_REMOTE_ENABLE ioctl in the handles
+array field of the kcov_remote_arg struct. This will attach the used kcov
+device to the code sections, that are referenced by those handles.
 
 Since there might be many local background threads spawned from different
 userspace processes, we can't use a single global handle per annotation.
@@ -242,7 +243,7 @@ handles as they don't belong to a particular subsystem. The bytes 4-7 are
 currently reserved and must be zero. In the future the number of bytes
 used for the subsystem or handle ids might be increased.
 
-When a particular userspace proccess collects coverage by via a common
+When a particular userspace proccess collects coverage via a common
 handle, kcov will collect coverage for each code section that is annotated
 to use the common handle obtained as kcov_handle from the current
 task_struct. However non common handles allow to collect coverage
include/linux/sched.h +3 -0
index 04278493bf15..ce47d685fa55 100644
@@ -1229,6 +1229,9 @@ struct task_struct {
 
 	/* KCOV sequence number: */
 	int				kcov_sequence;
+
+	/* Collect coverage from softirq context: */
+	bool				kcov_softirq;
 #endif
 
 #ifdef CONFIG_MEMCG
kernel/kcov.c +121 -44

379 lines not on this page · raw message

lib/Kconfig.debug +9 -0
index 69def4a9df00..a1f25b27d32d 100644
@@ -1740,6 +1740,15 @@ config KCOV_INSTRUMENT_ALL
 	  filesystem fuzzing with AFL) then you will want to enable coverage
 	  for more specific subsets of files, and should say n here.
 
+config KCOV_IRQ_AREA_SIZE
+	hex "Size of interrupt coverage collection area in words"
+	depends on KCOV
+	default 0x40000
+	help
+	  KCOV uses preallocated per-cpu areas to collect coverage from
+	  soft interrupts. This specifies the size of those areas in the
+	  number of unsigned long words.
+
 menuconfig RUNTIME_TESTING_MENU
 	bool "Runtime Testing"
 	def_bool y
Signature · 1 line
2.25.1.481.gfbce0eb801-goog
#3 lore raw

PATCH 3/3 usb: core: kcov: collect coverage from usb complete callback

In reply to Andrey Konovalov

This patch adds kcov_remote_start/stop() callbacks around the urb complete() callback that is executed in softirq context when dummy_hcd is in use. As the result, kcov can be used to collect coverage from those those callbacks, which is used to facilitate coverage-guided fuzzing with syzkaller.

---
 drivers/usb/core/hcd.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/core/hcd.c +3 -0
index aa45840d8273..de624c47e190 100644
@@ -31,6 +31,7 @@
 #include <linux/types.h>
 #include <linux/genalloc.h>
 #include <linux/io.h>
+#include <linux/kcov.h>
 
 #include <linux/phy/phy.h>
 #include <linux/usb.h>
@@ -1645,7 +1646,9 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
 
 	/* pass ownership to the completion handler */
 	urb->status = status;
+	kcov_remote_start_usb((u64)urb->dev->bus->busnum);
 	urb->complete(urb);
+	kcov_remote_stop();
 
 	usb_anchor_resume_wakeups(anchor);
 	atomic_dec(&urb->use_count);
Signature · 1 line
2.25.1.481.gfbce0eb801-goog
maintainer
#4 lore raw
In reply to Andrey Konovalov
Quoted 6 lines · On Wed, 26 Feb 2020 19:48:06 +0100 Andrey Konovalov <[email protected]>…

This patchset extends kcov to allow collecting coverage from soft interrupts and then uses the new functionality to collect coverage from USB code.

This has allowed to find at least one new HID bug [1], which was recently fixed by Alan [2].

I might have asked this before, but I don't see this obvious question addressed in the changelogs so maybe I didn't...

Will this only ever be useful for USB? Or is it anticipated that other subsystems will use this? If the latter, which ones?

Thanks.

#5 lore raw
Quoted 7 lines · Andrew Morton
Quoted 4 lines · On Wed, 26 Feb 2020 19:48:06 +0100 Andrey Konovalov <[email protected]>…

This patchset extends kcov to allow collecting coverage from soft interrupts and then uses the new functionality to collect coverage from USB code.

Quoted 5 lines
Quoted 2 lines

This has allowed to find at least one new HID bug [1], which was recently fixed by Alan [2].

I might have asked this before, but I don't see this obvious question addressed in the changelogs so maybe I didn't...

You've asked this for the previous patchset about collecting coverage from background threads :)

Quoted 2 lines

Will this only ever be useful for USB? Or is it anticipated that other subsystems will use this? If the latter, which ones?

Any subsystem that uses softirqs (e.g. timers) can make use of this. Looking at the recent syzbot reports, an obvious candidate is the networking subsystem [1, 2, 3 and many more]. I'll add this info into the cover letter in the next version.

Thanks!

[1] https://syzkaller.appspot.com/bug?extid=522ab502c69badc66ab7
[2] https://syzkaller.appspot.com/bug?extid=57f89d05946c53dbbb31
[3] https://syzkaller.appspot.com/bug?extid=df358e65d9c1b9d3f5f4