From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 27479C2BA19 for ; Tue, 14 Apr 2020 15:04:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D2DEA20644 for ; Tue, 14 Apr 2020 15:04:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="j3Zshk4y" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D2DEA20644 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7B1AA8E0003; Tue, 14 Apr 2020 11:04:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 761DA8E0001; Tue, 14 Apr 2020 11:04:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 62AF58E0003; Tue, 14 Apr 2020 11:04:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0056.hostedemail.com [216.40.44.56]) by kanga.kvack.org (Postfix) with ESMTP id 45F2C8E0001 for ; Tue, 14 Apr 2020 11:04:39 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 093405DD3 for ; Tue, 14 Apr 2020 15:04:39 +0000 (UTC) X-FDA: 76706782278.12.can31_62bcd188f5501 X-HE-Tag: can31_62bcd188f5501 X-Filterd-Recvd-Size: 13948 Received: from mail-pj1-f66.google.com (mail-pj1-f66.google.com [209.85.216.66]) by imf24.hostedemail.com (Postfix) with ESMTP for ; Tue, 14 Apr 2020 15:04:38 +0000 (UTC) Received: by mail-pj1-f66.google.com with SMTP id z9so5312315pjd.2 for ; Tue, 14 Apr 2020 08:04:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=AbSibCkizbHP2dZC+kmJ92Qoaoj0OSOfVtcVAWlB7fA=; b=j3Zshk4ynQZgjKKaFYFVY/t3tyj8k2a0zQxnBoEj+PgsqAkljccuvKuQizByu/ugnj VhFbuEB27RkJOl5bhp32nKIyReALOf0B821YDkqjSSEvUazKj3KvU1ZC9gwpFCvDSLNx kM234ylsISIoMmsKuXHRszGHMR6BoGcol4vfeXsbzrqyIuBegiWPo9IEOa9JJ2t0cTA9 ahF/xQ0+LdEqF+P2iKAhEfjj203xFpk95ry9X1OhU7Kh9u+aJ9iNNMqv1ENpbf3HgNAL Fc1IFalfxtceSTHfy3DMVEZxVI3U5iNBH0/k3DuBPt6Cuz3M1ZmjDrKx0RFrBruW43My aECA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=AbSibCkizbHP2dZC+kmJ92Qoaoj0OSOfVtcVAWlB7fA=; b=A0qmMk5FI+PqnNyJX2V3ww9VJCG3fFAPXoy2z+/vdd9qGodi/h25FgXDCdzciu24sC t3AiDGi3/EhdPewhF1bHn4joe0jhL58aL53xyaaHCK271sUshH3tRnBPG4IXkZ8rvKfP 8OWx2RZ63Wy5F25dBQLR7WXauKuCgK5VVyXUmPW32fgzQ0it35XMthmVMjPUK63odN32 EHE4dc9DHvnLQ2xK7+Y62+0lcTdTY+8pd9okEjZq+x9k71AP+tyuN7MHZ8MFWxBQLtWY BirEsWfy7CZiVPcRHth4PLeduOpeSVHTb2yuce+guKgZoF8lzdHX5GkfxrBhXaO1QGKG vNRA== X-Gm-Message-State: AGi0PuZrdCb7kCZoPA5wV+2/ciNudazG5n8zd7HAMglx5no5Woh2o1pF zZ4jUFlhlYM+WkHBz+s1NMf4j19ayywn9BPNMfUicw== X-Google-Smtp-Source: APiQypKaeVQYTfWZHwijJb97nXaSa2jVW7ObItDXS3a1UjUcvQW/N4XRHzHfM9VhMDZe0fVPSXMaWbYlPDFP+KR+cMw= X-Received: by 2002:a17:90a:e00f:: with SMTP id u15mr592538pjy.69.1586876677173; Tue, 14 Apr 2020 08:04:37 -0700 (PDT) MIME-Version: 1.0 References: <20200325161249.55095-1-glider@google.com> <20200325161249.55095-25-glider@google.com> In-Reply-To: <20200325161249.55095-25-glider@google.com> From: Andrey Konovalov Date: Tue, 14 Apr 2020 17:04:26 +0200 Message-ID: Subject: Re: [PATCH v5 24/38] kmsan: disable instrumentation of certain functions To: Alexander Potapenko Cc: Thomas Gleixner , Andrew Morton , Vegard Nossum , Dmitry Vyukov , Marco Elver , Linux Memory Management List , Alexander Viro , Andreas Dilger , Andrey Ryabinin , Andy Lutomirski , Ard Biesheuvel , Arnd Bergmann , Christoph Hellwig , Christoph Hellwig , "Darrick J. Wong" , "David S. Miller" , Dmitry Torokhov , Eric Biggers , Eric Dumazet , Eric Van Hensbergen , Greg Kroah-Hartman , Harry Wentland , Herbert Xu , Ilya Leoshkevich , Ingo Molnar , Jason Wang , Jens Axboe , Marek Szyprowski , Mark Rutland , "Martin K. Petersen" , Martin Schwidefsky , Matthew Wilcox , "Michael S . Tsirkin" , Michal Hocko , Michal Simek , Petr Mladek , Qian Cai , Randy Dunlap , Robin Murphy , Sergey Senozhatsky , Steven Rostedt , Takashi Iwai , "Theodore Ts'o" , Vasily Gorbik , Wolfram Sang Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Mar 25, 2020 at 5:14 PM wrote: > > Some functions are called from handwritten assembly, and therefore don't > have their arguments' metadata fully set up by the instrumentation code. > Mark them with __no_sanitize_memory to avoid false positives from > spreading further. > Certain functions perform task switching, so that the value of |current| > is different as they proceed. Because KMSAN state pointer is only read > once at the beginning of the function, touching it after |current| has > changed may be dangerous. I think that whenever a patch touches some subsystem, we need to mention that in the commit title. This patch touches mostly x86, so "kmsan, x86: ..." or something like that. This will probably make x86 maintainers more involved. > > Signed-off-by: Alexander Potapenko > To: Alexander Potapenko > Cc: Thomas Gleixner > Cc: Andrew Morton > Cc: Vegard Nossum > Cc: Dmitry Vyukov > Cc: Marco Elver > Cc: Andrey Konovalov > Cc: linux-mm@kvack.org > --- > v3: > - removed TODOs from comments > > v4: > - updated the comments, dropped __no_sanitize_memory from idle_cpu(), > sched_init(), profile_tick() > - split away the uprobes part as requested by Andrey Konovalov > > Change-Id: I684d23dac5a22eb0a4cea71993cb934302b17cea > --- > arch/x86/entry/common.c | 2 ++ > arch/x86/include/asm/irq_regs.h | 2 ++ > arch/x86/include/asm/syscall_wrapper.h | 2 ++ > arch/x86/kernel/apic/apic.c | 3 +++ > arch/x86/kernel/dumpstack_64.c | 5 +++++ > arch/x86/kernel/process_64.c | 5 +++++ > arch/x86/kernel/traps.c | 13 +++++++++++-- > kernel/sched/core.c | 22 ++++++++++++++++++++++ > 8 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c > index ec167d8c41cbd..5c3d0f3a14c37 100644 > --- a/arch/x86/entry/common.c > +++ b/arch/x86/entry/common.c > @@ -280,6 +280,8 @@ __visible inline void syscall_return_slowpath(struct pt_regs *regs) > } > > #ifdef CONFIG_X86_64 > +/* Tell KMSAN to not instrument this function and to initialize |regs|. */ > +__no_sanitize_memory > __visible void do_syscall_64(unsigned long nr, struct pt_regs *regs) > { > struct thread_info *ti; > diff --git a/arch/x86/include/asm/irq_regs.h b/arch/x86/include/asm/irq_regs.h > index 187ce59aea28e..a6fc1641e2861 100644 > --- a/arch/x86/include/asm/irq_regs.h > +++ b/arch/x86/include/asm/irq_regs.h > @@ -14,6 +14,8 @@ > > DECLARE_PER_CPU(struct pt_regs *, irq_regs); > > +/* Tell KMSAN to return an initialized struct pt_regs. */ > +__no_sanitize_memory > static inline struct pt_regs *get_irq_regs(void) > { > return __this_cpu_read(irq_regs); > diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h > index e2389ce9bf58a..098b1a8d6bc41 100644 > --- a/arch/x86/include/asm/syscall_wrapper.h > +++ b/arch/x86/include/asm/syscall_wrapper.h > @@ -196,6 +196,8 @@ struct pt_regs; > ALLOW_ERROR_INJECTION(__x64_sys##name, ERRNO); \ > static long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \ > static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\ > + /* Tell KMSAN to initialize |regs|. */ \ > + __no_sanitize_memory \ > asmlinkage long __x64_sys##name(const struct pt_regs *regs) \ > { \ > return __se_sys##name(SC_X86_64_REGS_TO_ARGS(x,__VA_ARGS__));\ > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index 5f973fed3c9ff..1f0250f14e462 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -1127,6 +1127,9 @@ static void local_apic_timer_interrupt(void) > * [ if a single-CPU system runs an SMP kernel then we call the local > * interrupt as well. Thus we cannot inline the local irq ... ] > */ > + > +/* Tell KMSAN to initialize |regs|. */ > +__no_sanitize_memory > __visible void __irq_entry smp_apic_timer_interrupt(struct pt_regs *regs) > { > struct pt_regs *old_regs = set_irq_regs(regs); > diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c > index 87b97897a8810..3d1691f81cada 100644 > --- a/arch/x86/kernel/dumpstack_64.c > +++ b/arch/x86/kernel/dumpstack_64.c > @@ -150,6 +150,11 @@ static bool in_irq_stack(unsigned long *stack, struct stack_info *info) > return true; > } > > +/* > + * This function may touch stale uninitialized values on stack. Do not > + * instrument it with KMSAN to avoid false positives. > + */ > +__no_sanitize_memory > int get_stack_info(unsigned long *stack, struct task_struct *task, > struct stack_info *info, unsigned long *visit_mask) > { > diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c > index ffd497804dbc3..5e8c6767e9916 100644 > --- a/arch/x86/kernel/process_64.c > +++ b/arch/x86/kernel/process_64.c > @@ -424,6 +424,11 @@ void compat_start_thread(struct pt_regs *regs, u32 new_ip, u32 new_sp) > * Kprobes not supported here. Set the probe on schedule instead. > * Function graph tracer not supported too. > */ > +/* > + * Avoid touching KMSAN state or reporting anything here, as __switch_to() does > + * weird things with tasks. > + */ > +__no_sanitize_memory > __visible __notrace_funcgraph struct task_struct * > __switch_to(struct task_struct *prev_p, struct task_struct *next_p) > { > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index d54cffdc7cac2..917268aee054e 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -638,7 +638,11 @@ NOKPROBE_SYMBOL(do_int3); > * Help handler running on a per-cpu (IST or entry trampoline) stack > * to switch to the normal thread stack if the interrupted code was in > * user mode. The actual stack switch is done in entry_64.S > + * > */ > + > +/* This function switches the registers - don't instrument it with KMSAN. */ > +__no_sanitize_memory > asmlinkage __visible notrace struct pt_regs *sync_regs(struct pt_regs *eregs) > { > struct pt_regs *regs = (struct pt_regs *)this_cpu_read(cpu_current_top_of_stack) - 1; > @@ -654,6 +658,11 @@ struct bad_iret_stack { > }; > > asmlinkage __visible notrace > +/* > + * Dark magic happening here, let's not instrument this function. > + * Also avoid copying any metadata by using raw __memmove(). > + */ > +__no_sanitize_memory > struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s) > { > /* > @@ -668,10 +677,10 @@ struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s) > (struct bad_iret_stack *)this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1; > > /* Copy the IRET target to the new stack. */ > - memmove(&new_stack->regs.ip, (void *)s->regs.sp, 5*8); > + __memmove(&new_stack->regs.ip, (void *)s->regs.sp, 5*8); > > /* Copy the remainder of the stack from the current stack. */ > - memmove(new_stack, s, offsetof(struct bad_iret_stack, regs.ip)); > + __memmove(new_stack, s, offsetof(struct bad_iret_stack, regs.ip)); > > BUG_ON(!user_mode(&new_stack->regs)); > return new_stack; > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 1a5937936ac75..bb1b659c12f6a 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -471,6 +471,11 @@ void wake_q_add_safe(struct wake_q_head *head, struct task_struct *task) > put_task_struct(task); > } > > +/* > + * Context switch here may lead to KMSAN task state corruption. Disable KMSAN > + * instrumentation. > + */ > +__no_sanitize_memory > void wake_up_q(struct wake_q_head *head) > { > struct wake_q_node *node = head->first; > @@ -3217,6 +3222,12 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev, > * past. prev == current is still correct but we need to recalculate this_rq > * because prev may have moved to another CPU. > */ > + > +/* > + * Context switch here may lead to KMSAN task state corruption. Disable KMSAN > + * instrumentation. > + */ > +__no_sanitize_memory > static struct rq *finish_task_switch(struct task_struct *prev) > __releases(rq->lock) > { > @@ -4052,6 +4063,12 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf) > * > * WARNING: must be called with preemption disabled! > */ > + > +/* > + * Context switch here may lead to KMSAN task state corruption. Disable KMSAN > + * instrumentation. > + */ > +__no_sanitize_memory > static void __sched notrace __schedule(bool preempt) > { > struct task_struct *prev, *next; > @@ -6789,6 +6806,11 @@ static inline int preempt_count_equals(int preempt_offset) > return (nested == preempt_offset); > } > > +/* > + * This function might be called from code that is not instrumented with KMSAN. > + * Nevertheless, treat its arguments as initialized. > + */ > +__no_sanitize_memory > void __might_sleep(const char *file, int line, int preempt_offset) > { > /* > -- > 2.25.1.696.g5e7596f4ac-goog >