[PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt"

[PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt"

[Padmanabha Srinivasaiah]

Clang ThinLTO adds a postfix ".cfi_jt" to a symbols of extern functions.
For example this breaks syscall tracer that doesn't expect such postfix,
so strip out the postfix from the output.

Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
---
Change in v2:
  - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
    extern function name.
  - Modified the commit message accordingly

 kernel/kallsyms.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 0ba87982d017..e9148626ae6c 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -166,16 +166,20 @@ static unsigned long kallsyms_sym_address(int idx)
 
 #if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
 /*
- * LLVM appends a hash to static function names when ThinLTO and CFI are
- * both enabled, i.e. foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
- * This causes confusion and potentially breaks user space tools, so we
- * strip the suffix from expanded symbol names.
+ * LLVM appends a hash to static function names and just ".cfi_jt" postfix
+ * for non-static functions when both ThinLTO and CFI are enabled,
+ * i.e. for example foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
+ * This causes confusion and potentially breaks user space tools and
+ * built-in components, so we strip the suffix from expanded symbol names.
  */
 static inline bool cleanup_symbol_name(char *s)
 {
 	char *res;
 
 	res = strrchr(s, '$');
+	if (!res)
+		res = strstr(s, ".cfi_jt");
+
 	if (res)
 		*res = '\0';
 
-- 
2.17.1

Re: [PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt"

[Sami Tolvanen]

Hi,

On Tue, Jul 27, 2021 at 7:07 AM Padmanabha Srinivasaiah
<treasure4paddy@gmail.com> wrote:
>
> Clang ThinLTO adds a postfix ".cfi_jt" to a symbols of extern functions.

These symbols are added with CONFIG_CFI_CLANG no matter which LTO mode
is selected, so talking about ThinLTO here isn't quite correct.

> For example this breaks syscall tracer that doesn't expect such postfix,
> so strip out the postfix from the output.
>
> Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
> ---
> Change in v2:
>   - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
>     extern function name.
>   - Modified the commit message accordingly
>
>  kernel/kallsyms.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 0ba87982d017..e9148626ae6c 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -166,16 +166,20 @@ static unsigned long kallsyms_sym_address(int idx)
>
>  #if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
>  /*
> - * LLVM appends a hash to static function names when ThinLTO and CFI are
> - * both enabled, i.e. foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
> - * This causes confusion and potentially breaks user space tools, so we
> - * strip the suffix from expanded symbol names.
> + * LLVM appends a hash to static function names and just ".cfi_jt" postfix
> + * for non-static functions when both ThinLTO and CFI are enabled,

Functions aren't technically speaking renamed to add a .cfi_jt
postfix. Instead, these are separate symbols that point to the CFI
jump table. Perhaps the comment should just say that we want to strip
.cfi_jt from CFI jump table symbols?

> + * i.e. for example foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
> + * This causes confusion and potentially breaks user space tools and
> + * built-in components, so we strip the suffix from expanded symbol names.
>   */
>  static inline bool cleanup_symbol_name(char *s)
>  {
>         char *res;
>
>         res = strrchr(s, '$');
> +       if (!res)
> +               res = strstr(s, ".cfi_jt");
> +
>         if (res)
>                 *res = '\0';

This looks otherwise fine to me, but it's going to conflict with
Nick's earlier patch:

https://lore.kernel.org/lkml/20210707181814.365496-1-ndesaulniers@google.com/

Could you please rebase this on top of that, and take into account
that we should do this when CONFIG_LTO_CLANG is enabled, not only with
LTO_CLANG_THIN?

Sami

Re: [PATCH v2] kallsyms: strip ThinLTO postfix ".cfi_jt"

[Padmanabha Srinivasaiah]

On Wed, Jul 28, 2021 at 01:57:21PM -0700, Sami Tolvanen wrote:
> Hi,
> 
> On Tue, Jul 27, 2021 at 7:07 AM Padmanabha Srinivasaiah
> <treasure4paddy@gmail.com> wrote:
> >
> > Clang ThinLTO adds a postfix ".cfi_jt" to a symbols of extern functions.
> 
> These symbols are added with CONFIG_CFI_CLANG no matter which LTO mode
> is selected, so talking about ThinLTO here isn't quite correct.
>
Yes, checked irrespective of the LTO mode choosen ".cfi_jt" postfix
is added with CONFIG_CFI_CLANG flag. Thanks for correcting out,
will make neccessary changes.

> > For example this breaks syscall tracer that doesn't expect such postfix,
> > so strip out the postfix from the output.
> >
> > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
> > ---
> > Change in v2:
> >   - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
> >     extern function name.
> >   - Modified the commit message accordingly
> >
> >  kernel/kallsyms.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 0ba87982d017..e9148626ae6c 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -166,16 +166,20 @@ static unsigned long kallsyms_sym_address(int idx)
> >
> >  #if defined(CONFIG_CFI_CLANG) && defined(CONFIG_LTO_CLANG_THIN)
> >  /*
> > - * LLVM appends a hash to static function names when ThinLTO and CFI are
> > - * both enabled, i.e. foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
> > - * This causes confusion and potentially breaks user space tools, so we
> > - * strip the suffix from expanded symbol names.
> > + * LLVM appends a hash to static function names and just ".cfi_jt" postfix
> > + * for non-static functions when both ThinLTO and CFI are enabled,
> 
> Functions aren't technically speaking renamed to add a .cfi_jt
> postfix. Instead, these are separate symbols that point to the CFI
> jump table. Perhaps the comment should just say that we want to strip
> .cfi_jt from CFI jump table symbols?
> 

Agree, in jest modified existing comment. Will address same.

> > + * i.e. for example foo() becomes foo$707af9a22804d33c81801f27dcfe489b.
> > + * This causes confusion and potentially breaks user space tools and
> > + * built-in components, so we strip the suffix from expanded symbol names.
> >   */
> >  static inline bool cleanup_symbol_name(char *s)
> >  {
> >         char *res;
> >
> >         res = strrchr(s, '$');
> > +       if (!res)
> > +               res = strstr(s, ".cfi_jt");
> > +
> >         if (res)
> >                 *res = '\0';
> 
> This looks otherwise fine to me, but it's going to conflict with
> Nick's earlier patch:
> 
> https://lore.kernel.org/lkml/20210707181814.365496-1-ndesaulniers@google.com/
> 
> Could you please rebase this on top of that, and take into account
> that we should do this when CONFIG_LTO_CLANG is enabled, not only with
> LTO_CLANG_THIN?
>

Thanks Sami for pointing out the link, will rebase and refactor the change.

> Sami

[PATCH v3] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Padmanabha Srinivasaiah]

Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
For example this breaks syscall tracer that doesn't expect such postfix,
so strip out the postfix from the expanded symbol.

Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
---

Change in v3:
  - Modified commit message to indicate fix is for Clang CFI postfix
  - Rebased on recent patch from ndesaulniers@google.com.
  - Fix is enabled even for CONFIG_LTO_CLANG

Change in v2:
  - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
    extern function name.
  - Modified the commit message accordingly

 kernel/kallsyms.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 5cabe4dd3ff4..67d015854cbd 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -174,7 +174,8 @@ static bool cleanup_symbol_name(char *s)
 	 * foo.llvm.974640843467629774. This can break hooking of static
 	 * functions with kprobes.
 	 */
-	if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
+	if (!(IS_ENABLED(CONFIG_LTO_CLANG) ||
+	      IS_ENABLED(CONFIG_LTO_CLANG_THIN)))
 		return false;
 
 	res = strstr(s, ".llvm.");
@@ -184,16 +185,24 @@ static bool cleanup_symbol_name(char *s)
 	}
 
 	/*
-	 * LLVM appends a hash to static function names when ThinLTO and CFI
-	 * are both enabled, i.e. foo() becomes
-	 * foo$707af9a22804d33c81801f27dcfe489b. This causes confusion and
-	 * potentially breaks user space tools, so we strip the suffix from
-	 * expanded symbol names.
+	 * LLVM appends a hash to static function names when both
+	 * (Thin/FULL) LTO and CFI are enabled, i.e. foo() becomes
+	 * foo$707af9a22804d33c81801f27dcfe489b.
+	 *
+	 * In case of non static function symbol <funcsym>,
+	 * the local jump table will have entry as <funcsym>.cfi_jt.
+	 *
+	 * This causes confusion and potentially breaks
+	 * user space tools and some built-in components.
+	 * So we strip the suffix from expanded symbol names.
 	 */
 	if (!IS_ENABLED(CONFIG_CFI_CLANG))
 		return false;
 
 	res = strrchr(s, '$');
+	if (!res)
+		res = strstr(s, ".cfi_jt");
+
 	if (res) {
 		*res = '\0';
 		return true;
-- 
2.17.1

Re: [PATCH v3] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Sami Tolvanen]

Hi,

On Thu, Jul 29, 2021 at 1:54 PM Padmanabha Srinivasaiah
<treasure4paddy@gmail.com> wrote:
>
> Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
> For example this breaks syscall tracer that doesn't expect such postfix,
> so strip out the postfix from the expanded symbol.
>
> Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
> ---
>
> Change in v3:
>   - Modified commit message to indicate fix is for Clang CFI postfix
>   - Rebased on recent patch from ndesaulniers@google.com.
>   - Fix is enabled even for CONFIG_LTO_CLANG
>
> Change in v2:
>   - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
>     extern function name.
>   - Modified the commit message accordingly
>
>  kernel/kallsyms.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 5cabe4dd3ff4..67d015854cbd 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -174,7 +174,8 @@ static bool cleanup_symbol_name(char *s)
>          * foo.llvm.974640843467629774. This can break hooking of static
>          * functions with kprobes.
>          */
> -       if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
> +       if (!(IS_ENABLED(CONFIG_LTO_CLANG) ||
> +             IS_ENABLED(CONFIG_LTO_CLANG_THIN)))

This is redundant. LTO_CLANG is selected for both LTO modes, so
there's no need to also check for LTO_CLANG_THIN here.

>                 return false;
>
>         res = strstr(s, ".llvm.");

However, we should probably check for ".llvm." only with LTO_CLANG_THIN.

> @@ -184,16 +185,24 @@ static bool cleanup_symbol_name(char *s)
>         }
>
>         /*
> -        * LLVM appends a hash to static function names when ThinLTO and CFI
> -        * are both enabled, i.e. foo() becomes
> -        * foo$707af9a22804d33c81801f27dcfe489b. This causes confusion and
> -        * potentially breaks user space tools, so we strip the suffix from
> -        * expanded symbol names.
> +        * LLVM appends a hash to static function names when both
> +        * (Thin/FULL) LTO and CFI are enabled, i.e. foo() becomes
> +        * foo$707af9a22804d33c81801f27dcfe489b.

That's not quite right, the hash is only appended with ThinLTO. I
would leave this comment untouched.

> +        *
> +        * In case of non static function symbol <funcsym>,
> +        * the local jump table will have entry as <funcsym>.cfi_jt.
> +        *
> +        * This causes confusion and potentially breaks
> +        * user space tools and some built-in components.
> +        * So we strip the suffix from expanded symbol names.
>          */
>         if (!IS_ENABLED(CONFIG_CFI_CLANG))
>                 return false;
>
>         res = strrchr(s, '$');
> +       if (!res)
> +               res = strstr(s, ".cfi_jt");

And add a comment about stripping .cfi_jt from jump table symbols
before this part.

> +
>         if (res) {
>                 *res = '\0';
>                 return true;
> --
> 2.17.1
>

Sami

Re: [PATCH v3] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Padmanabha Srinivasaiah]

On Tue, Aug 03, 2021 at 09:28:23AM -0700, Sami Tolvanen wrote:
> Hi,
> 
> On Thu, Jul 29, 2021 at 1:54 PM Padmanabha Srinivasaiah
> <treasure4paddy@gmail.com> wrote:
> >
> > Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
> > For example this breaks syscall tracer that doesn't expect such postfix,
> > so strip out the postfix from the expanded symbol.
> >
> > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
> > ---
> >
> > Change in v3:
> >   - Modified commit message to indicate fix is for Clang CFI postfix
> >   - Rebased on recent patch from ndesaulniers@google.com.
> >   - Fix is enabled even for CONFIG_LTO_CLANG
> >
> > Change in v2:
> >   - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
> >     extern function name.
> >   - Modified the commit message accordingly
> >
> >  kernel/kallsyms.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 5cabe4dd3ff4..67d015854cbd 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -174,7 +174,8 @@ static bool cleanup_symbol_name(char *s)
> >          * foo.llvm.974640843467629774. This can break hooking of static
> >          * functions with kprobes.
> >          */
> > -       if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
> > +       if (!(IS_ENABLED(CONFIG_LTO_CLANG) ||
> > +             IS_ENABLED(CONFIG_LTO_CLANG_THIN)))
> 
> This is redundant. LTO_CLANG is selected for both LTO modes, so
> there's no need to also check for LTO_CLANG_THIN here.
>

As my setup is little endian, couldn't verify for below condition and 
was the reason to add such check. Sure will remove it.

       " select ARCH_SUPPORTS_LTO_CLANG if CPU_LITTLE_ENDIAN
	 select ARCH_SUPPORTS_LTO_CLANG_THIN"

> >                 return false;
> >
> >         res = strstr(s, ".llvm.");
> 
> However, we should probably check for ".llvm." only with LTO_CLANG_THIN.
>

Thank you for the input, will regenrate the patch with suggested check

> > @@ -184,16 +185,24 @@ static bool cleanup_symbol_name(char *s)
> >         }
> >
> >         /*
> > -        * LLVM appends a hash to static function names when ThinLTO and CFI
> > -        * are both enabled, i.e. foo() becomes
> > -        * foo$707af9a22804d33c81801f27dcfe489b. This causes confusion and
> > -        * potentially breaks user space tools, so we strip the suffix from
> > -        * expanded symbol names.
> > +        * LLVM appends a hash to static function names when both
> > +        * (Thin/FULL) LTO and CFI are enabled, i.e. foo() becomes
> > +        * foo$707af9a22804d33c81801f27dcfe489b.
> 
> That's not quite right, the hash is only appended with ThinLTO. I
> would leave this comment untouched.
>

sure, will revert it.

> > +        *
> > +        * In case of non static function symbol <funcsym>,
> > +        * the local jump table will have entry as <funcsym>.cfi_jt.
> > +        *
> > +        * This causes confusion and potentially breaks
> > +        * user space tools and some built-in components.
> > +        * So we strip the suffix from expanded symbol names.
> >          */
> >         if (!IS_ENABLED(CONFIG_CFI_CLANG))
> >                 return false;
> >
> >         res = strrchr(s, '$');
> > +       if (!res)
> > +               res = strstr(s, ".cfi_jt");
> 
> And add a comment about stripping .cfi_jt from jump table symbols
> before this part.
>

sure, will add it

> > +
> >         if (res) {
> >                 *res = '\0';
> >                 return true;
> > --
> > 2.17.1
> >
> 
> Sami

[PATCH v4] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Padmanabha Srinivasaiah]

Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
For e.g. this breaks syscall tracer that doesn't expect such postfix,
so strip out the postfix from the expanded symbol.

Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
---
Change in v4:
  - Remove redundant check; irrespective of LTO type (THIN/FULL),
    LTO_CLANG will be always enabled. Hence will be used as entry flag
    to check various postfix patterns.
  - And prior to stripping postfix ".cfi_jt", added a comment to
    justify why we are doing so.
 
Change in v3:
  - Modified commit message to indicate fix is for Clang CFI postfix
  - Rebased on recent patch from ndesaulniers@google.com.
    https://lore.kernel.org/lkml/
	20210707181814.365496-1-ndesaulniers@google.com/#t
  - Fix is enabled even for CONFIG_LTO_CLANG

Change in v2:
  - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
    extern function name.
  - Modified the commit message accordingly

 kernel/kallsyms.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 5cabe4dd3ff4..1b40bcf20fe6 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -174,13 +174,15 @@ static bool cleanup_symbol_name(char *s)
 	 * foo.llvm.974640843467629774. This can break hooking of static
 	 * functions with kprobes.
 	 */
-	if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
+	if (!IS_ENABLED(CONFIG_LTO_CLANG))
 		return false;
 
-	res = strstr(s, ".llvm.");
-	if (res) {
-		*res = '\0';
-		return true;
+	if (IS_ENABLED(CONFIG_LTO_CLANG_THIN)) {
+		res = strstr(s, ".llvm.");
+		if (res) {
+			*res = '\0';
+			return true;
+		}
 	}
 
 	/*
@@ -194,6 +196,17 @@ static bool cleanup_symbol_name(char *s)
 		return false;
 
 	res = strrchr(s, '$');
+	if (!res) {
+		/*
+		 * In case of non static function symbol <funcsym>,
+		 * the local jump table will have entry as <funcsym>.cfi_jt.
+		 *
+		 * Such expansion breaks some built-in components,
+		 * e.g. syscall tracer. Hence remove postfix ".cfi_jt".
+		 */
+		res = strstr(s, ".cfi_jt");
+	}
+
 	if (res) {
 		*res = '\0';
 		return true;
-- 
2.17.1

Re: [PATCH v4] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Sami Tolvanen]

On Thu, Aug 5, 2021 at 4:28 PM Padmanabha Srinivasaiah
<treasure4paddy@gmail.com> wrote:
>
> Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
> For e.g. this breaks syscall tracer that doesn't expect such postfix,
> so strip out the postfix from the expanded symbol.
>
> Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
> ---
> Change in v4:
>   - Remove redundant check; irrespective of LTO type (THIN/FULL),
>     LTO_CLANG will be always enabled. Hence will be used as entry flag
>     to check various postfix patterns.
>   - And prior to stripping postfix ".cfi_jt", added a comment to
>     justify why we are doing so.
>
> Change in v3:
>   - Modified commit message to indicate fix is for Clang CFI postfix
>   - Rebased on recent patch from ndesaulniers@google.com.
>     https://lore.kernel.org/lkml/
>         20210707181814.365496-1-ndesaulniers@google.com/#t
>   - Fix is enabled even for CONFIG_LTO_CLANG
>
> Change in v2:
>   - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
>     extern function name.
>   - Modified the commit message accordingly
>
>  kernel/kallsyms.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 5cabe4dd3ff4..1b40bcf20fe6 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -174,13 +174,15 @@ static bool cleanup_symbol_name(char *s)
>          * foo.llvm.974640843467629774. This can break hooking of static
>          * functions with kprobes.
>          */
> -       if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
> +       if (!IS_ENABLED(CONFIG_LTO_CLANG))
>                 return false;
>
> -       res = strstr(s, ".llvm.");
> -       if (res) {
> -               *res = '\0';
> -               return true;
> +       if (IS_ENABLED(CONFIG_LTO_CLANG_THIN)) {
> +               res = strstr(s, ".llvm.");
> +               if (res) {
> +                       *res = '\0';
> +                       return true;
> +               }
>         }

I confirmed that LLVM renames these also with full LTO, so the config
check can be dropped here.

>
>         /*
> @@ -194,6 +196,17 @@ static bool cleanup_symbol_name(char *s)
>                 return false;
>
>         res = strrchr(s, '$');
> +       if (!res) {
> +               /*
> +                * In case of non static function symbol <funcsym>,
> +                * the local jump table will have entry as <funcsym>.cfi_jt.
> +                *
> +                * Such expansion breaks some built-in components,
> +                * e.g. syscall tracer. Hence remove postfix ".cfi_jt".
> +                */
> +               res = strstr(s, ".cfi_jt");
> +       }
> +
>         if (res) {
>                 *res = '\0';
>                 return true;

Otherwise, the logic looks pretty good to me. Nick, are you planning
to resend your earlier patch? Should this be just folded into the next
version?

Sami

Re: [PATCH v4] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Padmanabha Srinivasaiah]

On Thu, Aug 12, 2021 at 04:05:25PM -0700, Sami Tolvanen wrote:
> On Thu, Aug 5, 2021 at 4:28 PM Padmanabha Srinivasaiah
> <treasure4paddy@gmail.com> wrote:
> >
> > Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
> > For e.g. this breaks syscall tracer that doesn't expect such postfix,
> > so strip out the postfix from the expanded symbol.
> >
> > Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
> > ---
> > Change in v4:
> >   - Remove redundant check; irrespective of LTO type (THIN/FULL),
> >     LTO_CLANG will be always enabled. Hence will be used as entry flag
> >     to check various postfix patterns.
> >   - And prior to stripping postfix ".cfi_jt", added a comment to
> >     justify why we are doing so.
> >
> > Change in v3:
> >   - Modified commit message to indicate fix is for Clang CFI postfix
> >   - Rebased on recent patch from ndesaulniers@google.com.
> >     https://lore.kernel.org/lkml/
> >         20210707181814.365496-1-ndesaulniers@google.com/#t
> >   - Fix is enabled even for CONFIG_LTO_CLANG
> >
> > Change in v2:
> >   - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
> >     extern function name.
> >   - Modified the commit message accordingly
> >
> >  kernel/kallsyms.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > index 5cabe4dd3ff4..1b40bcf20fe6 100644
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -174,13 +174,15 @@ static bool cleanup_symbol_name(char *s)
> >          * foo.llvm.974640843467629774. This can break hooking of static
> >          * functions with kprobes.
> >          */
> > -       if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
> > +       if (!IS_ENABLED(CONFIG_LTO_CLANG))
> >                 return false;
> >
> > -       res = strstr(s, ".llvm.");
> > -       if (res) {
> > -               *res = '\0';
> > -               return true;
> > +       if (IS_ENABLED(CONFIG_LTO_CLANG_THIN)) {
> > +               res = strstr(s, ".llvm.");
> > +               if (res) {
> > +                       *res = '\0';
> > +                       return true;
> > +               }
> >         }
> 
> I confirmed that LLVM renames these also with full LTO, so the config
> check can be dropped here.
>
Thank you sami for the input I missread your earlier review commit, will
re-genrate the patch

> >
> >         /*
> > @@ -194,6 +196,17 @@ static bool cleanup_symbol_name(char *s)
> >                 return false;
> >
> >         res = strrchr(s, '$');
> > +       if (!res) {
> > +               /*
> > +                * In case of non static function symbol <funcsym>,
> > +                * the local jump table will have entry as <funcsym>.cfi_jt.
> > +                *
> > +                * Such expansion breaks some built-in components,
> > +                * e.g. syscall tracer. Hence remove postfix ".cfi_jt".
> > +                */
> > +               res = strstr(s, ".cfi_jt");
> > +       }
> > +
> >         if (res) {
> >                 *res = '\0';
> >                 return true;
> 
> Otherwise, the logic looks pretty good to me. Nick, are you planning
> to resend your earlier patch? Should this be just folded into the next
> version?
> 
> Sami

[PATCH v5] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Padmanabha Srinivasaiah]

Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
For e.g. this breaks syscall tracer that doesn't expect such postfix,
so strip out the postfix from the expanded symbol.

Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
---
Change in v5:
  - Also remove explcit config check for postfix ".llvm." as LLVM
    renames even in full LTO case

Change in v4:
  - Remove redundant check; irrespective of LTO type (THIN/FULL),
    LTO_CLANG will be always enabled. Hence will be used as entry flag
    to check various postfix patterns.
  - And prior to stripping postfix ".cfi_jt", added a comment to
    justify why we are doing so.
 
Change in v3:
  - Modified commit message to indicate fix is for Clang CFI postfix
  - Rebased on recent patch from ndesaulniers@google.com.
    https://lore.kernel.org/lkml/
	20210707181814.365496-1-ndesaulniers@google.com/#t
  - Fix is enabled even for CONFIG_LTO_CLANG

Change in v2:
  - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
    extern function name.
  - Modified the commit message accordingly

 kernel/kallsyms.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 5cabe4dd3ff4..c8ef618e2a71 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -174,7 +174,7 @@ static bool cleanup_symbol_name(char *s)
 	 * foo.llvm.974640843467629774. This can break hooking of static
 	 * functions with kprobes.
 	 */
-	if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
+	if (!IS_ENABLED(CONFIG_LTO_CLANG))
 		return false;
 
 	res = strstr(s, ".llvm.");
@@ -194,6 +194,17 @@ static bool cleanup_symbol_name(char *s)
 		return false;
 
 	res = strrchr(s, '$');
+	if (!res) {
+		/*
+		 * In case of non static function symbol <funcsym>,
+		 * the local jump table will have entry as <funcsym>.cfi_jt.
+		 *
+		 * Such expansion breaks some built-in components,
+		 * e.g. syscall tracer. Hence remove postfix ".cfi_jt".
+		 */
+		res = strstr(s, ".cfi_jt");
+	}
+
 	if (res) {
 		*res = '\0';
 		return true;
-- 
2.17.1

Re: [PATCH v5] kallsyms: strip CLANG CFI postfix ".cfi_jt"

[Nick Desaulniers]

On Sat, Aug 14, 2021 at 5:43 AM Padmanabha Srinivasaiah
<treasure4paddy@gmail.com> wrote:
>
> Clang CFI adds a postfix ".cfi_jt" to a symbols of extern functions.
> For e.g. this breaks syscall tracer that doesn't expect such postfix,
> so strip out the postfix from the expanded symbol.

$ llvm-nm vmlinux | grep -e 'T ' -e 't '
...
ffff800010cce56c t xhci_map_urb_for_dma
ffff800010cce56c t xhci_map_urb_for_dma.86d975cb70058c10e8ae4c2960627264
ffff800011227f28 t xhci_map_urb_for_dma.86d975cb70058c10e8ae4c2960627264.cfi_jt
...

so I think it's not just the `.cfi_jt` that we want to truncate.  Sami
asked me about sending a v5 for
https://lore.kernel.org/lkml/20210707181814.365496-1-ndesaulniers@google.com/;
I was looking to rebase your v5 on my patch, but Sami also noted that
here https://lore.kernel.org/lkml/CABCJKue5Ay6_+8sibzh5wRh3gPzV1g72gJ9m2ot4E1ezj8bpHA@mail.gmail.com/
that the separator was changed from $ to . for other CFI symbols in
clang-13.

So I think I'm going to "combine" our patches to truncate after the
first `.` as long as CONFIG_LTO_CLANG is enabled, but still check for
`$` for clang-12 for CONFIG_CFI_CLANG.  I will credit you with the
Suggested-by tag; stay tuned.

>
> Signed-off-by: Padmanabha Srinivasaiah <treasure4paddy@gmail.com>
> ---
> Change in v5:
>   - Also remove explcit config check for postfix ".llvm." as LLVM
>     renames even in full LTO case
>
> Change in v4:
>   - Remove redundant check; irrespective of LTO type (THIN/FULL),
>     LTO_CLANG will be always enabled. Hence will be used as entry flag
>     to check various postfix patterns.
>   - And prior to stripping postfix ".cfi_jt", added a comment to
>     justify why we are doing so.
>
> Change in v3:
>   - Modified commit message to indicate fix is for Clang CFI postfix
>   - Rebased on recent patch from ndesaulniers@google.com.
>     https://lore.kernel.org/lkml/
>         20210707181814.365496-1-ndesaulniers@google.com/#t
>   - Fix is enabled even for CONFIG_LTO_CLANG
>
> Change in v2:
>   - Use existing routine in kallsyms to strip postfix ".cfi_jt" from
>     extern function name.
>   - Modified the commit message accordingly
>
>  kernel/kallsyms.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 5cabe4dd3ff4..c8ef618e2a71 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -174,7 +174,7 @@ static bool cleanup_symbol_name(char *s)
>          * foo.llvm.974640843467629774. This can break hooking of static
>          * functions with kprobes.
>          */
> -       if (!IS_ENABLED(CONFIG_LTO_CLANG_THIN))
> +       if (!IS_ENABLED(CONFIG_LTO_CLANG))
>                 return false;
>
>         res = strstr(s, ".llvm.");
> @@ -194,6 +194,17 @@ static bool cleanup_symbol_name(char *s)
>                 return false;
>
>         res = strrchr(s, '$');
> +       if (!res) {
> +               /*
> +                * In case of non static function symbol <funcsym>,
> +                * the local jump table will have entry as <funcsym>.cfi_jt.
> +                *
> +                * Such expansion breaks some built-in components,
> +                * e.g. syscall tracer. Hence remove postfix ".cfi_jt".
> +                */
> +               res = strstr(s, ".cfi_jt");
> +       }
> +
>         if (res) {
>                 *res = '\0';
>                 return true;
> --
> 2.17.1
>


-- 
Thanks,
~Nick Desaulniers