linux-kernel - Re: objtool warning "uses BP as a scratch register" with clang-9

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Aug 2019 10:14:22 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Ilie Halip <ilie.halip@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: objtool warning "uses BP as a scratch register" with clang-9

On Fri, Aug 30, 2019 at 12:44:24PM +0200, Arnd Bergmann wrote:
> On Fri, Aug 30, 2019 at 1:24 AM Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > On Wed, Aug 28, 2019 at 05:40:01PM +0200, Arnd Bergmann wrote:
> > > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> > > index 8eb7193e158d..fd49d28abbc5 100644
> > > --- a/arch/x86/kernel/signal.c
> > > +++ b/arch/x86/kernel/signal.c
> > > @@ -414,6 +414,9 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
> > >                  */
> > >                 put_user_ex(*((u64 *)&rt_retcode), (u64 *)frame->retcode);
> > >         } put_user_catch(err);
> > > +
> > > +       if (current->sas_ss_flags & SS_AUTODISARM)
> > > +               sas_ss_reset(current);
> > >
> > >         err |= copy_siginfo_to_user(&frame->info, &ksig->info);
> > >         err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
> 
> > > diff --git a/include/linux/signal.h b/include/linux/signal.h
> > > index 67ceb6d7c869..9056239787f7 100644
> > > --- a/include/linux/signal.h
> > > +++ b/include/linux/signal.h
> > > @@ -435,8 +435,6 @@ int __save_altstack(stack_t __user *, unsigned long);
> > >         put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
> > >         put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
> > >         put_user_ex(t->sas_ss_size, &__uss->ss_size); \
> > > -       if (t->sas_ss_flags & SS_AUTODISARM) \
> > > -               sas_ss_reset(t); \
> > >  } while (0);
> > >
> > >  #ifdef CONFIG_PROC_FS
> >
> > Reviewed-by: Josh Poimboeuf <jpoimboe@...hat.com>
> 
> Thanks! Before I submit this version for inclusion, let's make sure this
> is the best variant. I noticed later that save_altstack_ex() is meant to
> behave the same as __save_altstack(), but my patch breaks that
> assumption.

Good point.

There's also compat_save_altstack_ex() -- which presumably needs the
same fix? -- and __compat_save_altstack().

> Two other alternatives I can think of are
> 
> - completely open-code save_altstack_ex() in its only call site on x86,
>   in addition to the change above

But it has two call sites: the 32-bit and 64-bit versions of
save_altstack_ex().

> - explicitly mark memset() as an exception in objtool in
>   uaccess_safe_builtin[], assuming that is actually safe.

I wonder if this might open up more theoretical SMAP holes for other
callers to memset().

What about just adding a couple of WRITE_ONCE's to sas_ss_reset()?  That
would probably be the least disruptive option.

Or even better, it would be great if we could get Clang to change their
memset() insertion heuristics, so that KASAN acts more like non-KASAN
code in that regard.

-- 
Josh