Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1583256049-15497-1-git-send-email-cai@lca.pw>
In-Reply-To: <1583256049-15497-1-git-send-email-cai@lca.pw>
From:   Marco Elver <elver@google.com>
Date:   Tue, 3 Mar 2020 18:53:20 +0100
Message-ID: <CANpmjNPJc_45+h_yJWwmw=YUuWduD6pPX2vdfPVekPvnnHd+_Q@mail.gmail.com>
Subject: Re: [PATCH -next] signal: annotate data races in sys_rt_sigaction
To:     Qian Cai <cai@lca.pw>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>, catalin.marinas@arm.com,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 3 Mar 2020 at 18:21, Qian Cai <cai@lca.pw> wrote:
>
> Kmemleak could scan task stacks while plain writes happens to those
> stack variables which could results in data races. For example, in
> sys_rt_sigaction and do_sigaction(), it could have plain writes in
> a 32-byte size. Since the kmemleak does not care about the actual values
> of a non-pointer and all do_sigaction() call sites only copy to stack
> variables, annotate them as intentional data races using the
> data_race() macro. The data races were reported by KCSAN,
>
>  BUG: KCSAN: data-race in _copy_from_user / scan_block
>
>  read to 0xffffb3074e61fe58 of 8 bytes by task 356 on cpu 19:
>   scan_block+0x6e/0x1a0
>   scan_block at mm/kmemleak.c:1251
>   kmemleak_scan+0xbea/0xd20
>   kmemleak_scan at mm/kmemleak.c:1482
>   kmemleak_scan_thread+0xcc/0xfa
>   kthread+0x1cd/0x1f0
>   ret_from_fork+0x3a/0x50

I think we should move the annotations to kmemleak instead of signal.c.

Because putting a "data_race()" on the accesses in signal.c just
because of Kmemleak feels wrong because then we might miss other more
serious issues. Kmemleak isn't normally enabled in a non-debug kernel.

I wonder if it'd be a better idea to just disable KCSAN on scan_block
with __no_kcsan? If Kmemleak only does reads, then __no_kcsan will do
the right thing here, because the reads are hidden completely from
KCSAN. With "data_race()" you would still have to mark both accesses
in signal.c and kmemleak (this is by design, so that we document all
intentionally data-racy accesses).

An alternative would be to just exempt kmemleak from KCSAN with
"KCSAN_SANITIZE_kmemleak.o := n". Given Kmemleak is a debugging tool
and it's expected to race with all kinds of accesses, maybe that's the
best option.

Thanks,
-- Marco

>  write to 0xffffb3074e61fe58 of 32 bytes by task 30208 on cpu 2:
>   _copy_from_user+0xb2/0xe0
>   copy_user_generic at arch/x86/include/asm/uaccess_64.h:37
>   (inlined by) raw_copy_from_user at arch/x86/include/asm/uaccess_64.h:71
>   (inlined by) _copy_from_user at lib/usercopy.c:15
>   __x64_sys_rt_sigaction+0x83/0x140
>   __do_sys_rt_sigaction at kernel/signal.c:4245
>   (inlined by) __se_sys_rt_sigaction at kernel/signal.c:4233
>   (inlined by) __x64_sys_rt_sigaction at kernel/signal.c:4233
>   do_syscall_64+0x91/0xb05
>   entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  kernel/signal.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 5b2396350dd1..bf39078c8be1 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3964,14 +3964,15 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
>
>         spin_lock_irq(&p->sighand->siglock);
>         if (oact)
> -               *oact = *k;
> +               /* Kmemleak could scan the task stack. */
> +               data_race(*oact = *k);
>
>         sigaction_compat_abi(act, oact);
>
>         if (act) {
>                 sigdelsetmask(&act->sa.sa_mask,
>                               sigmask(SIGKILL) | sigmask(SIGSTOP));
> -               *k = *act;
> +               data_race(*k = *act);
>                 /*
>                  * POSIX 3.3.1.3:
>                  *  "Setting a signal action to SIG_IGN for a signal that is
> @@ -4242,7 +4243,9 @@ int __compat_save_altstack(compat_stack_t __user *uss, unsigned long sp)
>         if (sigsetsize != sizeof(sigset_t))
>                 return -EINVAL;
>
> -       if (act && copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa)))
> +       if (act &&
> +           /* Kmemleak could scan the task stack. */
> +           data_race(copy_from_user(&new_sa.sa, act, sizeof(new_sa.sa))))
>                 return -EFAULT;
>
>         ret = do_sigaction(sig, act ? &new_sa : NULL, oact ? &old_sa : NULL);
> --
> 1.8.3.1
>