| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 19 Jun 2024 15:05:54 +0200
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, linux-kernel@...r.kernel.org,
syzkaller@...glegroups.com, elver@...gle.com, glider@...gle.com,
nogikh@...gle.com, tarasmadan@...gle.com
Subject: Re: [PATCH v2 1/4] x86/entry: Remove unwanted instrumentation in common_interrupt()
On Wed, 19 Jun 2024 at 13:19, Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Tue, Jun 11, 2024 at 09:50:30AM +0200, Dmitry Vyukov wrote:
> > common_interrupt() and friends call kvm_set_cpu_l1tf_flush_l1d(),
> > which is not marked as noinstr nor __always_inline.
> > So compiler outlines it and adds instrumentation to it.
> > Since the call is inside of instrumentation_begin/end(),
> > objtool does not warn about it.
> >
> > The manifestation is that KCOV produces spurious coverage
> > in kvm_set_cpu_l1tf_flush_l1d() in random places because
> > the call happens when preempt count is not yet updated
> > to say that we are in an interrupt.
>
> So I'm reading spurious here, but the next patch trips BUG, them very
> much not the same thing. Which is it?
I am not sure I understand the question.
Currently kvm_set_cpu_l1tf_flush_l1d() is traced, so the added test
produces a BUG without this fix (and does not produce with this fix).
By "spurious" I meant that tracing of kvm_set_cpu_l1tf_flush_l1d() and
interrupts in general is parasitic/unwanted.
> > Mark kvm_set_cpu_l1tf_flush_l1d() as __always_inline and move
> > out of instrumentation_begin/end() section.
> > It only calls __this_cpu_write() which is already safe to call
> > in noinstr contexts.
> >
> > Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
> > Reviewed-by: Alexander Potapenko <glider@...gle.com>
> > Fixes: 6368558c3710 ("x86/entry: Provide IDTENTRY_SYSVEC")
> > Cc: x86@...nel.org
> > Cc: linux-kernel@...r.kernel.org
> > Cc: syzkaller@...glegroups.com
>
> Anyway, the patch is fine,
>
> Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>
>
> > ---
> > arch/x86/include/asm/hardirq.h | 8 ++++++--
> > arch/x86/include/asm/idtentry.h | 6 +++---
> > 2 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
> > index c67fa6ad098a..6ffa8b75f4cd 100644
> > --- a/arch/x86/include/asm/hardirq.h
> > +++ b/arch/x86/include/asm/hardirq.h
> > @@ -69,7 +69,11 @@ extern u64 arch_irq_stat(void);
> > #define local_softirq_pending_ref pcpu_hot.softirq_pending
> >
> > #if IS_ENABLED(CONFIG_KVM_INTEL)
> > -static inline void kvm_set_cpu_l1tf_flush_l1d(void)
> > +/*
> > + * This function is called from noinstr interrupt contexts
> > + * and must be inlined to not get instrumentation.
> > + */
> > +static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void)
> > {
> > __this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
> > }
> > @@ -84,7 +88,7 @@ static __always_inline bool kvm_get_cpu_l1tf_flush_l1d(void)
> > return __this_cpu_read(irq_stat.kvm_cpu_l1tf_flush_l1d);
> > }
> > #else /* !IS_ENABLED(CONFIG_KVM_INTEL) */
> > -static inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
> > +static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
> > #endif /* IS_ENABLED(CONFIG_KVM_INTEL) */
> >
> > #endif /* _ASM_X86_HARDIRQ_H */
> > diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
> > index d4f24499b256..ad5c68f0509d 100644
> > --- a/arch/x86/include/asm/idtentry.h
> > +++ b/arch/x86/include/asm/idtentry.h
> > @@ -212,8 +212,8 @@ __visible noinstr void func(struct pt_regs *regs, \
> > irqentry_state_t state = irqentry_enter(regs); \
> > u32 vector = (u32)(u8)error_code; \
> > \
> > + kvm_set_cpu_l1tf_flush_l1d(); \
> > instrumentation_begin(); \
> > - kvm_set_cpu_l1tf_flush_l1d(); \
> > run_irq_on_irqstack_cond(__##func, regs, vector); \
> > instrumentation_end(); \
> > irqentry_exit(regs, state); \
> > @@ -250,7 +250,6 @@ static void __##func(struct pt_regs *regs); \
> > \
> > static __always_inline void instr_##func(struct pt_regs *regs) \
> > { \
> > - kvm_set_cpu_l1tf_flush_l1d(); \
> > run_sysvec_on_irqstack_cond(__##func, regs); \
> > } \
> > \
> > @@ -258,6 +257,7 @@ __visible noinstr void func(struct pt_regs *regs) \
> > { \
> > irqentry_state_t state = irqentry_enter(regs); \
> > \
> > + kvm_set_cpu_l1tf_flush_l1d(); \
> > instrumentation_begin(); \
> > instr_##func (regs); \
> > instrumentation_end(); \
> > @@ -288,7 +288,6 @@ static __always_inline void __##func(struct pt_regs *regs); \
> > static __always_inline void instr_##func(struct pt_regs *regs) \
> > { \
> > __irq_enter_raw(); \
> > - kvm_set_cpu_l1tf_flush_l1d(); \
> > __##func (regs); \
> > __irq_exit_raw(); \
> > } \
> > @@ -297,6 +296,7 @@ __visible noinstr void func(struct pt_regs *regs) \
> > { \
> > irqentry_state_t state = irqentry_enter(regs); \
> > \
> > + kvm_set_cpu_l1tf_flush_l1d(); \
> > instrumentation_begin(); \
> > instr_##func (regs); \
> > instrumentation_end(); \
> > --
> > 2.45.2.505.gda0bf45e8d-goog
> >
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20240619111936.GK31592%40noisy.programming.kicks-ass.net.
Powered by blists - more mailing lists