PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
on those three architectures because LKDTM messes up function
descriptors with functions.
This series does some cleanup in the three architectures and
refactors function descriptors so that it can then easily use it
in a generic way in LKDTM.
Changes in v4:
- Added patch 1 which Fixes 'sparse' for powerpc64le after wrong report on previous series, refer https://github.com/ruscur/linux-ci/actions/runs/1351427671
- Exported dereference_function_descriptor() to modules
- Addressed other received comments
- Rebased on latest powerpc/next (5a72345e6a78120368fcc841b570331b6c5a50da)
Changes in v3:
- Addressed received comments
- Swapped some of the powerpc patches to keep func_descr_t renamed as struct func_desc and remove 'struct ppc64_opd_entry'
- Changed HAVE_FUNCTION_DESCRIPTORS macro to a config item CONFIG_HAVE_FUNCTION_DESCRIPTORS
- Dropped patch 11 ("Fix lkdtm_EXEC_RODATA()")
Changes in v2:
- Addressed received comments
- Moved dereference_[kernel]_function_descriptor() out of line
- Added patches to remove func_descr_t and func_desc_t in powerpc
- Using func_desc_t instead of funct_descr_t
- Renamed HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to HAVE_FUNCTION_DESCRIPTORS
- Added a new lkdtm test to check protection of function descriptors
Christophe Leroy (13):
powerpc: Fix 'sparse' checking on PPC64le
powerpc: Move and rename func_descr_t
powerpc: Use 'struct func_desc' instead of 'struct ppc64_opd_entry'
powerpc: Remove 'struct ppc64_opd_entry'
powerpc: Prepare func_desc_t for refactorisation
ia64: Rename 'ip' to 'addr' in 'struct fdesc'
asm-generic: Define CONFIG_HAVE_FUNCTION_DESCRIPTORS
asm-generic: Define 'func_desc_t' to commonly describe function
descriptors
asm-generic: Refactor dereference_[kernel]_function_descriptor()
lkdtm: Force do_nothing() out of line
lkdtm: Really write into kernel text in WRITE_KERN
lkdtm: Fix execute_[user]_location()
lkdtm: Add a test for function descriptors protection
arch/Kconfig | 3 +
arch/ia64/Kconfig | 1 +
arch/ia64/include/asm/elf.h | 2 +-
arch/ia64/include/asm/sections.h | 24 +-------
arch/ia64/kernel/module.c | 6 +-
arch/parisc/Kconfig | 1 +
arch/parisc/include/asm/sections.h | 16 ++----
arch/parisc/kernel/process.c | 21 -------
arch/powerpc/Kconfig | 1 +
arch/powerpc/Makefile | 2 +-
arch/powerpc/include/asm/code-patching.h | 2 +-
arch/powerpc/include/asm/elf.h | 6 ++
arch/powerpc/include/asm/sections.h | 29 ++--------
arch/powerpc/include/asm/types.h | 6 --
arch/powerpc/include/uapi/asm/elf.h | 8 ---
arch/powerpc/kernel/module_64.c | 42 ++++++--------
arch/powerpc/kernel/ptrace/ptrace.c | 6 ++
arch/powerpc/kernel/signal_64.c | 8 +--
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/lkdtm.h | 1 +
drivers/misc/lkdtm/perms.c | 71 +++++++++++++++++++-----
include/asm-generic/sections.h | 15 ++++-
include/linux/kallsyms.h | 2 +-
kernel/extable.c | 24 +++++++-
tools/testing/selftests/lkdtm/tests.txt | 1 +
25 files changed, 155 insertions(+), 144 deletions(-)
--
2.34.1
'sparse' is architecture agnostic and knows nothing about ELF ABI
version.
Just like it gets arch and powerpc type and endian from Makefile,
it also need to get _CALL_ELF from there, otherwise it won't set
PPC64_ELF_ABI_v2 macro for PPC64le and won't check the correct code.
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index ddc5a706760a..4d4d8175f4a1 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -213,7 +213,7 @@ CHECKFLAGS += -m$(BITS) -D__powerpc__ -D__powerpc$(BITS)__
ifdef CONFIG_CPU_BIG_ENDIAN
CHECKFLAGS += -D__BIG_ENDIAN__
else
-CHECKFLAGS += -D__LITTLE_ENDIAN__
+CHECKFLAGS += -D__LITTLE_ENDIAN__ -D_CALL_ELF=2
endif
ifdef CONFIG_476FPE_ERR46
--
2.34.1
dereference_function_descriptor() and
dereference_kernel_function_descriptor() are identical on the
three architectures implementing them.
Make them common and put them out-of-line in kernel/extable.c
which is one of the users and has similar type of functions.
Reviewed-by: Kees Cook <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
Acked-by: Helge Deller <[email protected]>
---
arch/ia64/include/asm/sections.h | 19 -------------------
arch/parisc/include/asm/sections.h | 9 ---------
arch/parisc/kernel/process.c | 21 ---------------------
arch/powerpc/include/asm/sections.h | 23 -----------------------
include/asm-generic/sections.h | 2 ++
kernel/extable.c | 23 ++++++++++++++++++++++-
6 files changed, 24 insertions(+), 73 deletions(-)
diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 3abe0562b01a..8e0875cf6071 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -30,23 +30,4 @@ extern char __start_gate_brl_fsys_bubble_down_patchlist[], __end_gate_brl_fsys_b
extern char __start_unwind[], __end_unwind[];
extern char __start_ivt_text[], __end_ivt_text[];
-#undef dereference_function_descriptor
-static inline void *dereference_function_descriptor(void *ptr)
-{
- struct fdesc *desc = ptr;
- void *p;
-
- if (!get_kernel_nofault(p, (void *)&desc->addr))
- ptr = p;
- return ptr;
-}
-
-#undef dereference_kernel_function_descriptor
-static inline void *dereference_kernel_function_descriptor(void *ptr)
-{
- if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
- return ptr;
- return dereference_function_descriptor(ptr);
-}
-
#endif /* _ASM_IA64_SECTIONS_H */
diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index ace1d4047a0b..33df42b5cc6d 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -12,13 +12,4 @@ typedef Elf64_Fdesc func_desc_t;
extern char __alt_instructions[], __alt_instructions_end[];
-#ifdef CONFIG_64BIT
-
-#undef dereference_function_descriptor
-void *dereference_function_descriptor(void *);
-
-#undef dereference_kernel_function_descriptor
-void *dereference_kernel_function_descriptor(void *);
-#endif
-
#endif
diff --git a/arch/parisc/kernel/process.c b/arch/parisc/kernel/process.c
index ea3d83b6fb62..2030c77592d3 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -263,27 +263,6 @@ __get_wchan(struct task_struct *p)
return 0;
}
-#ifdef CONFIG_64BIT
-void *dereference_function_descriptor(void *ptr)
-{
- Elf64_Fdesc *desc = ptr;
- void *p;
-
- if (!get_kernel_nofault(p, (void *)&desc->addr))
- ptr = p;
- return ptr;
-}
-
-void *dereference_kernel_function_descriptor(void *ptr)
-{
- if (ptr < (void *)__start_opd ||
- ptr >= (void *)__end_opd)
- return ptr;
-
- return dereference_function_descriptor(ptr);
-}
-#endif
-
static inline unsigned long brk_rnd(void)
{
return (get_random_int() & BRK_RND_MASK) << PAGE_SHIFT;
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index fddfb3937868..8be2c491c733 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -58,29 +58,6 @@ static inline int overlaps_kernel_text(unsigned long start, unsigned long end)
(unsigned long)_stext < end;
}
-#ifdef PPC64_ELF_ABI_v1
-
-#undef dereference_function_descriptor
-static inline void *dereference_function_descriptor(void *ptr)
-{
- struct func_desc *desc = ptr;
- void *p;
-
- if (!get_kernel_nofault(p, (void *)&desc->addr))
- ptr = p;
- return ptr;
-}
-
-#undef dereference_kernel_function_descriptor
-static inline void *dereference_kernel_function_descriptor(void *ptr)
-{
- if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
- return ptr;
-
- return dereference_function_descriptor(ptr);
-}
-#endif /* PPC64_ELF_ABI_v1 */
-
#endif
#endif /* __KERNEL__ */
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index bbf97502470c..d0f7bdd2fdf2 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -60,6 +60,8 @@ extern __visible const void __nosave_begin, __nosave_end;
/* Function descriptor handling (if any). Override in asm/sections.h */
#ifdef CONFIG_HAVE_FUNCTION_DESCRIPTORS
+void *dereference_function_descriptor(void *ptr);
+void *dereference_kernel_function_descriptor(void *ptr);
#else
#define dereference_function_descriptor(p) ((void *)(p))
#define dereference_kernel_function_descriptor(p) ((void *)(p))
diff --git a/kernel/extable.c b/kernel/extable.c
index b6f330f0fe74..394c39b86e38 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -3,6 +3,7 @@
Copyright (C) 2001 Rusty Russell, 2002 Rusty Russell IBM.
*/
+#include <linux/elf.h>
#include <linux/ftrace.h>
#include <linux/memory.h>
#include <linux/extable.h>
@@ -132,12 +133,32 @@ int kernel_text_address(unsigned long addr)
}
/*
- * On some architectures (PPC64, IA64) function pointers
+ * On some architectures (PPC64, IA64, PARISC) function pointers
* are actually only tokens to some data that then holds the
* real function address. As a result, to find if a function
* pointer is part of the kernel text, we need to do some
* special dereferencing first.
*/
+#ifdef CONFIG_HAVE_FUNCTION_DESCRIPTORS
+void *dereference_function_descriptor(void *ptr)
+{
+ func_desc_t *desc = ptr;
+ void *p;
+
+ if (!get_kernel_nofault(p, (void *)&desc->addr))
+ ptr = p;
+ return ptr;
+}
+
+void *dereference_kernel_function_descriptor(void *ptr)
+{
+ if (ptr < (void *)__start_opd || ptr >= (void *)__end_opd)
+ return ptr;
+
+ return dereference_function_descriptor(ptr);
+}
+#endif
+
int func_ptr_is_kernel_text(void *ptr)
{
unsigned long addr;
--
2.34.1
Add WRITE_OPD to check that you can't modify function
descriptors.
Gives the following result when function descriptors are
not protected:
lkdtm: Performing direct entry WRITE_OPD
lkdtm: attempting bad 16 bytes write at c00000000269b358
lkdtm: FAIL: survived bad write
lkdtm: do_nothing was hijacked!
Looks like a standard compiler barrier() is not enough to force
GCC to use the modified function descriptor. Had to add a fake empty
inline assembly to force GCC to reload the function descriptor.
Signed-off-by: Christophe Leroy <[email protected]>
Acked-by: Kees Cook <[email protected]>
---
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/lkdtm.h | 1 +
drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++
tools/testing/selftests/lkdtm/tests.txt | 1 +
4 files changed, 25 insertions(+)
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index f69b964b9952..e2228b6fc09b 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -149,6 +149,7 @@ static const struct crashtype crashtypes[] = {
CRASHTYPE(WRITE_RO),
CRASHTYPE(WRITE_RO_AFTER_INIT),
CRASHTYPE(WRITE_KERN),
+ CRASHTYPE(WRITE_OPD),
CRASHTYPE(REFCOUNT_INC_OVERFLOW),
CRASHTYPE(REFCOUNT_ADD_OVERFLOW),
CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index d6137c70ebbe..305fc2ec3f25 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -106,6 +106,7 @@ void __init lkdtm_perms_init(void);
void lkdtm_WRITE_RO(void);
void lkdtm_WRITE_RO_AFTER_INIT(void);
void lkdtm_WRITE_KERN(void);
+void lkdtm_WRITE_OPD(void);
void lkdtm_EXEC_DATA(void);
void lkdtm_EXEC_STACK(void);
void lkdtm_EXEC_KMALLOC(void);
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 1cf24c4a79e9..2c6aba3ff32b 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -44,6 +44,11 @@ static noinline void do_overwritten(void)
return;
}
+static noinline void do_almost_nothing(void)
+{
+ pr_info("do_nothing was hijacked!\n");
+}
+
static void *setup_function_descriptor(func_desc_t *fdesc, void *dst)
{
if (!have_function_descriptors())
@@ -144,6 +149,23 @@ void lkdtm_WRITE_KERN(void)
do_overwritten();
}
+void lkdtm_WRITE_OPD(void)
+{
+ size_t size = sizeof(func_desc_t);
+ void (*func)(void) = do_nothing;
+
+ if (!have_function_descriptors()) {
+ pr_info("XFAIL: Platform doesn't use function descriptors.\n");
+ return;
+ }
+ pr_info("attempting bad %zu bytes write at %px\n", size, do_nothing);
+ memcpy(do_nothing, do_almost_nothing, size);
+ pr_err("FAIL: survived bad write\n");
+
+ asm("" : "=m"(func));
+ func();
+}
+
void lkdtm_EXEC_DATA(void)
{
execute_location(data_area, CODE_WRITE);
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
index 6b36b7f5dcf9..243c781f0780 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -44,6 +44,7 @@ ACCESS_NULL
WRITE_RO
WRITE_RO_AFTER_INIT
WRITE_KERN
+WRITE_OPD
REFCOUNT_INC_OVERFLOW
REFCOUNT_ADD_OVERFLOW
REFCOUNT_INC_NOT_ZERO_OVERFLOW
--
2.34.1
'struct ppc64_opd_entry' doesn't belong to uapi/asm/elf.h
It was initially in module_64.c and commit 2d291e902791 ("Fix compile
failure with non modular builds") moved it into asm/elf.h
But it was by mistake added outside of __KERNEL__ section,
therefore commit c3617f72036c ("UAPI: (Scripted) Disintegrate
arch/powerpc/include/asm") moved it to uapi/asm/elf.h
Now that it is not used anymore by the kernel, remove it.
Reviewed-by: Kees Cook <[email protected]>
Reviewed-by: Nicholas Piggin <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/uapi/asm/elf.h | 8 --------
1 file changed, 8 deletions(-)
diff --git a/arch/powerpc/include/uapi/asm/elf.h b/arch/powerpc/include/uapi/asm/elf.h
index 860c59291bfc..308857123a08 100644
--- a/arch/powerpc/include/uapi/asm/elf.h
+++ b/arch/powerpc/include/uapi/asm/elf.h
@@ -289,12 +289,4 @@ typedef elf_fpreg_t elf_vsrreghalf_t32[ELF_NVSRHALFREG];
/* Keep this the last entry. */
#define R_PPC64_NUM 253
-/* There's actually a third entry here, but it's unused */
-struct ppc64_opd_entry
-{
- unsigned long funcaddr;
- unsigned long r2;
-};
-
-
#endif /* _UAPI_ASM_POWERPC_ELF_H */
--
2.34.1
We have three architectures using function descriptors, each with its
own type and name.
Add a common typedef that can be used in generic code.
Also add a stub typedef for architecture without function descriptors,
to avoid a forest of #ifdefs.
It replaces the similar 'func_desc_t' previously defined in
arch/powerpc/kernel/module_64.c
Reviewed-by: Kees Cook <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
Acked-by: Helge Deller <[email protected]>
---
arch/ia64/include/asm/sections.h | 3 +++
arch/parisc/include/asm/sections.h | 5 +++++
arch/powerpc/include/asm/sections.h | 4 ++++
arch/powerpc/kernel/module_64.c | 8 --------
include/asm-generic/sections.h | 5 +++++
5 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 2460d365a057..3abe0562b01a 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -9,6 +9,9 @@
#include <linux/elf.h>
#include <linux/uaccess.h>
+
+typedef struct fdesc func_desc_t;
+
#include <asm-generic/sections.h>
extern char __phys_per_cpu_start[];
diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index c8092e4d94de..ace1d4047a0b 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -2,6 +2,11 @@
#ifndef _PARISC_SECTIONS_H
#define _PARISC_SECTIONS_H
+#ifdef CONFIG_HAVE_FUNCTION_DESCRIPTORS
+#include <asm/elf.h>
+typedef Elf64_Fdesc func_desc_t;
+#endif
+
/* nothing to see, move along */
#include <asm-generic/sections.h>
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 7728a7a146c3..fddfb3937868 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -6,6 +6,10 @@
#include <linux/elf.h>
#include <linux/uaccess.h>
+#ifdef CONFIG_HAVE_FUNCTION_DESCRIPTORS
+typedef struct func_desc func_desc_t;
+#endif
+
#include <asm-generic/sections.h>
extern char __head_end[];
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ff93ef4cb5a2..9abc0e783e36 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -32,11 +32,6 @@
#ifdef PPC64_ELF_ABI_v2
-/* An address is simply the address of the function. */
-typedef struct {
- unsigned long addr;
-} func_desc_t;
-
static func_desc_t func_desc(unsigned long addr)
{
func_desc_t desc = {
@@ -61,9 +56,6 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
}
#else
-/* An address is address of the OPD entry, which contains address of fn. */
-typedef struct func_desc func_desc_t;
-
static func_desc_t func_desc(unsigned long addr)
{
return *(struct func_desc *)addr;
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 3ef83e1aebee..bbf97502470c 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -63,6 +63,11 @@ extern __visible const void __nosave_begin, __nosave_end;
#else
#define dereference_function_descriptor(p) ((void *)(p))
#define dereference_kernel_function_descriptor(p) ((void *)(p))
+
+/* An address is simply the address of the function. */
+typedef struct {
+ unsigned long addr;
+} func_desc_t;
#endif
static inline bool have_function_descriptors(void)
--
2.34.1
LKDTM tests display that the run do_nothing() at a given
address, but in reality do_nothing() is inlined into the
caller.
Force it out of line so that it really runs text at the
displayed address.
Acked-by: Kees Cook <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
---
drivers/misc/lkdtm/perms.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 2dede2ef658f..60b3b2fe929d 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -21,7 +21,7 @@
/* This is non-const, so it will end up in the .data section. */
static u8 data_area[EXEC_SIZE];
-/* This is cost, so it will end up in the .rodata section. */
+/* This is const, so it will end up in the .rodata section. */
static const unsigned long rodata = 0xAA55AA55;
/* This is marked __ro_after_init, so it should ultimately be .rodata. */
@@ -31,7 +31,7 @@ static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
* This just returns to the caller. It is designed to be copied into
* non-executable memory regions.
*/
-static void do_nothing(void)
+static noinline void do_nothing(void)
{
return;
}
--
2.34.1
On Tue, Feb 15, 2022 at 01:40:56PM +0100, Christophe Leroy wrote:
> 'sparse' is architecture agnostic and knows nothing about ELF ABI
> version.
>
> Just like it gets arch and powerpc type and endian from Makefile,
> it also need to get _CALL_ELF from there, otherwise it won't set
> PPC64_ELF_ABI_v2 macro for PPC64le and won't check the correct code.
>
> Signed-off-by: Christophe Leroy <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Tue, Feb 15, 2022 at 01:40:55PM +0100, Christophe Leroy wrote:
> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
> on those three architectures because LKDTM messes up function
> descriptors with functions.
>
> This series does some cleanup in the three architectures and
> refactors function descriptors so that it can then easily use it
> in a generic way in LKDTM.
Thanks for doing this! It looks good to me. :)
-Kees
>
> Changes in v4:
> - Added patch 1 which Fixes 'sparse' for powerpc64le after wrong report on previous series, refer https://github.com/ruscur/linux-ci/actions/runs/1351427671
> - Exported dereference_function_descriptor() to modules
> - Addressed other received comments
> - Rebased on latest powerpc/next (5a72345e6a78120368fcc841b570331b6c5a50da)
>
> Changes in v3:
> - Addressed received comments
> - Swapped some of the powerpc patches to keep func_descr_t renamed as struct func_desc and remove 'struct ppc64_opd_entry'
> - Changed HAVE_FUNCTION_DESCRIPTORS macro to a config item CONFIG_HAVE_FUNCTION_DESCRIPTORS
> - Dropped patch 11 ("Fix lkdtm_EXEC_RODATA()")
>
> Changes in v2:
> - Addressed received comments
> - Moved dereference_[kernel]_function_descriptor() out of line
> - Added patches to remove func_descr_t and func_desc_t in powerpc
> - Using func_desc_t instead of funct_descr_t
> - Renamed HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to HAVE_FUNCTION_DESCRIPTORS
> - Added a new lkdtm test to check protection of function descriptors
>
> Christophe Leroy (13):
> powerpc: Fix 'sparse' checking on PPC64le
> powerpc: Move and rename func_descr_t
> powerpc: Use 'struct func_desc' instead of 'struct ppc64_opd_entry'
> powerpc: Remove 'struct ppc64_opd_entry'
> powerpc: Prepare func_desc_t for refactorisation
> ia64: Rename 'ip' to 'addr' in 'struct fdesc'
> asm-generic: Define CONFIG_HAVE_FUNCTION_DESCRIPTORS
> asm-generic: Define 'func_desc_t' to commonly describe function
> descriptors
> asm-generic: Refactor dereference_[kernel]_function_descriptor()
> lkdtm: Force do_nothing() out of line
> lkdtm: Really write into kernel text in WRITE_KERN
> lkdtm: Fix execute_[user]_location()
> lkdtm: Add a test for function descriptors protection
>
> arch/Kconfig | 3 +
> arch/ia64/Kconfig | 1 +
> arch/ia64/include/asm/elf.h | 2 +-
> arch/ia64/include/asm/sections.h | 24 +-------
> arch/ia64/kernel/module.c | 6 +-
> arch/parisc/Kconfig | 1 +
> arch/parisc/include/asm/sections.h | 16 ++----
> arch/parisc/kernel/process.c | 21 -------
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/Makefile | 2 +-
> arch/powerpc/include/asm/code-patching.h | 2 +-
> arch/powerpc/include/asm/elf.h | 6 ++
> arch/powerpc/include/asm/sections.h | 29 ++--------
> arch/powerpc/include/asm/types.h | 6 --
> arch/powerpc/include/uapi/asm/elf.h | 8 ---
> arch/powerpc/kernel/module_64.c | 42 ++++++--------
> arch/powerpc/kernel/ptrace/ptrace.c | 6 ++
> arch/powerpc/kernel/signal_64.c | 8 +--
> drivers/misc/lkdtm/core.c | 1 +
> drivers/misc/lkdtm/lkdtm.h | 1 +
> drivers/misc/lkdtm/perms.c | 71 +++++++++++++++++++-----
> include/asm-generic/sections.h | 15 ++++-
> include/linux/kallsyms.h | 2 +-
> kernel/extable.c | 24 +++++++-
> tools/testing/selftests/lkdtm/tests.txt | 1 +
> 25 files changed, 155 insertions(+), 144 deletions(-)
>
> --
> 2.34.1
>
--
Kees Cook
execute_location() and execute_user_location() intent
to copy do_nothing() text and execute it at a new location.
However, at the time being it doesn't copy do_nothing() function
but do_nothing() function descriptor which still points to the
original text. So at the end it still executes do_nothing() at
its original location allthough using a copied function descriptor.
So, fix that by really copying do_nothing() text and build a new
function descriptor by copying do_nothing() function descriptor and
updating the target address with the new location.
Also fix the displayed addresses by dereferencing do_nothing()
function descriptor.
Signed-off-by: Christophe Leroy <[email protected]>
Acked-by: Kees Cook <[email protected]>
---
drivers/misc/lkdtm/perms.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 035fcca441f0..1cf24c4a79e9 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -44,19 +44,34 @@ static noinline void do_overwritten(void)
return;
}
+static void *setup_function_descriptor(func_desc_t *fdesc, void *dst)
+{
+ if (!have_function_descriptors())
+ return dst;
+
+ memcpy(fdesc, do_nothing, sizeof(*fdesc));
+ fdesc->addr = (unsigned long)dst;
+ barrier();
+
+ return fdesc;
+}
+
static noinline void execute_location(void *dst, bool write)
{
- void (*func)(void) = dst;
+ void (*func)(void);
+ func_desc_t fdesc;
+ void *do_nothing_text = dereference_function_descriptor(do_nothing);
- pr_info("attempting ok execution at %px\n", do_nothing);
+ pr_info("attempting ok execution at %px\n", do_nothing_text);
do_nothing();
if (write == CODE_WRITE) {
- memcpy(dst, do_nothing, EXEC_SIZE);
+ memcpy(dst, do_nothing_text, EXEC_SIZE);
flush_icache_range((unsigned long)dst,
(unsigned long)dst + EXEC_SIZE);
}
- pr_info("attempting bad execution at %px\n", func);
+ pr_info("attempting bad execution at %px\n", dst);
+ func = setup_function_descriptor(&fdesc, dst);
func();
pr_err("FAIL: func returned\n");
}
@@ -66,16 +81,19 @@ static void execute_user_location(void *dst)
int copied;
/* Intentionally crossing kernel/user memory boundary. */
- void (*func)(void) = dst;
+ void (*func)(void);
+ func_desc_t fdesc;
+ void *do_nothing_text = dereference_function_descriptor(do_nothing);
- pr_info("attempting ok execution at %px\n", do_nothing);
+ pr_info("attempting ok execution at %px\n", do_nothing_text);
do_nothing();
- copied = access_process_vm(current, (unsigned long)dst, do_nothing,
+ copied = access_process_vm(current, (unsigned long)dst, do_nothing_text,
EXEC_SIZE, FOLL_WRITE);
if (copied < EXEC_SIZE)
return;
- pr_info("attempting bad execution at %px\n", func);
+ pr_info("attempting bad execution at %px\n", dst);
+ func = setup_function_descriptor(&fdesc, dst);
func();
pr_err("FAIL: func returned\n");
}
@@ -153,7 +171,8 @@ void lkdtm_EXEC_VMALLOC(void)
void lkdtm_EXEC_RODATA(void)
{
- execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
+ execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing),
+ CODE_AS_IS);
}
void lkdtm_EXEC_USERSPACE(void)
--
2.34.1
There are three architectures with function descriptors, try to
have common names for the address they contain in order to
refactor some functions into generic functions later.
powerpc has 'entry'
ia64 has 'ip'
parisc has 'addr'
Vote for 'addr' and update 'struct fdesc' accordingly.
Reviewed-by: Kees Cook <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/ia64/include/asm/elf.h | 2 +-
arch/ia64/include/asm/sections.h | 2 +-
arch/ia64/kernel/module.c | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/ia64/include/asm/elf.h b/arch/ia64/include/asm/elf.h
index 6629301a2620..2ef5f9966ad1 100644
--- a/arch/ia64/include/asm/elf.h
+++ b/arch/ia64/include/asm/elf.h
@@ -226,7 +226,7 @@ struct got_entry {
* Layout of the Function Descriptor
*/
struct fdesc {
- uint64_t ip;
+ uint64_t addr;
uint64_t gp;
};
diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 3a033d2008b3..35f24e52149a 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -35,7 +35,7 @@ static inline void *dereference_function_descriptor(void *ptr)
struct fdesc *desc = ptr;
void *p;
- if (!get_kernel_nofault(p, (void *)&desc->ip))
+ if (!get_kernel_nofault(p, (void *)&desc->addr))
ptr = p;
return ptr;
}
diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 360f36b0eb3f..8f62cf97f691 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -602,15 +602,15 @@ get_fdesc (struct module *mod, uint64_t value, int *okp)
return value;
/* Look for existing function descriptor. */
- while (fdesc->ip) {
- if (fdesc->ip == value)
+ while (fdesc->addr) {
+ if (fdesc->addr == value)
return (uint64_t)fdesc;
if ((uint64_t) ++fdesc >= mod->arch.opd->sh_addr + mod->arch.opd->sh_size)
BUG();
}
/* Create new one */
- fdesc->ip = value;
+ fdesc->addr = value;
fdesc->gp = mod->arch.gp;
return (uint64_t) fdesc;
}
--
2.34.1
'struct ppc64_opd_entry' is somehow redundant with 'struct func_desc',
the later is more correct/complete as it includes the third
field which is unused.
So use 'struct func_desc' instead of 'struct ppc64_opd_entry'
Reviewed-by: Kees Cook <[email protected]>
Reviewed-by: Daniel Axtens <[email protected]>
Reviewed-by: Nicholas Piggin <[email protected]>
Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/sections.h | 4 ++--
arch/powerpc/kernel/module_64.c | 10 +++++-----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 38f79e42bf3c..baca39f4c6d3 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -61,10 +61,10 @@ static inline int overlaps_kernel_text(unsigned long start, unsigned long end)
#undef dereference_function_descriptor
static inline void *dereference_function_descriptor(void *ptr)
{
- struct ppc64_opd_entry *desc = ptr;
+ struct func_desc *desc = ptr;
void *p;
- if (!get_kernel_nofault(p, (void *)&desc->funcaddr))
+ if (!get_kernel_nofault(p, (void *)&desc->addr))
ptr = p;
return ptr;
}
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 6a45e6ddbe58..46e8eeb7c432 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -64,19 +64,19 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
#else
/* An address is address of the OPD entry, which contains address of fn. */
-typedef struct ppc64_opd_entry func_desc_t;
+typedef struct func_desc func_desc_t;
static func_desc_t func_desc(unsigned long addr)
{
- return *(struct ppc64_opd_entry *)addr;
+ return *(struct func_desc *)addr;
}
static unsigned long func_addr(unsigned long addr)
{
- return func_desc(addr).funcaddr;
+ return func_desc(addr).addr;
}
static unsigned long stub_func_addr(func_desc_t func)
{
- return func.funcaddr;
+ return func.addr;
}
static unsigned int local_entry_offset(const Elf64_Sym *sym)
{
@@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y)
static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
const Elf64_Shdr *sechdrs)
{
- /* One extra reloc so it's always 0-funcaddr terminated */
+ /* One extra reloc so it's always 0-addr terminated */
unsigned long relocs = 1;
unsigned i;
--
2.34.1
Hi!
On 2/15/22 13:40, Christophe Leroy wrote:
> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
> on those three architectures because LKDTM messes up function
> descriptors with functions.
>
> This series does some cleanup in the three architectures and
> refactors function descriptors so that it can then easily use it
> in a generic way in LKDTM.
I'll test the series on ia64 later this week. I have an Itanium box at
home for testing kernel patches.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Kees Cook <[email protected]> writes:
> On Tue, Feb 15, 2022 at 01:40:55PM +0100, Christophe Leroy wrote:
>> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
>> on those three architectures because LKDTM messes up function
>> descriptors with functions.
>>
>> This series does some cleanup in the three architectures and
>> refactors function descriptors so that it can then easily use it
>> in a generic way in LKDTM.
>
> Thanks for doing this! It looks good to me. :)
How should we merge this series, it's a bit all over the map.
I could put it in a topic branch?
cheers
On 2/15/22 17:07, Kees Cook wrote:
> On Tue, Feb 15, 2022 at 01:40:55PM +0100, Christophe Leroy wrote:
>> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
>> on those three architectures because LKDTM messes up function
>> descriptors with functions.
>>
>> This series does some cleanup in the three architectures and
>> refactors function descriptors so that it can then easily use it
>> in a generic way in LKDTM.
>
> Thanks for doing this! It looks good to me. :)
I endorse that.
Thank you, Christophe!
Acked-by: Helge Deller <[email protected]>
Helge
> -Kees
>
>>
>> Changes in v4:
>> - Added patch 1 which Fixes 'sparse' for powerpc64le after wrong report on previous series, refer https://github.com/ruscur/linux-ci/actions/runs/1351427671
>> - Exported dereference_function_descriptor() to modules
>> - Addressed other received comments
>> - Rebased on latest powerpc/next (5a72345e6a78120368fcc841b570331b6c5a50da)
>>
>> Changes in v3:
>> - Addressed received comments
>> - Swapped some of the powerpc patches to keep func_descr_t renamed as struct func_desc and remove 'struct ppc64_opd_entry'
>> - Changed HAVE_FUNCTION_DESCRIPTORS macro to a config item CONFIG_HAVE_FUNCTION_DESCRIPTORS
>> - Dropped patch 11 ("Fix lkdtm_EXEC_RODATA()")
>>
>> Changes in v2:
>> - Addressed received comments
>> - Moved dereference_[kernel]_function_descriptor() out of line
>> - Added patches to remove func_descr_t and func_desc_t in powerpc
>> - Using func_desc_t instead of funct_descr_t
>> - Renamed HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to HAVE_FUNCTION_DESCRIPTORS
>> - Added a new lkdtm test to check protection of function descriptors
>>
>> Christophe Leroy (13):
>> powerpc: Fix 'sparse' checking on PPC64le
>> powerpc: Move and rename func_descr_t
>> powerpc: Use 'struct func_desc' instead of 'struct ppc64_opd_entry'
>> powerpc: Remove 'struct ppc64_opd_entry'
>> powerpc: Prepare func_desc_t for refactorisation
>> ia64: Rename 'ip' to 'addr' in 'struct fdesc'
>> asm-generic: Define CONFIG_HAVE_FUNCTION_DESCRIPTORS
>> asm-generic: Define 'func_desc_t' to commonly describe function
>> descriptors
>> asm-generic: Refactor dereference_[kernel]_function_descriptor()
>> lkdtm: Force do_nothing() out of line
>> lkdtm: Really write into kernel text in WRITE_KERN
>> lkdtm: Fix execute_[user]_location()
>> lkdtm: Add a test for function descriptors protection
>>
>> arch/Kconfig | 3 +
>> arch/ia64/Kconfig | 1 +
>> arch/ia64/include/asm/elf.h | 2 +-
>> arch/ia64/include/asm/sections.h | 24 +-------
>> arch/ia64/kernel/module.c | 6 +-
>> arch/parisc/Kconfig | 1 +
>> arch/parisc/include/asm/sections.h | 16 ++----
>> arch/parisc/kernel/process.c | 21 -------
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/Makefile | 2 +-
>> arch/powerpc/include/asm/code-patching.h | 2 +-
>> arch/powerpc/include/asm/elf.h | 6 ++
>> arch/powerpc/include/asm/sections.h | 29 ++--------
>> arch/powerpc/include/asm/types.h | 6 --
>> arch/powerpc/include/uapi/asm/elf.h | 8 ---
>> arch/powerpc/kernel/module_64.c | 42 ++++++--------
>> arch/powerpc/kernel/ptrace/ptrace.c | 6 ++
>> arch/powerpc/kernel/signal_64.c | 8 +--
>> drivers/misc/lkdtm/core.c | 1 +
>> drivers/misc/lkdtm/lkdtm.h | 1 +
>> drivers/misc/lkdtm/perms.c | 71 +++++++++++++++++++-----
>> include/asm-generic/sections.h | 15 ++++-
>> include/linux/kallsyms.h | 2 +-
>> kernel/extable.c | 24 +++++++-
>> tools/testing/selftests/lkdtm/tests.txt | 1 +
>> 25 files changed, 155 insertions(+), 144 deletions(-)
>>
>> --
>> 2.34.1
>>
>
On Wed, Feb 16, 2022 at 11:22:33PM +1100, Michael Ellerman wrote:
> Kees Cook <[email protected]> writes:
> > On Tue, Feb 15, 2022 at 01:40:55PM +0100, Christophe Leroy wrote:
> >> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
> >> on those three architectures because LKDTM messes up function
> >> descriptors with functions.
> >>
> >> This series does some cleanup in the three architectures and
> >> refactors function descriptors so that it can then easily use it
> >> in a generic way in LKDTM.
> >
> > Thanks for doing this! It looks good to me. :)
>
> How should we merge this series, it's a bit all over the map.
>
> I could put it in a topic branch?
That's fine by me -- I had assumed it'd go via the ppc tree. But if
you'd rather I take it as a topic branch I can do that too.
--
Kees Cook
Hi!
On 2/16/22 13:25, John Paul Adrian Glaubitz wrote:
>> This series does some cleanup in the three architectures and
>> refactors function descriptors so that it can then easily use it
>> in a generic way in LKDTM.
>
> I'll test the series on ia64 later this week. I have an Itanium box at
> home for testing kernel patches.
Series applied on top of 038101e6b2cd5c55f888f85db42ea2ad3aecb4b6 and
successfully tested on my HP Integrity RX2600 server.
Tested-by: John Paul Adrian Glaubitz <[email protected]>
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On Tue, 15 Feb 2022 13:40:55 +0100, Christophe Leroy wrote:
> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
> on those three architectures because LKDTM messes up function
> descriptors with functions.
>
> This series does some cleanup in the three architectures and
> refactors function descriptors so that it can then easily use it
> in a generic way in LKDTM.
>
> [...]
Applied to powerpc/next.
[01/13] powerpc: Fix 'sparse' checking on PPC64le
https://git.kernel.org/powerpc/c/81df21de8fb45d3a55d41da9c7f5724797d51ce6
[02/13] powerpc: Move and rename func_descr_t
https://git.kernel.org/powerpc/c/5b23cb8cc6b0aab0535253cc2aa362572bab7072
[03/13] powerpc: Use 'struct func_desc' instead of 'struct ppc64_opd_entry'
https://git.kernel.org/powerpc/c/d3e32b997a4ca2e7be71cb770bcb2c000ee20b36
[04/13] powerpc: Remove 'struct ppc64_opd_entry'
https://git.kernel.org/powerpc/c/0a9c5ae279c963149df9a84588281d3d607f7a1f
[05/13] powerpc: Prepare func_desc_t for refactorisation
https://git.kernel.org/powerpc/c/2fd986377d546bedaf27e36554dc9090d272f15d
[06/13] ia64: Rename 'ip' to 'addr' in 'struct fdesc'
https://git.kernel.org/powerpc/c/41a88b45479da873bfc5d29ba1a545a780c5329a
[07/13] asm-generic: Define CONFIG_HAVE_FUNCTION_DESCRIPTORS
https://git.kernel.org/powerpc/c/a257cacc38718c83cee003487e03197f237f5c3f
[08/13] asm-generic: Define 'func_desc_t' to commonly describe function descriptors
https://git.kernel.org/powerpc/c/0dc690e4ef5b901e9d4b53520854fbd5c749e09d
[09/13] asm-generic: Refactor dereference_[kernel]_function_descriptor()
https://git.kernel.org/powerpc/c/e1478d8eaf27704db17a44dee4c53696ed01fc9c
[10/13] lkdtm: Force do_nothing() out of line
https://git.kernel.org/powerpc/c/69b420ed8fd3917ac7073256b4929aa246b6fe31
[11/13] lkdtm: Really write into kernel text in WRITE_KERN
https://git.kernel.org/powerpc/c/b64913394f123e819bffabc79a0e48f98e78dc5d
[12/13] lkdtm: Fix execute_[user]_location()
https://git.kernel.org/powerpc/c/72a86433049dcfe918886645ac3d19c1eaaa67ab
[13/13] lkdtm: Add a test for function descriptors protection
https://git.kernel.org/powerpc/c/5e5a6c5441654d1b9e576ce4ca8a1759e701079e
cheers
Michael Ellerman <[email protected]> writes:
> On Tue, 15 Feb 2022 13:40:55 +0100, Christophe Leroy wrote:
>> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
>> on those three architectures because LKDTM messes up function
>> descriptors with functions.
>>
>> This series does some cleanup in the three architectures and
>> refactors function descriptors so that it can then easily use it
>> in a generic way in LKDTM.
>>
>> [...]
>
> Applied to powerpc/next.
I also have it in an rc2-based topic branch if there are any merge
conflicts that people want to resolve, I don't see any in linux-next at
the moment though.
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/log/?h=topic/func-desc-lkdtm
cheers
On Wed, Mar 09, 2022 at 12:37:14PM +1100, Michael Ellerman wrote:
> Michael Ellerman <[email protected]> writes:
> > On Tue, 15 Feb 2022 13:40:55 +0100, Christophe Leroy wrote:
> >> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
> >> on those three architectures because LKDTM messes up function
> >> descriptors with functions.
> >>
> >> This series does some cleanup in the three architectures and
> >> refactors function descriptors so that it can then easily use it
> >> in a generic way in LKDTM.
> >>
> >> [...]
> >
> > Applied to powerpc/next.
>
> I also have it in an rc2-based topic branch if there are any merge
> conflicts that people want to resolve, I don't see any in linux-next at
> the moment though.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/log/?h=topic/func-desc-lkdtm
Thanks! I've got some core changes coming for lkdtm, but I'm waiting
until after the merge window to rebase them and get them into -next.
--
Kees Cook