samples/ftrace: mark my_tramp[12]? global

PATCH In review Sami Tolvanen 3 participants 8 messages Nov 13, 2020 – Nov 16, 2020 ftrace

Share & tools

Sami Tolvanen

author

2020-11-13 18:34 UTC #0 lore raw

PATCH 1/1 samples/ftrace: mark my_tramp[12]? global

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 <[email protected]>

---
 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(+)

samples/ftrace/ftrace-direct-modify.c +2 -0

index c13a5bc5095b..5b9a09957c6e 100644
@@ -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"

samples/ftrace/ftrace-direct-too.c +1 -0

index d5c5022be664..3f0079c9bd6f 100644
@@ -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"

samples/ftrace/ftrace-direct.c +1 -0

index 63ca06d42c80..a2729d1ef17f 100644
@@ -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

Signature · 1 line

2.29.2.299.gdc1121823c-goog

Kees Cook

Reviewed-by on v1 1/1

2020-11-13 18:57 UTC #1 lore raw

Quoted 6 lines · 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 <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

Steven Rostedt

maintainer

2020-11-16 16:39 UTC #2 lore raw

In reply to Sami Tolvanen

Quoted 5 lines · On Fri, 13 Nov 2020 10:34:14 -0800 Sami Tolvanen <[email protected]>…

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

Quoted 53 lines

Signed-off-by Sami Tolvanen <[email protected]>

---
 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(+)

samples/ftrace/ftrace-direct-modify.c +2 -0

index c13a5bc5095b..5b9a09957c6e 100644
@@ -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"

samples/ftrace/ftrace-direct-too.c +1 -0

index d5c5022be664..3f0079c9bd6f 100644
@@ -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"

samples/ftrace/ftrace-direct.c +1 -0

index 63ca06d42c80..a2729d1ef17f 100644
@@ -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

Sami Tolvanen

author

2020-11-16 20:10 UTC #3 lore raw

Quoted 4 lines · Steven Rostedt

On Fri, 13 Nov 2020 10:34:14 -0800
Sami Tolvanen <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines
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.

Quoted 2 lines

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

Steven Rostedt

maintainer

2020-11-16 20:38 UTC #4 lore raw

Quoted 14 lines · On Mon, 16 Nov 2020 12:10:10 -0800 Sami Tolvanen <[email protected]>…

Quoted 4 lines · Steven Rostedt
On Fri, 13 Nov 2020 10:34:14 -0800
Sami Tolvanen <[email protected]> wrote:
Quoted 8 lines
Quoted 5 lines
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?

Quoted 8 lines

Unfortunately not, removing the "extern" doesn't seem to change anything.
Quoted 2 lines
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;
}

Sami Tolvanen

author

2020-11-16 21:11 UTC #5 lore raw

Quoted 23 lines · Steven Rostedt

Quoted 19 lines · On Mon, 16 Nov 2020 12:10:10 -0800 Sami Tolvanen <[email protected]>…
Quoted 4 lines · Steven Rostedt
On Fri, 13 Nov 2020 10:34:14 -0800
Sami Tolvanen <[email protected]> wrote:
Quoted 8 lines
Quoted 5 lines
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.
Quoted 2 lines
In other words, is there a way to tell C that a function is declared in an inline assembly block?

Quoted 6 lines

Quoted 3 lines
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!

Quoted 1 line

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

Steven Rostedt

maintainer

2020-11-16 21:18 UTC #6 lore raw

Quoted 13 lines · On Mon, 16 Nov 2020 13:10:57 -0800 Sami Tolvanen <[email protected]>…

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!

Quoted 8 lines

Quoted 1 line
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

Sami Tolvanen

author

2020-11-16 21:28 UTC #7 lore raw

Quoted 24 lines · Steven Rostedt

Quoted 20 lines · On Mon, 16 Nov 2020 13:10:57 -0800 Sami Tolvanen <[email protected]>…
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!
Quoted 1 line
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:

Quoted 8 lines

Quoted 1 line
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