linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ubsan: mark ubsan_type_mismatch_common inline
@ 2019-06-17 12:31 Arnd Bergmann
  2019-06-17 14:02 ` Peter Zijlstra
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2019-06-17 12:31 UTC (permalink / raw)
  To: Andrey Ryabinin, akpm
  Cc: Josh Poimboeuf, Peter Zijlstra, linux-kernel, Arnd Bergmann, stable

objtool points out a condition that it does not like:

lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x4a: call to stackleak_track_stack() with UACCESS enabled
lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x4a: call to stackleak_track_stack() with UACCESS enabled

I guess this is related to the call ubsan_type_mismatch_common()
not being inline before it calls user_access_restore(), though
I don't fully understand why that is a problem.

Marking the function inline shuts up the warning and might be
the right thing to do. The patch that caused this is marked
for stable backports, so this one should probably be backported
as well.

Fixes: 42440c1f9911 ("lib/ubsan: add type mismatch handler for new GCC/Clang")
Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/ubsan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ubsan.c b/lib/ubsan.c
index ecc179338094..3d8836f0fc5c 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -309,7 +309,7 @@ static void handle_object_size_mismatch(struct type_mismatch_data_common *data,
 	ubsan_epilogue(&flags);
 }
 
-static void ubsan_type_mismatch_common(struct type_mismatch_data_common *data,
+static __always_inline void ubsan_type_mismatch_common(struct type_mismatch_data_common *data,
 				unsigned long ptr)
 {
 	unsigned long flags = user_access_save();
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] ubsan: mark ubsan_type_mismatch_common inline
  2019-06-17 12:31 [PATCH] ubsan: mark ubsan_type_mismatch_common inline Arnd Bergmann
@ 2019-06-17 14:02 ` Peter Zijlstra
  2019-06-18 12:56   ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2019-06-17 14:02 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Andrey Ryabinin, akpm, Josh Poimboeuf, linux-kernel, stable

On Mon, Jun 17, 2019 at 02:31:09PM +0200, Arnd Bergmann wrote:
> objtool points out a condition that it does not like:
> 
> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x4a: call to stackleak_track_stack() with UACCESS enabled
> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x4a: call to stackleak_track_stack() with UACCESS enabled
> 
> I guess this is related to the call ubsan_type_mismatch_common()
> not being inline before it calls user_access_restore(), though
> I don't fully understand why that is a problem.

The rules are that when AC is set, one is not allowed to CALL schedule,
because scheduling does not save/restore AC.  Preemption, through the
exceptions is fine, because the exceptions do save/restore AC.

And while most functions do not appear to call into schedule, function
trace ensures that every single call does in fact call into schedule.
Therefore any CALL (with AC set) is invalid.

> Marking the function inline shuts up the warning and might be
> the right thing to do. The patch that caused this is marked
> for stable backports, so this one should probably be backported
> as well.

This appears to be a 'fun' interaction between different checkers. What
happens is that __ubsan_handle_type_mismatch*() calls into
stackleak_track_stack() because it has an on-stack variable. It does
this before calling ubsan_type_mismatch_common().

ubsan_type_mismatch_common() does user_access_save/restore which
saves/restores AC and allows 'normal' code to be ran.

With the proposed __always_inline, the code generation changes such that
we run user_access_save() _before_ stackleack_track_stack() (for,
afaict, undefined raisins), and the warning goes away.

Maybe we should disable stackleak when building ubsan instead? We
already disable stack-protector when building ubsan.

> Fixes: 42440c1f9911 ("lib/ubsan: add type mismatch handler for new GCC/Clang")

I don't think this is quite right, because back then there wasn't any
uaccess validation.

> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  lib/ubsan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/ubsan.c b/lib/ubsan.c
> index ecc179338094..3d8836f0fc5c 100644
> --- a/lib/ubsan.c
> +++ b/lib/ubsan.c
> @@ -309,7 +309,7 @@ static void handle_object_size_mismatch(struct type_mismatch_data_common *data,
>  	ubsan_epilogue(&flags);
>  }
>  
> -static void ubsan_type_mismatch_common(struct type_mismatch_data_common *data,
> +static __always_inline void ubsan_type_mismatch_common(struct type_mismatch_data_common *data,
>  				unsigned long ptr)
>  {
>  	unsigned long flags = user_access_save();

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ubsan: mark ubsan_type_mismatch_common inline
  2019-06-17 14:02 ` Peter Zijlstra
@ 2019-06-18 12:56   ` Arnd Bergmann
  2019-06-18 13:27     ` Andrey Ryabinin
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2019-06-18 12:56 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Andrey Ryabinin, Andrew Morton, Josh Poimboeuf,
	Linux Kernel Mailing List, # 3.4.x

On Mon, Jun 17, 2019 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Jun 17, 2019 at 02:31:09PM +0200, Arnd Bergmann wrote:
> > objtool points out a condition that it does not like:
> >
> > lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x4a: call to stackleak_track_stack() with UACCESS enabled
> > lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x4a: call to stackleak_track_stack() with UACCESS enabled
> >
> > I guess this is related to the call ubsan_type_mismatch_common()
> > not being inline before it calls user_access_restore(), though
> > I don't fully understand why that is a problem.
>
> The rules are that when AC is set, one is not allowed to CALL schedule,
> because scheduling does not save/restore AC.  Preemption, through the
> exceptions is fine, because the exceptions do save/restore AC.
>
> And while most functions do not appear to call into schedule, function
> trace ensures that every single call does in fact call into schedule.
> Therefore any CALL (with AC set) is invalid.

I see that stackleak_track_stack is already marked 'notrace',
since we must ensure we don't recurse when calling into it from
any of the function trace logic.

Does that mean we could just mark it as another safe call?

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -486,6 +486,7 @@ static const char *uaccess_safe_builtin[] = {
        "__ubsan_handle_type_mismatch",
        "__ubsan_handle_type_mismatch_v1",
        /* misc */
+       "stackleak_track_stack",
        "csum_partial_copy_generic",
        "__memcpy_mcsafe",
        "ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */


> Maybe we should disable stackleak when building ubsan instead? We
> already disable stack-protector when building ubsan.

I couldn't find out how that is done.

> > Fixes: 42440c1f9911 ("lib/ubsan: add type mismatch handler for new GCC/Clang")
>
> I don't think this is quite right, because back then there wasn't any
> uaccess validation.

Right.

       Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ubsan: mark ubsan_type_mismatch_common inline
  2019-06-18 12:56   ` Arnd Bergmann
@ 2019-06-18 13:27     ` Andrey Ryabinin
  2019-06-18 13:59       ` Peter Zijlstra
  0 siblings, 1 reply; 7+ messages in thread
From: Andrey Ryabinin @ 2019-06-18 13:27 UTC (permalink / raw)
  To: Arnd Bergmann, Peter Zijlstra
  Cc: Andrew Morton, Josh Poimboeuf, Linux Kernel Mailing List, # 3.4.x



On 6/18/19 3:56 PM, Arnd Bergmann wrote:
> On Mon, Jun 17, 2019 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Mon, Jun 17, 2019 at 02:31:09PM +0200, Arnd Bergmann wrote:
>>> objtool points out a condition that it does not like:
>>>
>>> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x4a: call to stackleak_track_stack() with UACCESS enabled
>>> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x4a: call to stackleak_track_stack() with UACCESS enabled
>>>
>>> I guess this is related to the call ubsan_type_mismatch_common()
>>> not being inline before it calls user_access_restore(), though
>>> I don't fully understand why that is a problem.
>>
>> The rules are that when AC is set, one is not allowed to CALL schedule,
>> because scheduling does not save/restore AC.  Preemption, through the
>> exceptions is fine, because the exceptions do save/restore AC.
>>
>> And while most functions do not appear to call into schedule, function
>> trace ensures that every single call does in fact call into schedule.
>> Therefore any CALL (with AC set) is invalid.
> 
> I see that stackleak_track_stack is already marked 'notrace',
> since we must ensure we don't recurse when calling into it from
> any of the function trace logic.
> 
> Does that mean we could just mark it as another safe call?
> 
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -486,6 +486,7 @@ static const char *uaccess_safe_builtin[] = {
>         "__ubsan_handle_type_mismatch",
>         "__ubsan_handle_type_mismatch_v1",
>         /* misc */
> +       "stackleak_track_stack",
>         "csum_partial_copy_generic",
>         "__memcpy_mcsafe",
>         "ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */
> 
> 
>> Maybe we should disable stackleak when building ubsan instead? We
>> already disable stack-protector when building ubsan.
> 
> I couldn't find out how that is done.
> 

I guess this:
ccflags-y += $(DISABLE_STACKLEAK_PLUGIN)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ubsan: mark ubsan_type_mismatch_common inline
  2019-06-18 13:27     ` Andrey Ryabinin
@ 2019-06-18 13:59       ` Peter Zijlstra
  2019-06-18 15:06         ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2019-06-18 13:59 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Arnd Bergmann, Andrew Morton, Josh Poimboeuf,
	Linux Kernel Mailing List, # 3.4.x

On Tue, Jun 18, 2019 at 04:27:45PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 6/18/19 3:56 PM, Arnd Bergmann wrote:
> > On Mon, Jun 17, 2019 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >>
> >> On Mon, Jun 17, 2019 at 02:31:09PM +0200, Arnd Bergmann wrote:
> >>> objtool points out a condition that it does not like:
> >>>
> >>> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch()+0x4a: call to stackleak_track_stack() with UACCESS enabled
> >>> lib/ubsan.o: warning: objtool: __ubsan_handle_type_mismatch_v1()+0x4a: call to stackleak_track_stack() with UACCESS enabled
> >>>
> >>> I guess this is related to the call ubsan_type_mismatch_common()
> >>> not being inline before it calls user_access_restore(), though
> >>> I don't fully understand why that is a problem.
> >>
> >> The rules are that when AC is set, one is not allowed to CALL schedule,
> >> because scheduling does not save/restore AC.  Preemption, through the
> >> exceptions is fine, because the exceptions do save/restore AC.
> >>
> >> And while most functions do not appear to call into schedule, function
> >> trace ensures that every single call does in fact call into schedule.
> >> Therefore any CALL (with AC set) is invalid.
> > 
> > I see that stackleak_track_stack is already marked 'notrace',
> > since we must ensure we don't recurse when calling into it from
> > any of the function trace logic.
> > 
> > Does that mean we could just mark it as another safe call?
> > 
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -486,6 +486,7 @@ static const char *uaccess_safe_builtin[] = {
> >         "__ubsan_handle_type_mismatch",
> >         "__ubsan_handle_type_mismatch_v1",
> >         /* misc */
> > +       "stackleak_track_stack",
> >         "csum_partial_copy_generic",
> >         "__memcpy_mcsafe",
> >         "ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */

Indeed, we could do this.

> > 
> >> Maybe we should disable stackleak when building ubsan instead? We
> >> already disable stack-protector when building ubsan.
> > 
> > I couldn't find out how that is done.
> > 
> 
> I guess this:
> ccflags-y += $(DISABLE_STACKLEAK_PLUGIN)

Or more specifically this, I guess:

CFLAGS_ubsan.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector) $(DISABLE_STACKLEAK_PLUGIN)

we'd not want to exclude all of lib/ from stackleak I figure.

Of these two options, I think I prefer the latter, because a smaller
whitelist is a better whitelist and since we already disable
stack protector, it is only consistent to also disable stack leak.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ubsan: mark ubsan_type_mismatch_common inline
  2019-06-18 13:59       ` Peter Zijlstra
@ 2019-06-18 15:06         ` Arnd Bergmann
  2019-06-19  7:57           ` Peter Zijlstra
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2019-06-18 15:06 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Andrey Ryabinin, Andrew Morton, Josh Poimboeuf,
	Linux Kernel Mailing List, # 3.4.x

On Tue, Jun 18, 2019 at 3:59 PM Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jun 18, 2019 at 04:27:45PM +0300, Andrey Ryabinin wrote:
> > On 6/18/19 3:56 PM, Arnd Bergmann wrote:
> > > On Mon, Jun 17, 2019 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > I guess this:
> > ccflags-y += $(DISABLE_STACKLEAK_PLUGIN)
>
> Or more specifically this, I guess:
>
> CFLAGS_ubsan.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector) $(DISABLE_STACKLEAK_PLUGIN)
>
> we'd not want to exclude all of lib/ from stackleak I figure.
>
> Of these two options, I think I prefer the latter, because a smaller
> whitelist is a better whitelist and since we already disable
> stack protector, it is only consistent to also disable stack leak.

Ok, sounds good to me. Can you send that upstream then, or should
I write it up as a proper patch?

       Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ubsan: mark ubsan_type_mismatch_common inline
  2019-06-18 15:06         ` Arnd Bergmann
@ 2019-06-19  7:57           ` Peter Zijlstra
  0 siblings, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2019-06-19  7:57 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Andrey Ryabinin, Andrew Morton, Josh Poimboeuf,
	Linux Kernel Mailing List, # 3.4.x

On Tue, Jun 18, 2019 at 05:06:39PM +0200, Arnd Bergmann wrote:
> On Tue, Jun 18, 2019 at 3:59 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > On Tue, Jun 18, 2019 at 04:27:45PM +0300, Andrey Ryabinin wrote:
> > > On 6/18/19 3:56 PM, Arnd Bergmann wrote:
> > > > On Mon, Jun 17, 2019 at 4:02 PM Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > I guess this:
> > > ccflags-y += $(DISABLE_STACKLEAK_PLUGIN)
> >
> > Or more specifically this, I guess:
> >
> > CFLAGS_ubsan.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector) $(DISABLE_STACKLEAK_PLUGIN)
> >
> > we'd not want to exclude all of lib/ from stackleak I figure.
> >
> > Of these two options, I think I prefer the latter, because a smaller
> > whitelist is a better whitelist and since we already disable
> > stack protector, it is only consistent to also disable stack leak.
> 
> Ok, sounds good to me. Can you send that upstream then, or should
> I write it up as a proper patch?

If you could verify it actually works that would be great, I haven't
tried to construct a failing .config yet.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-06-19  7:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 12:31 [PATCH] ubsan: mark ubsan_type_mismatch_common inline Arnd Bergmann
2019-06-17 14:02 ` Peter Zijlstra
2019-06-18 12:56   ` Arnd Bergmann
2019-06-18 13:27     ` Andrey Ryabinin
2019-06-18 13:59       ` Peter Zijlstra
2019-06-18 15:06         ` Arnd Bergmann
2019-06-19  7:57           ` Peter Zijlstra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).