Re: [PATCH RFC 1/2] kcov: collect early boot coverage of init_task

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kcov: NULL deref in __sanitizer_cov_trace_pc() during selftest()
--

commit 2f2cd36073f62980c1305efe82a4239b11c076f5
Author: Yunseong Kim <yunseong.kim@est.tech>

kcov: collect early boot coverage of init_task

This commit adds CONFIG_EARLY_KCOV to record code coverage of init_task
from right after early parameter parsing in start_kernel() until the start
of rest_init(), and exposes the collected PCs via
/sys/kernel/debug/early_kcov. It also introduces CONFIG_KCOV_INSTRUMENT_NOISY
and a kcov-noisy kbuild helper to instrument core subsystems normally
excluded due to coverage noise.

> diff --git a/kernel/Makefile b/kernel/Makefile
> index 4ff92e24680c5..b9e4f06e02ad0 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -27,14 +27,14 @@ endif
>  
>  # Prevents flicker of uninteresting __do_softirq()/__local_bh_disable_ip()
>  # in coverage traces.
> -KCOV_INSTRUMENT_softirq.o := n
> +KCOV_INSTRUMENT_softirq.o := $(kcov-noisy)

[ ... ]

> diff --git a/kernel/locking/Makefile b/kernel/locking/Makefile
> index cee1901d4cff8..9041cbbec9aae 100644
> --- a/kernel/locking/Makefile
> +++ b/kernel/locking/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Any varying coverage in these files is non-deterministic
>  # and is generally not a function of system call inputs.
> -KCOV_INSTRUMENT		:= n
> +KCOV_INSTRUMENT		:= $(kcov-noisy)

[ ... ]

> diff --git a/kernel/rcu/Makefile b/kernel/rcu/Makefile
> index 0cfb009a99b9f..fd23c1dad641b 100644
> --- a/kernel/rcu/Makefile
> +++ b/kernel/rcu/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Any varying coverage in these files is non-deterministic
>  # and is generally not a function of system call inputs.
> -KCOV_INSTRUMENT := n
> +KCOV_INSTRUMENT := $(kcov-noisy)

[ ... ]

> diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
> index b1f1a367034ff..4b4db5cca9b08 100644
> --- a/kernel/sched/Makefile
> +++ b/kernel/sched/Makefile
> @@ -9,7 +9,7 @@ ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
>  
>  # These files are disabled because they produce non-interesting flaky coverage
>  # that is not a function of syscall inputs. E.g. involuntary context switches.
> -KCOV_INSTRUMENT := n
> +KCOV_INSTRUMENT := $(kcov-noisy)

[ ... ]

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 6759087154edd..40bf654ec7515 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2233,6 +2233,51 @@ 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_INSTRUMENT_NOISY
> +	bool "Also instrument code excluded only for noisy coverage"
> +	depends on EARLY_KCOV && KCOV_INSTRUMENT_ALL
> +	help
> +	  Some code (scheduler, RCU, locking, page and slab allocators,
> +	  rbtree, stack unwinding, ...) is excluded from KCOV instrumentation
> +	  because its coverage is not a function of system call inputs and
> +	  only adds noise to coverage-guided fuzzing. Code that is excluded
> +	  because it cannot be instrumented safely is not affected.
> +
> +	  Say y to instrument the noisy code as well, so that the early boot
> +	  coverage shows the complete execution path. This degrades the
> +	  coverage signal for syscall fuzzing, so say n for fuzzing kernels.

[Severity: High]
Can enabling CONFIG_KCOV_INSTRUMENT_NOISY trigger a NULL pointer dereference
in __sanitizer_cov_trace_pc() during kcov_init() -> selftest(), as well as
record interrupt entry and exit PCs into task coverage buffers?

In kernel/kcov.c, check_kcov_mode() filters out interrupt context using
in_task(), which relies on HARDIRQ_OFFSET being set in preempt_count().
However, several functions in kernel/softirq.c, kernel/sched/core.c,
kernel/locking/lockdep.c, and kernel/rcu/ run on every hardware interrupt
either before HARDIRQ_OFFSET is added or after it is subtracted, while
in_task() is still true:

- In kernel/softirq.c, irq_enter_rcu() and irq_enter() are not marked
  noinstr or __no_sanitize_coverage, so the compiler inserts a call to
  __sanitizer_cov_trace_pc() at function entry before __irq_enter_raw()
  adds HARDIRQ_OFFSET to preempt_count(). Likewise, __irq_exit_rcu()
  (inlined into irq_exit_rcu() and irq_exit()) calls
  preempt_count_sub(HARDIRQ_OFFSET) before finishing execution.

- When CONFIG_DEBUG_PREEMPT=y or CONFIG_TRACE_PREEMPT_TOGGLE=y,
  __irq_enter_raw() calls preempt_count_add(HARDIRQ_OFFSET) in
  kernel/sched/core.c, which is instrumented at entry before
  __preempt_count_add() updates preempt_count().

- On interrupt exit after HARDIRQ_OFFSET is cleared,
  irqentry_exit_to_kernel_mode() / irqentry_exit_cond_resched() can call
  trace_hardirqs_on() -> lockdep_hardirqs_on_prepare() in
  kernel/locking/lockdep.c, rcu_irq_exit_check_preempt() in kernel/rcu/,
  and preempt_schedule_irq() -> __schedule() in kernel/sched/core.c
  (before prepare_task_switch() sets KCOV_IN_CTXSW and after
  kcov_finish_switch() clears it).

When CONFIG_KCOV_SELFTEST=y is also enabled (for example in allyesconfig
builds), kcov_init() calls selftest() at device_initcall time with
current->kcov_area == NULL and current->kcov_size == 0:

kernel/kcov.c:selftest() {
	...
	start = jiffies;
	WRITE_ONCE(current->kcov_mode, KCOV_MODE_TRACE_PC);
	while ((jiffies - start) * MSEC_PER_SEC / HZ < 300)
		;
	WRITE_ONCE(current->kcov_mode, 0);
}

When a timer interrupt fires during that loop, the instrumented interrupt
entry/exit path invokes __sanitizer_cov_trace_pc() while in_task() is true:

kernel/kcov.c:__sanitizer_cov_trace_pc() {
	...
	t = current;
	if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
		return;

	area = t->kcov_area;
	/* The first 64-bit word is the number of subsequent PCs. */
	pos = READ_ONCE(area[0]) + 1;
	if (likely(pos < t->kcov_size)) {
	...
}

Because area[0] is read before checking t->kcov_size, dereferencing area
when t->kcov_area is NULL crashes the kernel on boot.

Should these interrupt entry/exit helpers be marked __no_sanitize_coverage
(or noinstr), or should CONFIG_KCOV_SELFTEST depend on
!KCOV_INSTRUMENT_NOISY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-early-kcov-v1-0-6f2f053255ad@est.tech?part=1