[PATCH -next] signal: annotate data races in sys_rt_sigaction

[PATCH -next] signal: annotate data races in sys_rt_sigaction

[Qian Cai]

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

 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

Re: [PATCH -next] signal: annotate data races in sys_rt_sigaction

[Marco Elver]

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
>

Re: [PATCH -next] signal: annotate data races in sys_rt_sigaction

[Marco Elver]

On Tue, 3 Mar 2020 at 18:53, Marco Elver <elver@google.com> wrote:
>
> 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.

I saw there are already some data_race() annotations in Kmemleak.
Given there are probably more things waiting to be found in Kmemleak,
KCSAN_SANITIZE_kmemleak.o := n might just be the best option. I think
this is fair, because we really do not want to annotate anything
outside Kmemleak just because Kmemleak scans everything. The existing
annotations should probably be reverted in that case.

Thanks,
-- Marco


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

Re: [PATCH -next] signal: annotate data races in sys_rt_sigaction

[Qian Cai]

On Tue, 2020-03-03 at 19:26 +0100, Marco Elver wrote:
> On Tue, 3 Mar 2020 at 18:53, Marco Elver <elver@google.com> wrote:
> > 
> > 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.
> 
> I saw there are already some data_race() annotations in Kmemleak.
> Given there are probably more things waiting to be found in Kmemleak,
> KCSAN_SANITIZE_kmemleak.o := n might just be the best option. I think
> this is fair, because we really do not want to annotate anything
> outside Kmemleak just because Kmemleak scans everything. The existing
> annotations should probably be reverted in that case.

Good idea. I'll post a new patch for that.