* [PATCH] samples/ftrace: mark my_tramp[12]? global
@ 2020-11-13 18:34 Sami Tolvanen
2020-11-13 18:57 ` Kees Cook
2020-11-16 16:39 ` Steven Rostedt
0 siblings, 2 replies; 8+ messages in thread
From: Sami Tolvanen @ 2020-11-13 18:34 UTC (permalink / raw)
To: Steven Rostedt (VMware)
Cc: Kees Cook, Josh Poimboeuf, Colin Ian King, linux-kernel, Sami Tolvanen
my_tramp[12]? are declared as global functions in C, but they are not
marked global in the inline assembly definition. This mismatch confuses
Clang's Control-Flow Integrity checking. Fix the definitions by adding
.globl.
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
samples/ftrace/ftrace-direct-modify.c | 2 ++
samples/ftrace/ftrace-direct-too.c | 1 +
samples/ftrace/ftrace-direct.c | 1 +
3 files changed, 4 insertions(+)
diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c
index c13a5bc5095b..5b9a09957c6e 100644
--- a/samples/ftrace/ftrace-direct-modify.c
+++ b/samples/ftrace/ftrace-direct-modify.c
@@ -21,6 +21,7 @@ static unsigned long my_ip = (unsigned long)schedule;
asm (
" .pushsection .text, \"ax\", @progbits\n"
" .type my_tramp1, @function\n"
+" .globl my_tramp1\n"
" my_tramp1:"
" pushq %rbp\n"
" movq %rsp, %rbp\n"
@@ -29,6 +30,7 @@ asm (
" .size my_tramp1, .-my_tramp1\n"
" ret\n"
" .type my_tramp2, @function\n"
+" .globl my_tramp2\n"
" my_tramp2:"
" pushq %rbp\n"
" movq %rsp, %rbp\n"
diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c
index d5c5022be664..3f0079c9bd6f 100644
--- a/samples/ftrace/ftrace-direct-too.c
+++ b/samples/ftrace/ftrace-direct-too.c
@@ -16,6 +16,7 @@ extern void my_tramp(void *);
asm (
" .pushsection .text, \"ax\", @progbits\n"
" .type my_tramp, @function\n"
+" .globl my_tramp\n"
" my_tramp:"
" pushq %rbp\n"
" movq %rsp, %rbp\n"
diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c
index 63ca06d42c80..a2729d1ef17f 100644
--- a/samples/ftrace/ftrace-direct.c
+++ b/samples/ftrace/ftrace-direct.c
@@ -14,6 +14,7 @@ extern void my_tramp(void *);
asm (
" .pushsection .text, \"ax\", @progbits\n"
" .type my_tramp, @function\n"
+" .globl my_tramp\n"
" my_tramp:"
" pushq %rbp\n"
" movq %rsp, %rbp\n"
base-commit: 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
--
2.29.2.299.gdc1121823c-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] samples/ftrace: mark my_tramp[12]? global
2020-11-13 18:34 [PATCH] samples/ftrace: mark my_tramp[12]? global Sami Tolvanen
@ 2020-11-13 18:57 ` Kees Cook
2020-11-16 16:39 ` Steven Rostedt
1 sibling, 0 replies; 8+ messages in thread
From: Kees Cook @ 2020-11-13 18:57 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Steven Rostedt (VMware), Josh Poimboeuf, Colin Ian King, linux-kernel
On Fri, Nov 13, 2020 at 10:34:14AM -0800, Sami Tolvanen wrote:
> my_tramp[12]? are declared as global functions in C, but they are not
> marked global in the inline assembly definition. This mismatch confuses
> Clang's Control-Flow Integrity checking. Fix the definitions by adding
> .globl.
>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] samples/ftrace: mark my_tramp[12]? global
2020-11-13 18:34 [PATCH] samples/ftrace: mark my_tramp[12]? global Sami Tolvanen
2020-11-13 18:57 ` Kees Cook
@ 2020-11-16 16:39 ` Steven Rostedt
2020-11-16 20:10 ` Sami Tolvanen
1 sibling, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2020-11-16 16:39 UTC (permalink / raw)
To: Sami Tolvanen; +Cc: Kees Cook, Josh Poimboeuf, Colin Ian King, linux-kernel
On Fri, 13 Nov 2020 10:34:14 -0800
Sami Tolvanen <samitolvanen@google.com> wrote:
> my_tramp[12]? are declared as global functions in C, but they are not
> marked global in the inline assembly definition. This mismatch confuses
> Clang's Control-Flow Integrity checking. Fix the definitions by adding
> .globl.
>
Actually, since that function is not really global, would it work if you
removed the "extern" from the my_tramp declaration?
In other words, is there a way to tell C that a function is declared in an
inline assembly block?
-- Steve
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
> samples/ftrace/ftrace-direct-modify.c | 2 ++
> samples/ftrace/ftrace-direct-too.c | 1 +
> samples/ftrace/ftrace-direct.c | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/samples/ftrace/ftrace-direct-modify.c b/samples/ftrace/ftrace-direct-modify.c
> index c13a5bc5095b..5b9a09957c6e 100644
> --- a/samples/ftrace/ftrace-direct-modify.c
> +++ b/samples/ftrace/ftrace-direct-modify.c
> @@ -21,6 +21,7 @@ static unsigned long my_ip = (unsigned long)schedule;
> asm (
> " .pushsection .text, \"ax\", @progbits\n"
> " .type my_tramp1, @function\n"
> +" .globl my_tramp1\n"
> " my_tramp1:"
> " pushq %rbp\n"
> " movq %rsp, %rbp\n"
> @@ -29,6 +30,7 @@ asm (
> " .size my_tramp1, .-my_tramp1\n"
> " ret\n"
> " .type my_tramp2, @function\n"
> +" .globl my_tramp2\n"
> " my_tramp2:"
> " pushq %rbp\n"
> " movq %rsp, %rbp\n"
> diff --git a/samples/ftrace/ftrace-direct-too.c b/samples/ftrace/ftrace-direct-too.c
> index d5c5022be664..3f0079c9bd6f 100644
> --- a/samples/ftrace/ftrace-direct-too.c
> +++ b/samples/ftrace/ftrace-direct-too.c
> @@ -16,6 +16,7 @@ extern void my_tramp(void *);
> asm (
> " .pushsection .text, \"ax\", @progbits\n"
> " .type my_tramp, @function\n"
> +" .globl my_tramp\n"
> " my_tramp:"
> " pushq %rbp\n"
> " movq %rsp, %rbp\n"
> diff --git a/samples/ftrace/ftrace-direct.c b/samples/ftrace/ftrace-direct.c
> index 63ca06d42c80..a2729d1ef17f 100644
> --- a/samples/ftrace/ftrace-direct.c
> +++ b/samples/ftrace/ftrace-direct.c
> @@ -14,6 +14,7 @@ extern void my_tramp(void *);
> asm (
> " .pushsection .text, \"ax\", @progbits\n"
> " .type my_tramp, @function\n"
> +" .globl my_tramp\n"
> " my_tramp:"
> " pushq %rbp\n"
> " movq %rsp, %rbp\n"
>
> base-commit: 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] samples/ftrace: mark my_tramp[12]? global
2020-11-16 16:39 ` Steven Rostedt
@ 2020-11-16 20:10 ` Sami Tolvanen
2020-11-16 20:38 ` Steven Rostedt
0 siblings, 1 reply; 8+ messages in thread
From: Sami Tolvanen @ 2020-11-16 20:10 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Kees Cook, Josh Poimboeuf, Colin Ian King, LKML
On Mon, Nov 16, 2020 at 8:39 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 13 Nov 2020 10:34:14 -0800
> Sami Tolvanen <samitolvanen@google.com> wrote:
>
> > my_tramp[12]? are declared as global functions in C, but they are not
> > marked global in the inline assembly definition. This mismatch confuses
> > Clang's Control-Flow Integrity checking. Fix the definitions by adding
> > .globl.
> >
>
> Actually, since that function is not really global, would it work if you
> removed the "extern" from the my_tramp declaration?
Unfortunately not, removing the "extern" doesn't seem to change anything.
> In other words, is there a way to tell C that a function is declared in an
> inline assembly block?
I'm not sure if there's a way to tell C that a static function is
declared in inline assembly. At least I couldn't find a way that would
make the compiler happy.
Sami
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] samples/ftrace: mark my_tramp[12]? global
2020-11-16 20:10 ` Sami Tolvanen
@ 2020-11-16 20:38 ` Steven Rostedt
2020-11-16 21:10 ` Sami Tolvanen
0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2020-11-16 20:38 UTC (permalink / raw)
To: Sami Tolvanen; +Cc: Kees Cook, Josh Poimboeuf, Colin Ian King, LKML
On Mon, 16 Nov 2020 12:10:10 -0800
Sami Tolvanen <samitolvanen@google.com> wrote:
> On Mon, Nov 16, 2020 at 8:39 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Fri, 13 Nov 2020 10:34:14 -0800
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > > my_tramp[12]? are declared as global functions in C, but they are not
> > > marked global in the inline assembly definition. This mismatch confuses
> > > Clang's Control-Flow Integrity checking. Fix the definitions by adding
> > > .globl.
> > >
> >
> > Actually, since that function is not really global, would it work if you
> > removed the "extern" from the my_tramp declaration?
>
> Unfortunately not, removing the "extern" doesn't seem to change anything.
>
> > In other words, is there a way to tell C that a function is declared in an
> > inline assembly block?
>
> I'm not sure if there's a way to tell C that a static function is
> declared in inline assembly. At least I couldn't find a way that would
> make the compiler happy.
I'm trying to see the warning. What option makes clang trigger a warning on
this?
From user space, I'm just using the following file:
#include <stdio.h>
void my_direct_func(char *str)
{
printf("%s\n", str);
}
int test(char *str);
asm (
" .pushsection .text, \"ax\", @progbits\n"
" .type test, @function\n"
" test:"
" pushq %rbp\n"
" movq %rsp, %rbp\n"
" pushq %rdi\n"
" call my_direct_func\n"
" popq %rdi\n"
" leave\n"
" ret\n"
" .size test, .-test\n"
" .popsection\n"
);
int main (int argc, char **argv)
{
test("hello");
return 0;
}
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] samples/ftrace: mark my_tramp[12]? global
2020-11-16 20:38 ` Steven Rostedt
@ 2020-11-16 21:10 ` Sami Tolvanen
2020-11-16 21:18 ` Steven Rostedt
0 siblings, 1 reply; 8+ messages in thread
From: Sami Tolvanen @ 2020-11-16 21:10 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Kees Cook, Josh Poimboeuf, Colin Ian King, LKML
On Mon, Nov 16, 2020 at 12:38 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 16 Nov 2020 12:10:10 -0800
> Sami Tolvanen <samitolvanen@google.com> wrote:
>
> > On Mon, Nov 16, 2020 at 8:39 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > On Fri, 13 Nov 2020 10:34:14 -0800
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >
> > > > my_tramp[12]? are declared as global functions in C, but they are not
> > > > marked global in the inline assembly definition. This mismatch confuses
> > > > Clang's Control-Flow Integrity checking. Fix the definitions by adding
> > > > .globl.
> > > >
> > >
> > > Actually, since that function is not really global, would it work if you
> > > removed the "extern" from the my_tramp declaration?
> >
> > Unfortunately not, removing the "extern" doesn't seem to change anything.
> >
> > > In other words, is there a way to tell C that a function is declared in an
> > > inline assembly block?
> >
> > I'm not sure if there's a way to tell C that a static function is
> > declared in inline assembly. At least I couldn't find a way that would
> > make the compiler happy.
>
> I'm trying to see the warning. What option makes clang trigger a warning on
> this?
Clang doesn't warn about this as we're building a module, it just
generates a reference to a non-existing global "my_tramp" symbol,
because the one defined in inline assembly has a local binding:
$ readelf --symbols --wide ftrace-direct.lto.o | grep my_tramp
16: 0000000000000000 13 FUNC LOCAL DEFAULT 1 my_tramp
33: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND my_tramp
42: 0000000000000000 8 FUNC GLOBAL HIDDEN 8 my_tramp.cfi_jt
This would prevent the module from loading, which modpost catches:
ERROR: modpost: "my_tramp" [samples/ftrace/ftrace-direct.ko] undefined!
> From user space, I'm just using the following file:
As this error happens only with Control-Flow Integrity, we need to
take the address of the "test" function to force the compiler to
generate a jump table entry for it. Here's a slightly tweaked
stand-alone reproducer:
https://godbolt.org/z/GnzjE4
Sami
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] samples/ftrace: mark my_tramp[12]? global
2020-11-16 21:10 ` Sami Tolvanen
@ 2020-11-16 21:18 ` Steven Rostedt
2020-11-16 21:27 ` Sami Tolvanen
0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2020-11-16 21:18 UTC (permalink / raw)
To: Sami Tolvanen; +Cc: Kees Cook, Josh Poimboeuf, Colin Ian King, LKML
On Mon, 16 Nov 2020 13:10:57 -0800
Sami Tolvanen <samitolvanen@google.com> wrote:
> Clang doesn't warn about this as we're building a module, it just
> generates a reference to a non-existing global "my_tramp" symbol,
> because the one defined in inline assembly has a local binding:
>
> $ readelf --symbols --wide ftrace-direct.lto.o | grep my_tramp
> 16: 0000000000000000 13 FUNC LOCAL DEFAULT 1 my_tramp
> 33: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND my_tramp
> 42: 0000000000000000 8 FUNC GLOBAL HIDDEN 8 my_tramp.cfi_jt
>
> This would prevent the module from loading, which modpost catches:
>
> ERROR: modpost: "my_tramp" [samples/ftrace/ftrace-direct.ko] undefined!
>
> > From user space, I'm just using the following file:
>
> As this error happens only with Control-Flow Integrity, we need to
> take the address of the "test" function to force the compiler to
> generate a jump table entry for it. Here's a slightly tweaked
> stand-alone reproducer:
>
> https://godbolt.org/z/GnzjE4
Thanks, we don't need to look more into this. It was mostly my curiosity to
find a way to have the compiler know about a function declared statically
in inline assembly. Maybe I'm asking for too much ;-)
I'll take your original patch. Does it need to go to stable, or is this not
that big of an issue to allow it to be added in the next merge window?
-- Steve
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] samples/ftrace: mark my_tramp[12]? global
2020-11-16 21:18 ` Steven Rostedt
@ 2020-11-16 21:27 ` Sami Tolvanen
0 siblings, 0 replies; 8+ messages in thread
From: Sami Tolvanen @ 2020-11-16 21:27 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Kees Cook, Josh Poimboeuf, Colin Ian King, LKML
On Mon, Nov 16, 2020 at 1:18 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 16 Nov 2020 13:10:57 -0800
> Sami Tolvanen <samitolvanen@google.com> wrote:
>
> > Clang doesn't warn about this as we're building a module, it just
> > generates a reference to a non-existing global "my_tramp" symbol,
> > because the one defined in inline assembly has a local binding:
> >
> > $ readelf --symbols --wide ftrace-direct.lto.o | grep my_tramp
> > 16: 0000000000000000 13 FUNC LOCAL DEFAULT 1 my_tramp
> > 33: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND my_tramp
> > 42: 0000000000000000 8 FUNC GLOBAL HIDDEN 8 my_tramp.cfi_jt
> >
> > This would prevent the module from loading, which modpost catches:
> >
> > ERROR: modpost: "my_tramp" [samples/ftrace/ftrace-direct.ko] undefined!
> >
> > > From user space, I'm just using the following file:
> >
> > As this error happens only with Control-Flow Integrity, we need to
> > take the address of the "test" function to force the compiler to
> > generate a jump table entry for it. Here's a slightly tweaked
> > stand-alone reproducer:
> >
> > https://godbolt.org/z/GnzjE4
>
> Thanks, we don't need to look more into this. It was mostly my curiosity to
> find a way to have the compiler know about a function declared statically
> in inline assembly. Maybe I'm asking for too much ;-)
>
> I'll take your original patch. Does it need to go to stable, or is this not
> that big of an issue to allow it to be added in the next merge window?
Thanks! This is definitely not a big issue, it just fixes the
allyesconfig build with CFI, and it's not needed in -stable.
Sami
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-16 21:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-13 18:34 [PATCH] samples/ftrace: mark my_tramp[12]? global Sami Tolvanen
2020-11-13 18:57 ` Kees Cook
2020-11-16 16:39 ` Steven Rostedt
2020-11-16 20:10 ` Sami Tolvanen
2020-11-16 20:38 ` Steven Rostedt
2020-11-16 21:10 ` Sami Tolvanen
2020-11-16 21:18 ` Steven Rostedt
2020-11-16 21:27 ` Sami Tolvanen
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).