Add support for Clang CFI

kernel/cfi.c new file +17 -0

+static void update_shadow(struct module *mod, unsigned long base_addr,
+		update_shadow_fn fn)
+{
+	struct cfi_shadow *prev;
+	struct cfi_shadow *next;
+	unsigned long min_addr, max_addr;
+
+	next = vmalloc(SHADOW_SIZE);
+
+	mutex_lock(&shadow_update_lock);
+	prev = rcu_dereference_protected(cfi_shadow,
+					 mutex_is_locked(&shadow_update_lock));
+
+	if (next) {
+		next->base = base_addr >> PAGE_SHIFT;
+		prepare_next_shadow(prev, next);
+

kernel/cfi.c new file +6 -0

+
+	if (prev) {
+		set_memory_rw((unsigned long)prev, SHADOW_PAGES);
+		vfree(prev);
+	}
+}

Quoted 22 lines · Sami Tolvanen

kernel/cfi.c new file +22 -0

+static void update_shadow(struct module *mod, unsigned long base_addr,
+             update_shadow_fn fn)
+{
+     struct cfi_shadow *prev;
+     struct cfi_shadow *next;
+     unsigned long min_addr, max_addr;
+
+     next = vmalloc(SHADOW_SIZE);
+
+     mutex_lock(&shadow_update_lock);
+     prev = rcu_dereference_protected(cfi_shadow,
+                                      mutex_is_locked(&shadow_update_lock));
+
+     if (next) {
+             next->base = base_addr >> PAGE_SHIFT;
+             prepare_next_shadow(prev, next);
+
+             min_addr = (unsigned long)mod->core_layout.base;
+             max_addr = min_addr + mod->core_layout.text_size;
+             fn(next, mod, min_addr & PAGE_MASK, max_addr & PAGE_MASK);
+
+             set_memory_ro((unsigned long)next, SHADOW_PAGES);

kernel/cfi.c line 214

Quoted 5 lines

kernel/cfi.c new file +5 -0

+     }
+
+     rcu_assign_pointer(cfi_shadow, next);
+     mutex_unlock(&shadow_update_lock);
+     synchronize_rcu_expedited();

expedited is BAD(tm), why is it required and why doesn't it have a comment?

Quoted 24 lines · Peter Zijlstra

Quoted 22 lines · Sami Tolvanen

kernel/cfi.c new file +22 -0

+static void update_shadow(struct module *mod, unsigned long base_addr,
+             update_shadow_fn fn)
+{
+     struct cfi_shadow *prev;
+     struct cfi_shadow *next;
+     unsigned long min_addr, max_addr;
+
+     next = vmalloc(SHADOW_SIZE);
+
+     mutex_lock(&shadow_update_lock);
+     prev = rcu_dereference_protected(cfi_shadow,
+                                      mutex_is_locked(&shadow_update_lock));
+
+     if (next) {
+             next->base = base_addr >> PAGE_SHIFT;
+             prepare_next_shadow(prev, next);
+
+             min_addr = (unsigned long)mod->core_layout.base;
+             max_addr = min_addr + mod->core_layout.text_size;
+             fn(next, mod, min_addr & PAGE_MASK, max_addr & PAGE_MASK);
+
+             set_memory_ro((unsigned long)next, SHADOW_PAGES);

Quoted 8 lines

kernel/cfi.c line 214

Quoted 5 lines

kernel/cfi.c new file +5 -0

+     }
+
+     rcu_assign_pointer(cfi_shadow, next);
+     mutex_unlock(&shadow_update_lock);
+     synchronize_rcu_expedited();

expedited is BAD(tm), why is it required and why doesn't it have a comment?

Ah, this uses synchronize_rcu_expedited() because we have a case where synchronize_rcu() hangs here with a specific SoC family after the vendor's cpu_pm driver powers down CPU cores.

Would you say expedited is bad enough that we should avoid it here? The function is called only when kernel modules are loaded or unloaded, so not very frequently.

Quoted 34 lines · Sami Tolvanen

Quoted 32 lines · Peter Zijlstra

Quoted 17 lines · Sami Tolvanen

kernel/cfi.c new file +17 -0

+static void update_shadow(struct module *mod, unsigned long base_addr,
+             update_shadow_fn fn)
+{
+     struct cfi_shadow *prev;
+     struct cfi_shadow *next;
+     unsigned long min_addr, max_addr;
+
+     next = vmalloc(SHADOW_SIZE);
+
+     mutex_lock(&shadow_update_lock);
+     prev = rcu_dereference_protected(cfi_shadow,
+                                      mutex_is_locked(&shadow_update_lock));
+
+     if (next) {
+             next->base = base_addr >> PAGE_SHIFT;
+             prepare_next_shadow(prev, next);
+

kernel/cfi.c line 214

Quoted 10 lines

kernel/cfi.c new file +10 -0

+             min_addr = (unsigned long)mod->core_layout.base;
+             max_addr = min_addr + mod->core_layout.text_size;
+             fn(next, mod, min_addr & PAGE_MASK, max_addr & PAGE_MASK);
+
+             set_memory_ro((unsigned long)next, SHADOW_PAGES);
+     }
+
+     rcu_assign_pointer(cfi_shadow, next);
+     mutex_unlock(&shadow_update_lock);
+     synchronize_rcu_expedited();

expedited is BAD(tm), why is it required and why doesn't it have a comment?

Quoted 3 lines

Ah, this uses synchronize_rcu_expedited() because we have a case where synchronize_rcu() hangs here with a specific SoC family after the vendor's cpu_pm driver powers down CPU cores.

Broken vendor drivers seem like an exceedingly poor reason for this.

Quoted 35 lines · Peter Zijlstra

Quoted 34 lines · Sami Tolvanen

Quoted 32 lines · Peter Zijlstra

Quoted 17 lines · Sami Tolvanen

kernel/cfi.c new file +17 -0

+static void update_shadow(struct module *mod, unsigned long base_addr,
+             update_shadow_fn fn)
+{
+     struct cfi_shadow *prev;
+     struct cfi_shadow *next;
+     unsigned long min_addr, max_addr;
+
+     next = vmalloc(SHADOW_SIZE);
+
+     mutex_lock(&shadow_update_lock);
+     prev = rcu_dereference_protected(cfi_shadow,
+                                      mutex_is_locked(&shadow_update_lock));
+
+     if (next) {
+             next->base = base_addr >> PAGE_SHIFT;
+             prepare_next_shadow(prev, next);
+

kernel/cfi.c line 214

Quoted 10 lines

kernel/cfi.c new file +10 -0

+             min_addr = (unsigned long)mod->core_layout.base;
+             max_addr = min_addr + mod->core_layout.text_size;
+             fn(next, mod, min_addr & PAGE_MASK, max_addr & PAGE_MASK);
+
+             set_memory_ro((unsigned long)next, SHADOW_PAGES);
+     }
+
+     rcu_assign_pointer(cfi_shadow, next);
+     mutex_unlock(&shadow_update_lock);
+     synchronize_rcu_expedited();

expedited is BAD(tm), why is it required and why doesn't it have a comment?

Quoted 5 lines

Quoted 3 lines

Ah, this uses synchronize_rcu_expedited() because we have a case where synchronize_rcu() hangs here with a specific SoC family after the vendor's cpu_pm driver powers down CPU cores.

Broken vendor drivers seem like an exceedingly poor reason for this.

The vendor is supposed to make sure that RCU sees the CPU cores as either deep idle or offline before powering them down. My guess is that the CPU is powered down, but RCU (and probably much else in the system) thinks that the CPU is still up and running. So I bet that you are seeing other issues as well.

I take it that the IPIs from synchronize_rcu_expedited() have the effect of momentarily powering up those CPUs?

Quoted 49 lines · Paul E. McKenney

Quoted 35 lines · Peter Zijlstra

Quoted 34 lines · Sami Tolvanen

Quoted 32 lines · Peter Zijlstra

Quoted 17 lines · Sami Tolvanen

kernel/cfi.c new file +17 -0

+static void update_shadow(struct module *mod, unsigned long base_addr,
+             update_shadow_fn fn)
+{
+     struct cfi_shadow *prev;
+     struct cfi_shadow *next;
+     unsigned long min_addr, max_addr;
+
+     next = vmalloc(SHADOW_SIZE);
+
+     mutex_lock(&shadow_update_lock);
+     prev = rcu_dereference_protected(cfi_shadow,
+                                      mutex_is_locked(&shadow_update_lock));
+
+     if (next) {
+             next->base = base_addr >> PAGE_SHIFT;
+             prepare_next_shadow(prev, next);
+

kernel/cfi.c line 214

Quoted 10 lines

kernel/cfi.c new file +10 -0

+             min_addr = (unsigned long)mod->core_layout.base;
+             max_addr = min_addr + mod->core_layout.text_size;
+             fn(next, mod, min_addr & PAGE_MASK, max_addr & PAGE_MASK);
+
+             set_memory_ro((unsigned long)next, SHADOW_PAGES);
+     }
+
+     rcu_assign_pointer(cfi_shadow, next);
+     mutex_unlock(&shadow_update_lock);
+     synchronize_rcu_expedited();

expedited is BAD(tm), why is it required and why doesn't it have a comment?

Quoted 5 lines

Quoted 3 lines

Ah, this uses synchronize_rcu_expedited() because we have a case where synchronize_rcu() hangs here with a specific SoC family after the vendor's cpu_pm driver powers down CPU cores.

Broken vendor drivers seem like an exceedingly poor reason for this.

The vendor is supposed to make sure that RCU sees the CPU cores as either deep idle or offline before powering them down. My guess is that the CPU is powered down, but RCU (and probably much else in the system) thinks that the CPU is still up and running. So I bet that you are seeing other issues as well.

Quoted 2 lines

I take it that the IPIs from synchronize_rcu_expedited() have the effect of momentarily powering up those CPUs?

I suspect you're correct. I'll change this to use synchronize_rcu() in v3.

With CONFIG_CFI_CLANG, the compiler replaces a function address taken in C code with the address of a local jump table entry, which passes runtime indirect call checks. However, the compiler won't replace addresses taken in assembly code, which will result in a CFI failure if we later jump to such an address in instrumented C code. The code generated for the non-canonical jump table looks this:

<noncanonical.cfi_jt>: /* In C, &noncanonical points here */
jmp noncanonical
...
<noncanonical>: /* function body */

        ...

This change adds the __cficanonical attribute, which tells the compiler to use a canonical jump table for the function instead. This means the compiler will rename the actual function to <function>.cfi and points the original symbol to the jump table entry instead:

<canonical>: /* jump table entry */
jmp canonical.cfi
...
<canonical.cfi>: /* function body */

        ...

As a result, the address taken in assembly, or other non-instrumented code always points to the jump table and therefore, can be used for indirect calls in instrumented code without tripping CFI checks.

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

Reviewed-by Kees Cook <[email protected]>

Acked-by Bjorn Helgaas <[email protected]> # pci.h

---
 include/linux/compiler-clang.h | 1 +
 include/linux/compiler_types.h | 4 ++++
 include/linux/init.h           | 4 ++--
 include/linux/pci.h            | 4 ++--
 4 files changed, 9 insertions(+), 4 deletions(-)

include/linux/compiler-clang.h +1 -0

index 6de9d0c9377e..adbe76b203e2 100644
@@ -63,3 +63,4 @@
 #endif
 
 #define __nocfi                __attribute__((__no_sanitize__("cfi")))
+#define __cficanonical __attribute__((__cfi_canonical_jump_table__))

include/linux/compiler_types.h +4 -0

index 796935a37e37..d29bda7f6ebd 100644
@@ -246,6 +246,10 @@ struct ftrace_likely_data {
 # define __nocfi
 #endif
 
+#ifndef __cficanonical
+# define __cficanonical
+#endif
+
 #ifndef asm_volatile_goto
 #define asm_volatile_goto(x...) asm goto(x)
 #endif

include/linux/init.h +2 -2

index b3ea15348fbd..045ad1650ed1 100644
@@ -220,8 +220,8 @@ extern bool initcall_debug;
        __initcall_name(initstub, __iid, id)
 
 #define __define_initcall_stub(__stub, fn)                     \
-       int __init __stub(void);                                \
-       int __init __stub(void)                                 \
+       int __init __cficanonical __stub(void);                 \
+       int __init __cficanonical __stub(void)                  \
        {                                                       \
                return fn();                                    \
        }                                                       \

include/linux/pci.h +2 -3

index 86c799c97b77..39684b72db91 100644
@@ -1944,8 +1944,8 @@ enum pci_fixup_pass {
 #ifdef CONFIG_LTO_CLANG
 #define __DECLARE_PCI_FIXUP_SECTION(sec, name, vendor, device, class,  \
                                  class_shift, hook, stub)              \
-       void stub(struct pci_dev *dev);                                 \
-       void stub(struct pci_dev *dev)                                  \
+       void __cficanonical stub(struct pci_dev *dev);                  \
+       void __cficanonical stub(struct pci_dev *dev)                   \
        {                                                               \
                hook(dev);                                              \
        }                                                               \
--

2.31.0.291.g576ba9dcdaf-goog

CONFIG_CFI_CLANG_SHADOW assumes the __cfi_check() function is page aligned and at the beginning of the .text section. While Clang would normally align the function correctly, it fails to do so for modules with no executable code.

This change ensures the correct __cfi_check() location and alignment. It also discards the .eh_frame section, which Clang can generate with certain sanitizers, such as CFI.

Link https://bugs.llvm.org/show_bug.cgi?id=46293

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

---
 scripts/module.lds.S | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

scripts/module.lds.S +3 -0

index 168cd27e6122..93518579cf5d 100644
@@ -3,10 +3,19 @@
  * Archs are free to supply their own linker scripts.  ld will
  * combine them automatically.
  */
+#include <asm/page.h>
+
+#ifdef CONFIG_CFI_CLANG

scripts/module.lds.S +8 -2

        }
 
        __ksymtab               0 : { *(SORT(___ksymtab+*)) }

@@ -40,7 +49,14 @@ SECTIONS {
                *(.rodata..L*)
        }
 
-       .text : { *(.text .text.[0-9a-zA-Z_]*) }
+       /*
+        * With CONFIG_CFI_CLANG, we assume __cfi_check is at the beginning
+        * of the .text section, and is aligned to PAGE_SIZE.
+        */
+       .text : ALIGN_CFI {
+               *(.text.__cfi_check)
+               *(.text .text.[0-9a-zA-Z_]* .text..L.cfi*)
+       }
 }
 
 /* bring in arch-specific sections */
--

2.31.0.291.g576ba9dcdaf-goog

Quoted 33 lines · Sami Tolvanen

CONFIG_CFI_CLANG_SHADOW assumes the __cfi_check() function is page aligned and at the beginning of the .text section. While Clang would normally align the function correctly, it fails to do so for modules with no executable code.

This change ensures the correct __cfi_check() location and alignment. It also discards the .eh_frame section, which Clang can generate with certain sanitizers, such as CFI.

Link https://bugs.llvm.org/show_bug.cgi?id=46293

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

---
 scripts/module.lds.S | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

scripts/module.lds.S +8 -0

index 168cd27e6122..93518579cf5d 100644
@@ -3,10 +3,19 @@
  * Archs are free to supply their own linker scripts.  ld will
  * combine them automatically.
  */
+#include <asm/page.h>
+
+#ifdef CONFIG_CFI_CLANG
+# define ALIGN_CFI ALIGN(PAGE_SIZE)
+#else
+# define ALIGN_CFI
+#endif
+
 SECTIONS {

scripts/module.lds.S line 18

Quoted 4 lines

scripts/module.lds.S +1 -0

        /DISCARD/ : {
                *(.discard)
                *(.discard.*)
+               *(.eh_frame)

Do we want to unconditionally discard this section from modules for all arches/configs? I like how we conditionally do so on SANITIZER_DISCARDS in include/asm-generic/vmlinux.lds.h for example.

With CONFIG_CFI_CLANG, a callback function passed to __queue_delayed_work from a module points to a jump table entry defined in the module instead of the one used in the core kernel, which breaks function address equality in this check:

  WARN_ON_ONCE(timer->function != delayed_work_timer_fn);

Use WARN_ON_FUNCTION_MISMATCH() instead to disable the warning when CFI and modules are both enabled.

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

---
 kernel/workqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/workqueue.c +1 -2

index 0d150da252e8..03fe07d2f39f 100644
@@ -1630,7 +1630,7 @@ static void __queue_delayed_work(int cpu, struct workqueue_struct *wq,
        struct work_struct *work = &dwork->work;
 
        WARN_ON_ONCE(!wq);
-       WARN_ON_ONCE(timer->function != delayed_work_timer_fn);
+       WARN_ON_FUNCTION_MISMATCH(timer->function, delayed_work_timer_fn);
        WARN_ON_ONCE(timer_pending(timer));
        WARN_ON_ONCE(!list_empty(&work->entry));
 
--

2.31.0.291.g576ba9dcdaf-goog

Quoted 6 lines · Sami Tolvanen

With CONFIG_CFI_CLANG, a callback function passed to __queue_delayed_work from a module points to a jump table entry defined in the module instead of the one used in the core kernel, which breaks function address equality in this check:

Quoted 4 lines

  WARN_ON_ONCE(timer->function != delayed_work_timer_fn);

Use WARN_ON_FUNCTION_MISMATCH() instead to disable the warning when CFI and modules are both enabled.

Does __cficanonical help with such comparisons? Or would that be a very invasive change, if the concern was to try to keep these checks in place for CONFIG_CFI_CLANG?

With CONFIG_CFI_CLANG and ThinLTO, Clang appends a hash to the names of all static functions not marked __used. This can break userspace tools that don't expect the function name to change, so strip out the hash from the output.

Suggested-by Jack Pham <[email protected]>

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

Reviewed-by Kees Cook <[email protected]>

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

kernel/kallsyms.c +0 -0

index 8043a90aa50e..17d3a704bafa 100644

kernel/kallsyms.c +4 -0

+ */
+static inline char *cleanup_symbol_name(char *s)
+{
+       char *res = NULL;

kernel/kallsyms.c +30 -6

+
 /* Lookup the address for this symbol. Returns 0 if not found. */
 unsigned long kallsyms_lookup_name(const char *name)
 {

@@ -173,6 +193,9 @@ unsigned long kallsyms_lookup_name(const char *name)
 
                if (strcmp(namebuf, name) == 0)
                        return kallsyms_sym_address(i);
+
+               if (cleanup_symbol_name(namebuf) && strcmp(namebuf, name) == 0)
+                       return kallsyms_sym_address(i);
        }
        return module_kallsyms_lookup_name(name);
 }

@@ -303,7 +326,9 @@ const char *kallsyms_lookup(unsigned long addr,
                                       namebuf, KSYM_NAME_LEN);
                if (modname)
                        *modname = NULL;
-               return namebuf;
+
+               ret = namebuf;
+               goto found;
        }
 
        /* See if it's in a module or a BPF JITed image. */

@@ -316,11 +341,16 @@ const char *kallsyms_lookup(unsigned long addr,
        if (!ret)
                ret = ftrace_mod_address_lookup(addr, symbolsize,
                                                offset, modname, namebuf);
+
+found:
+       cleanup_symbol_name(namebuf);
        return ret;
 }
 
 int lookup_symbol_name(unsigned long addr, char *symname)
 {
+       int res;
+
        symname[0] = '\0';
        symname[KSYM_NAME_LEN - 1] = '\0';
 

@@ -331,15 +361,23 @@ int lookup_symbol_name(unsigned long addr, char *symname)
                /* Grab name */
                kallsyms_expand_symbol(get_symbol_offset(pos),
                                       symname, KSYM_NAME_LEN);
-               return 0;
+               goto found;
        }
        /* See if it's in a module. */
-       return lookup_module_symbol_name(addr, symname);
+       res = lookup_module_symbol_name(addr, symname);
+       if (res)
+               return res;
+
+found:
+       cleanup_symbol_name(symname);
+       return 0;
 }
 
 int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
                        unsigned long *offset, char *modname, char *name)
 {
+       int res;
+
        name[0] = '\0';
        name[KSYM_NAME_LEN - 1] = '\0';
 

@@ -351,10 +389,16 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
                kallsyms_expand_symbol(get_symbol_offset(pos),
                                       name, KSYM_NAME_LEN);
                modname[0] = '\0';
-               return 0;
+               goto found;
        }
        /* See if it's in a module. */
-       return lookup_module_symbol_attrs(addr, size, offset, modname, name);
+       res = lookup_module_symbol_attrs(addr, size, offset, modname, name);
+       if (res)
+               return res;
+
+found:
+       cleanup_symbol_name(name);
+       return 0;
 }
 
 /* Look up a kernel symbol and return it in a text buffer. */
--

2.31.0.291.g576ba9dcdaf-goog

Quoted 21 lines · Sami Tolvanen

With CONFIG_CFI_CLANG and ThinLTO, Clang appends a hash to the names of all static functions not marked __used. This can break userspace tools that don't expect the function name to change, so strip out the hash from the output.

Suggested-by Jack Pham <[email protected]>

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

Reviewed-by Kees Cook <[email protected]>

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

kernel/kallsyms.c +0 -0

index 8043a90aa50e..17d3a704bafa 100644
@@ -161,6 +161,26 @@ static unsigned long kallsyms_sym_address(int idx)
        return kallsyms_relative_base - 1 - kallsyms_offsets[idx];
 }
 

kernel/kallsyms.c static unsigned long kallsyms_sym_address(int idx) · line 167

Quoted 4 lines

kernel/kallsyms.c +4 -0

+#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, which causes confusion and potentially breaks user space

Might be nice to add an example, something along the lines of:
ie. foo() becomes foo$asfdasdfasdfasdf()

kernel/kallsyms.c static unsigned long kallsyms_sym_address(int idx) · line 168

Quoted 2 lines

kernel/kallsyms.c +1 -0

+ * tools, so we will strip the postfix from expanded symbol names.

s/postfix/suffix/ ?

Quoted 11 lines

kernel/kallsyms.c +10 -0

+ */
+static inline char *cleanup_symbol_name(char *s)
+{
+       char *res = NULL;
+
+       res = strrchr(s, '$');
+       if (res)
+               *res = '\0';
+
+       return res;

kernel/kallsyms.c static unsigned long kallsyms_sym_address(int idx) · line 182

Quoted 4 lines

kernel/kallsyms.c +4 -0

+}
+#else
+static inline char *cleanup_symbol_name(char *s) { return NULL; }
+#endif

Might be nicer to return a bool and have the larger definition return res != NULL). Not sure what a caller would do with res if it was not NULL?

lib/list_sort.c +3 -4

                if (likely(bits)) {
                        struct list_head *a = *tail, *b = a->prev;
 
-                       a = merge(priv, (cmp_func)cmp, b, a);
+                       a = merge(priv, cmp, b, a);
                        /* Install the merged result in place of the inputs */
                        a->prev = b->prev;
                        *tail = a;

@@ -249,10 +249,10 @@ void list_sort(void *priv, struct list_head *head,
 
                if (!next)
                        break;
-               list = merge(priv, (cmp_func)cmp, pending, list);
+               list = merge(priv, cmp, pending, list);
                pending = next;
        }
        /* The final merge, rebuilding prev links */
-       merge_final(priv, (cmp_func)cmp, head, pending, list);
+       merge_final(priv, cmp, head, pending, list);
 }
 EXPORT_SYMBOL(list_sort);
--

2.31.0.291.g576ba9dcdaf-goog

lib/list_sort.c void list_sort(void *priv, struct list_head *head,

Quoted 26 lines · Sami Tolvanen

Casting the comparison function to a different type trips indirect call Control-Flow Integrity (CFI) checking. Remove the additional consts from cmp_func, and the now unneeded casts.

Fixes 043b3f7b6388 ("lib/list_sort: simplify and remove MAX_LIST_LENGTH_BITS")

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

Reviewed-by Kees Cook <[email protected]>

---
 lib/list_sort.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

lib/list_sort.c +1 -1

index 52f0c258c895..b14accf4ef83 100644
@@ -8,7 +8,7 @@
 #include <linux/list.h>
 
 typedef int __attribute__((nonnull(2,3))) (*cmp_func)(void *,
-               struct list_head const *, struct list_head const *);
+               struct list_head *, struct list_head *);
 
 /*
  * Returns a list organized in an intermediate format suited

@@ -227,7 +227,7 @@ void list_sort(void *priv, struct list_head *head,

There's definitely some const confusion going on around here. Comparison functions that modify their in/out parameters are a code smell, and I wonder if any exist in tree?

I think it would be better to enforce one signature for cmp_func throughout lib/list_sort.c and the tree, either const or not, but not a mix of both. I know const is messy because it tends to propagate everywhere, so I don't care which is preferred (making cmp_func have const qualified params or not, though if we're already being pedantic about which params are non-null...), but something like this might be nicer:

```

block/blk-mq-sched.c +2 -1

index 5132f64a5cee..d475b3cfd06f 100644
@@ -75,7 +75,8 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
        blk_mq_run_hw_queue(hctx, true);
 }
 
-static int sched_rq_cmp(void *priv, struct list_head *a, struct list_head *b)
+static int sched_rq_cmp(void *priv, const struct list_head *a,
+               const struct list_head *b)
 {
        struct request *rqa = container_of(a, struct request, queuelist);
        struct request *rqb = container_of(b, struct request, queuelist);

block/blk-mq.c +2 -1

index 2e825a7a3606..9ed063ffdb27 100644
@@ -1905,7 +1905,8 @@ void blk_mq_insert_requests(struct blk_mq_hw_ctx
 *hctx, struct blk_mq_ctx *ctx,
        spin_unlock(&ctx->lock);
 }
 
-static int plug_rq_cmp(void *priv, struct list_head *a, struct list_head *b)
+static int plug_rq_cmp(void *priv, const struct list_head *a,
+               const struct list_head *b)
 {
        struct request *rqa = container_of(a, struct request, queuelist);

        struct request *rqb = container_of(b, struct request, queuelist);

drivers/gpu/drm/drm_modes.c +2 -1

index 1ac67d4505e0..5a3a343499f6 100644
@@ -1290,7 +1290,8 @@ EXPORT_SYMBOL(drm_mode_prune_invalid);
  * Negative if @lh_a is better than @lh_b, zero if they're equivalent, or
  * positive if @lh_b is better than @lh_a.
  */
-static int drm_mode_compare(void *priv, struct list_head *lh_a,
 struct list_head *lh_b)
+static int drm_mode_compare(void *priv, const struct list_head *lh_a,
+               const struct list_head *lh_b)
 {
        struct drm_display_mode *a = list_entry(lh_a, struct

drm_display_mode, head);
struct drm_display_mode *b = list_entry(lh_b, struct
drm_display_mode, head);

drivers/gpu/drm/i915/gt/intel_engine_user.c +2 -1

index 34e6096f196e..7586dffd27d3 100644
@@ -49,7 +49,8 @@ static const u8 uabi_classes[] = {
        [VIDEO_ENHANCEMENT_CLASS] = I915_ENGINE_CLASS_VIDEO_ENHANCE,
 };
 
-static int engine_cmp(void *priv, struct list_head *A, struct list_head *B)
+static int engine_cmp(void *priv, const struct list_head *A,
+               const struct list_head *B)
 {
        const struct intel_engine_cs *a =
                container_of((struct rb_node *)A, typeof(*a), uabi_node);

fs/ext4/fsmap.c +2 -2

index 4c2a9fe30067..4493ef0c715e 100644
@@ -354,8 +354,8 @@ static unsigned int ext4_getfsmap_find_sb(struct
 super_block *sb,
 
 /* Compare two fsmap items. */
 static int ext4_getfsmap_compare(void *priv,
-                                struct list_head *a,
-                                struct list_head *b)
+                                const struct list_head *a,
+                                const struct list_head *b)
 {
        struct ext4_fsmap *fa;

        struct ext4_fsmap *fb;

fs/iomap/buffered-io.c +2 -1

index 414769a6ad11..0129e6bab985 100644
@@ -1155,7 +1155,8 @@ iomap_ioend_try_merge(struct iomap_ioend *ioend,
 struct list_head *more_ioends,
 EXPORT_SYMBOL_GPL(iomap_ioend_try_merge);
 
 static int
-iomap_ioend_compare(void *priv, struct list_head *a, struct list_head *b)
+iomap_ioend_compare(void *priv, const struct list_head *a,
+               const struct list_head *b)
 {
        struct iomap_ioend *ia = container_of(a, struct iomap_ioend, io_list);

        struct iomap_ioend *ib = container_of(b, struct iomap_ioend, io_list);

include/linux/list_sort.h +0 -0

index 20f178c24e9d..4fe9cb94d0d1 100644

include/linux/list_sort.h +1 -3

 __attribute__((nonnull(2,3)))
-void list_sort(void *priv, struct list_head *head,
-              int (*cmp)(void *priv, struct list_head *a,
-                         struct list_head *b));
+void list_sort(void *priv, struct list_head *head, cmp_func cmp);
 #endif

lib/list_sort.c +1 -6

index 52f0c258c895..6cfac649c4a6 100644
@@ -7,9 +7,6 @@
 #include <linux/list_sort.h>
 #include <linux/list.h>
 
-typedef int __attribute__((nonnull(2,3))) (*cmp_func)(void *,
-               struct list_head const *, struct list_head const *);
-
 /*
  * Returns a list organized in an intermediate format suited
  * to chaining of merge() calls: null-terminated, no reserved or

@@ -185,9 +182,7 @@ static void merge_final(void *priv, cmp_func cmp,
 struct list_head *head,
  * 2^(k+1) - 1 (second merge of case 5 when x == 2^(k-1) - 1).
  */
 __attribute__((nonnull(2,3)))
-void list_sort(void *priv, struct list_head *head,
-               int (*cmp)(void *priv, struct list_head *a,
-                       struct list_head *b))
+void list_sort(void *priv, struct list_head *head, cmp_func cmp)
 {
        struct list_head *list = head->next, *pending = NULL;

size_t count = 0; /* Count of pending */
```
There are probably more instances in the tree to clean up, but that compiles with x86_64 defconfig, and I'm sure it doesn't suffer the CFI issue from the cast.

To ensure we take the actual address of a function in kernel text, use __va_function. Otherwise, with CONFIG_CFI_CLANG, the compiler replaces the address with a pointer to the CFI jump table, which is actually in the module when compiled with CONFIG_LKDTM=m.

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

Acked-by Kees Cook <[email protected]>

---
 drivers/misc/lkdtm/usercopy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/misc/lkdtm/usercopy.c +1 -2

index 109e8d4302c1..d173d6175c87 100644
@@ -314,7 +314,7 @@ void lkdtm_USERCOPY_KERNEL(void)
 
        pr_info("attempting bad copy_to_user from kernel text: %px\n",
                vm_mmap);
-       if (copy_to_user((void __user *)user_addr, vm_mmap,
+       if (copy_to_user((void __user *)user_addr, __va_function(vm_mmap),
                         unconst + PAGE_SIZE)) {
                pr_warn("copy_to_user failed, but lacked Oops\n");
                goto free_user;
--

2.31.0.291.g576ba9dcdaf-goog

Quoted 5 lines · Sami Tolvanen

To ensure we take the actual address of a function in kernel text, use __va_function. Otherwise, with CONFIG_CFI_CLANG, the compiler replaces the address with a pointer to the CFI jump table, which is actually in the module when compiled with CONFIG_LKDTM=m.

Should patch 10 and 12 be reordered against one another? Otherwise it looks like 12 defines __va_function while 10 uses it?

Quoted 23 lines

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

Acked-by Kees Cook <[email protected]>

---
 drivers/misc/lkdtm/usercopy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/misc/lkdtm/usercopy.c +1 -2

index 109e8d4302c1..d173d6175c87 100644
@@ -314,7 +314,7 @@ void lkdtm_USERCOPY_KERNEL(void)
 
        pr_info("attempting bad copy_to_user from kernel text: %px\n",
                vm_mmap);
-       if (copy_to_user((void __user *)user_addr, vm_mmap,
+       if (copy_to_user((void __user *)user_addr, __va_function(vm_mmap),
                         unconst + PAGE_SIZE)) {
                pr_warn("copy_to_user failed, but lacked Oops\n");
                goto free_user;
--

Quoted 2 lines

2.31.0.291.g576ba9dcdaf-goog

--
Thanks,
~Nick Desaulniers