Re: [PATCH RFC 2/2] x86/kcov: allow instrumenting noisy objects for early boot coverage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/kcov: instrumenting apic/ crashes selftest() on FRED timer entry
--

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

x86/kcov: allow instrumenting noisy objects for early boot coverage

This commit switches several x86 Makefile exclusions from hardcoded n to
$(kcov-noisy) so that CONFIG_KCOV_INSTRUMENT_NOISY=y can collect early boot
coverage from arch/x86/kernel/, arch/x86/kernel/apic/, and arch/x86/lib/.

> diff --git a/arch/x86/kernel/apic/Makefile b/arch/x86/kernel/apic/Makefile
> index 581db89477f92..146cbd766c924 100644
> --- a/arch/x86/kernel/apic/Makefile
> +++ b/arch/x86/kernel/apic/Makefile
> @@ -5,7 +5,7 @@
>  
>  # Leads to non-deterministic coverage that is not a function of syscall inputs.
>  # In particular, smp_apic_timer_interrupt() is called in random places.
> -KCOV_INSTRUMENT		:= n
> +KCOV_INSTRUMENT		:= $(kcov-noisy)

[Severity: High]
Does instrumenting arch/x86/kernel/apic/apic.o cause a NULL pointer
dereference during boot when CONFIG_KCOV_INSTRUMENT_NOISY=y and
CONFIG_KCOV_SELFTEST=y on FRED-enabled x86_64 systems?

In arch/x86/kernel/apic/apic.c, DEFINE_IDTENTRY_SYSVEC(sysvec_apic_timer_interrupt)
expands via arch/x86/include/asm/idtentry.h:

    void fred_##func(struct pt_regs *regs)
    {
        instr_##func (regs);
    }

Unlike the IDT entry wrapper sysvec_apic_timer_interrupt(),
fred_sysvec_apic_timer_interrupt() is not marked noinstr or
__no_sanitize_coverage. When apic.o is instrumented with KCOV,
fred_sysvec_apic_timer_interrupt() and the inlined
instr_sysvec_apic_timer_interrupt() -> run_sysvec_on_irqstack_cond() ->
call_on_irqstack_cond() call into __sanitizer_cov_trace_pc() before
irq_enter_rcu() adds HARDIRQ_OFFSET to preempt_count (and again after
irq_exit_rcu() removes HARDIRQ_OFFSET).

During boot, after rest_init() -> early_kcov_stop() clears
init_task.kcov_area to NULL, device_initcall(kcov_init) invokes selftest()
in kernel/kcov.c:

    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);

While selftest() spins waiting for timer interrupts to advance jiffies with
current->kcov_area == NULL, the first LAPIC timer interrupt under FRED
enters fred_sysvec_apic_timer_interrupt() while in_task() in
check_kcov_mode() is still true, reaching __sanitizer_cov_trace_pc() in
kernel/kcov.c:

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

and dereferencing NULL.

In addition, during normal task KCOV tracing on FRED systems, if a LAPIC
timer interrupt preempts a traced task inside __sanitizer_cov_trace_pc() or
write_comp_data() between reading area[0] and writing back the updated
count, can the nested KCOV calls in fred_sysvec_apic_timer_interrupt() have
their area[0] updates overwritten when the interrupted context resumes and
stores its stale index?

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