[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK8P3a33LQAzsReSUyB_aZxkws28RP=oJocQXonYbxxBky7aaQ@mail.gmail.com>
Date: Fri, 30 Aug 2019 17:58:41 +0200
From: Arnd Bergmann <arnd@...db.de>
To: Josh Poimboeuf <jpoimboe@...hat.com>
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 5:14 PM Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> 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().
Yes, I meant both here of course (as in my earlier patch).
> > 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().
Ah, that's what I get for looking only at the compat version.
> > - 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.
Fine with me, too.
> 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.
I suspect that's going to be harder. The clang-9 release is going to be
soon, and that change probably wouldn't be considered a regression fix.
Maybe Nick can find what happens, but I don't actually see any reference
to KASAN in the llvm source code related to the memset generation.
https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CGExprAgg.cpp#L1803
has a check for >16 bytes, but that again does not match my observation.
Arnd
Powered by blists - more mailing lists