[PATCH v1 00/10] Fix LKDTM for PPC64/IA64/PARISC

[PATCH v1 00/10] Fix LKDTM for PPC64/IA64/PARISC

[Christophe Leroy]

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.

Patch 6 is not absolutely necessary but it is a good trivial cleanup.

Christophe Leroy (10):
  powerpc: Move 'struct ppc64_opd_entry' back into asm/elf.h
  powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'
  ia64: Rename 'ip' to 'addr' in 'struct fdesc'
  asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define
    associated stubs
  asm-generic: Define 'funct_descr_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 lkdtm_EXEC_RODATA()
  lkdtm: Fix execute_[user]_location()

 arch/ia64/include/asm/elf.h         |  2 +-
 arch/ia64/include/asm/sections.h    | 24 ++---------
 arch/ia64/kernel/module.c           |  6 +--
 arch/parisc/include/asm/sections.h  | 16 +++----
 arch/parisc/kernel/process.c        | 21 ---------
 arch/powerpc/include/asm/elf.h      |  7 +++
 arch/powerpc/include/asm/sections.h | 30 +++----------
 arch/powerpc/include/uapi/asm/elf.h |  8 ----
 arch/powerpc/kernel/module_64.c     |  6 +--
 drivers/misc/lkdtm/perms.c          | 66 +++++++++++++++++++++++------
 include/asm-generic/sections.h      | 24 ++++++++++-
 11 files changed, 102 insertions(+), 108 deletions(-)

-- 
2.31.1

[PATCH v1 01/10] powerpc: Move 'struct ppc64_opd_entry' back into asm/elf.h

[Christophe Leroy]

'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

Move it back into asm/elf.h, this brings it back in line with
IA64 and PARISC architectures.

Fixes: 2d291e902791 ("Fix compile failure with non modular builds")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/elf.h      | 7 +++++++
 arch/powerpc/include/uapi/asm/elf.h | 8 --------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
index b8425e3cfd81..64b523848cd7 100644
--- a/arch/powerpc/include/asm/elf.h
+++ b/arch/powerpc/include/asm/elf.h
@@ -176,4 +176,11 @@ do {									\
 /* Relocate the kernel image to @final_address */
 void relocate(unsigned long final_address);
 
+/* There's actually a third entry here, but it's unused */
+struct ppc64_opd_entry
+{
+	unsigned long funcaddr;
+	unsigned long r2;
+};
+
 #endif /* _ASM_POWERPC_ELF_H */
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.31.1

Re: [PATCH v1 01/10] powerpc: Move 'struct ppc64_opd_entry' back into asm/elf.h

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 'struct ppc64_opd_entry' doesn't belong to uapi/asm/elf.h

Agree, but ...

> 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

... it's been visible to userspace since the first commit moved it, ~13
years ago in 2008, v2.6.27.

> Move it back into asm/elf.h, this brings it back in line with
> IA64 and PARISC architectures.

Removing it from the uapi header risks breaking userspace, I doubt
anything uses it, but who knows.

Given how long it's been there I think it's a bit risky to remove it :/

> diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
> index b8425e3cfd81..64b523848cd7 100644
> --- a/arch/powerpc/include/asm/elf.h
> +++ b/arch/powerpc/include/asm/elf.h
> @@ -176,4 +176,11 @@ do {									\
>  /* Relocate the kernel image to @final_address */
>  void relocate(unsigned long final_address);
>  
> +/* There's actually a third entry here, but it's unused */
> +struct ppc64_opd_entry
> +{
> +	unsigned long funcaddr;
> +	unsigned long r2;
> +};
> +
>  #endif /* _ASM_POWERPC_ELF_H */
> 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;
> -};

Rather than removing it we can make it uapi only with:

#ifndef __KERNEL__
/* There's actually a third entry here, but it's unused */
struct ppc64_opd_entry
{
	unsigned long funcaddr;
	unsigned long r2;
};
#endif


And then we can do whatever we want with the kernel internal version.

cheers

Re: [PATCH v1 01/10] powerpc: Move 'struct ppc64_opd_entry' back into asm/elf.h

[Arnd Bergmann]

On Tue, Oct 12, 2021 at 9:10 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> > 'struct ppc64_opd_entry' doesn't belong to uapi/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
>
> ... it's been visible to userspace since the first commit moved it, ~13
> years ago in 2008, v2.6.27.
>
> > Move it back into asm/elf.h, this brings it back in line with
> > IA64 and PARISC architectures.
>
> Removing it from the uapi header risks breaking userspace, I doubt
> anything uses it, but who knows.
>
> Given how long it's been there I think it's a bit risky to remove it :/

I would not be too worried about it. While we should absolutely
never break existing binaries, changing the visibility of internal
structures in header files only breaks compiling applications
that do rely on these entries, and they really should not be using
this in the first place.

        Arnd

Re: [PATCH v1 01/10] powerpc: Move 'struct ppc64_opd_entry' back into asm/elf.h

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:28PM +0200, Christophe Leroy wrote:
> '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
> 
> Move it back into asm/elf.h, this brings it back in line with
> IA64 and PARISC architectures.
> 
> Fixes: 2d291e902791 ("Fix compile failure with non modular builds")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

I'd agree with Arnd: this is a reasonable cleanup and nothing should be
using it.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH v1 02/10] powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'

[Christophe Leroy]

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 'funcaddr'
ia64 has 'ip'
parisc has 'addr'

Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/elf.h      | 2 +-
 arch/powerpc/include/asm/sections.h | 2 +-
 arch/powerpc/kernel/module_64.c     | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
index 64b523848cd7..90c3259a81ab 100644
--- a/arch/powerpc/include/asm/elf.h
+++ b/arch/powerpc/include/asm/elf.h
@@ -179,7 +179,7 @@ void relocate(unsigned long final_address);
 /* There's actually a third entry here, but it's unused */
 struct ppc64_opd_entry
 {
-	unsigned long funcaddr;
+	unsigned long addr;
 	unsigned long r2;
 };
 
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 6e4af4492a14..32e7035863ac 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -77,7 +77,7 @@ static inline void *dereference_function_descriptor(void *ptr)
 	struct ppc64_opd_entry *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 6baa676e7cb6..82908c9be627 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long 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.31.1

Re: [PATCH v1 02/10] powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:29PM +0200, Christophe Leroy wrote:
> 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 'funcaddr'
> ia64 has 'ip'
> parisc has 'addr'
> 
> Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Reasonable. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH v1 03/10] ia64: Rename 'ip' to 'addr' in 'struct fdesc'

[Christophe Leroy]

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 'funcaddr'
ia64 has 'ip'
parisc has 'addr'

Vote for 'addr' and update 'struct fdesc' accordingly.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 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 2cba53c1da82..4f6400cbf79e 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.31.1

Re: [PATCH v1 03/10] ia64: Rename 'ip' to 'addr' in 'struct fdesc'

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:30PM +0200, Christophe Leroy wrote:
> 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 'funcaddr'
> ia64 has 'ip'
> parisc has 'addr'
> 
> Vote for 'addr' and update 'struct fdesc' accordingly.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs

[Christophe Leroy]

Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of 'dereference_function_descriptor'
to know whether arch has function descriptors.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/ia64/include/asm/sections.h    | 4 ++--
 arch/parisc/include/asm/sections.h  | 6 ++++--
 arch/powerpc/include/asm/sections.h | 6 ++++--
 include/asm-generic/sections.h      | 3 ++-
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 35f24e52149a..80f5868afb06 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -7,6 +7,8 @@
  *	David Mosberger-Tang <davidm@hpl.hp.com>
  */
 
+#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+
 #include <linux/elf.h>
 #include <linux/uaccess.h>
 #include <asm-generic/sections.h>
@@ -27,8 +29,6 @@ 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[];
 
-#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
-
 #undef dereference_function_descriptor
 static inline void *dereference_function_descriptor(void *ptr)
 {
diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index bb52aea0cb21..2e781ee19b66 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -2,6 +2,10 @@
 #ifndef _PARISC_SECTIONS_H
 #define _PARISC_SECTIONS_H
 
+#ifdef CONFIG_64BIT
+#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+#endif
+
 /* nothing to see, move along */
 #include <asm-generic/sections.h>
 
@@ -9,8 +13,6 @@ extern char __alt_instructions[], __alt_instructions_end[];
 
 #ifdef CONFIG_64BIT
 
-#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
-
 #undef dereference_function_descriptor
 void *dereference_function_descriptor(void *);
 
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 32e7035863ac..b7f1ba04e756 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -8,6 +8,10 @@
 
 #define arch_is_kernel_initmem_freed arch_is_kernel_initmem_freed
 
+#ifdef PPC64_ELF_ABI_v1
+#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+#endif
+
 #include <asm-generic/sections.h>
 
 extern bool init_mem_is_free;
@@ -69,8 +73,6 @@ static inline int overlaps_kernel_text(unsigned long start, unsigned long end)
 
 #ifdef PPC64_ELF_ABI_v1
 
-#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
-
 #undef dereference_function_descriptor
 static inline void *dereference_function_descriptor(void *ptr)
 {
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index d16302d3eb59..1db5cfd69817 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -59,7 +59,8 @@ extern char __noinstr_text_start[], __noinstr_text_end[];
 extern __visible const void __nosave_begin, __nosave_end;
 
 /* Function descriptor handling (if any).  Override in asm/sections.h */
-#ifndef dereference_function_descriptor
+#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
+#else
 #define dereference_function_descriptor(p) ((void *)(p))
 #define dereference_kernel_function_descriptor(p) ((void *)(p))
 #endif
-- 
2.31.1

Re: [PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs

[Helge Deller]

On 10/11/21 17:25, Christophe Leroy wrote:
> Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of 'dereference_function_descriptor'
> to know whether arch has function descriptors.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/ia64/include/asm/sections.h    | 4 ++--
>  arch/parisc/include/asm/sections.h  | 6 ++++--
>  arch/powerpc/include/asm/sections.h | 6 ++++--
>  include/asm-generic/sections.h      | 3 ++-
>  4 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
> index 35f24e52149a..80f5868afb06 100644
> --- a/arch/ia64/include/asm/sections.h
> +++ b/arch/ia64/include/asm/sections.h
> @@ -7,6 +7,8 @@
>   *	David Mosberger-Tang <davidm@hpl.hp.com>
>   */
>
> +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +
>  #include <linux/elf.h>
>  #include <linux/uaccess.h>
>  #include <asm-generic/sections.h>
> @@ -27,8 +29,6 @@ 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[];
>
> -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> -
>  #undef dereference_function_descriptor
>  static inline void *dereference_function_descriptor(void *ptr)
>  {
> diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
> index bb52aea0cb21..2e781ee19b66 100644
> --- a/arch/parisc/include/asm/sections.h
> +++ b/arch/parisc/include/asm/sections.h
> @@ -2,6 +2,10 @@
>  #ifndef _PARISC_SECTIONS_H
>  #define _PARISC_SECTIONS_H
>
> +#ifdef CONFIG_64BIT
> +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +#endif
> +
>  /* nothing to see, move along */
>  #include <asm-generic/sections.h>
>
> @@ -9,8 +13,6 @@ extern char __alt_instructions[], __alt_instructions_end[];
>
>  #ifdef CONFIG_64BIT
>
> -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> -
>  #undef dereference_function_descriptor
>  void *dereference_function_descriptor(void *);
>
> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
> index 32e7035863ac..b7f1ba04e756 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -8,6 +8,10 @@
>
>  #define arch_is_kernel_initmem_freed arch_is_kernel_initmem_freed
>
> +#ifdef PPC64_ELF_ABI_v1
> +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +#endif
> +
>  #include <asm-generic/sections.h>
>
>  extern bool init_mem_is_free;
> @@ -69,8 +73,6 @@ static inline int overlaps_kernel_text(unsigned long start, unsigned long end)
>
>  #ifdef PPC64_ELF_ABI_v1
>
> -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> -
>  #undef dereference_function_descriptor
>  static inline void *dereference_function_descriptor(void *ptr)
>  {
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index d16302d3eb59..1db5cfd69817 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -59,7 +59,8 @@ extern char __noinstr_text_start[], __noinstr_text_end[];
>  extern __visible const void __nosave_begin, __nosave_end;
>
>  /* Function descriptor handling (if any).  Override in asm/sections.h */
> -#ifndef dereference_function_descriptor
> +#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> +#else

why not
#ifndef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
instead of #if/#else ?

>  #define dereference_function_descriptor(p) ((void *)(p))
>  #define dereference_kernel_function_descriptor(p) ((void *)(p))
>  #endif
>

Re: [PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs

[Christophe Leroy]

Le 12/10/2021 à 08:02, Helge Deller a écrit :
> On 10/11/21 17:25, Christophe Leroy wrote:
>> Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of 'dereference_function_descriptor'
>> to know whether arch has function descriptors.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/ia64/include/asm/sections.h    | 4 ++--
>>   arch/parisc/include/asm/sections.h  | 6 ++++--
>>   arch/powerpc/include/asm/sections.h | 6 ++++--
>>   include/asm-generic/sections.h      | 3 ++-
>>   4 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
>> index 35f24e52149a..80f5868afb06 100644
>> --- a/arch/ia64/include/asm/sections.h
>> +++ b/arch/ia64/include/asm/sections.h
>> @@ -7,6 +7,8 @@
>>    *	David Mosberger-Tang <davidm@hpl.hp.com>
>>    */
>>
>> +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +
>>   #include <linux/elf.h>
>>   #include <linux/uaccess.h>
>>   #include <asm-generic/sections.h>
>> @@ -27,8 +29,6 @@ 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[];
>>
>> -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> -
>>   #undef dereference_function_descriptor
>>   static inline void *dereference_function_descriptor(void *ptr)
>>   {
>> diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
>> index bb52aea0cb21..2e781ee19b66 100644
>> --- a/arch/parisc/include/asm/sections.h
>> +++ b/arch/parisc/include/asm/sections.h
>> @@ -2,6 +2,10 @@
>>   #ifndef _PARISC_SECTIONS_H
>>   #define _PARISC_SECTIONS_H
>>
>> +#ifdef CONFIG_64BIT
>> +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +#endif
>> +
>>   /* nothing to see, move along */
>>   #include <asm-generic/sections.h>
>>
>> @@ -9,8 +13,6 @@ extern char __alt_instructions[], __alt_instructions_end[];
>>
>>   #ifdef CONFIG_64BIT
>>
>> -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> -
>>   #undef dereference_function_descriptor
>>   void *dereference_function_descriptor(void *);
>>
>> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
>> index 32e7035863ac..b7f1ba04e756 100644
>> --- a/arch/powerpc/include/asm/sections.h
>> +++ b/arch/powerpc/include/asm/sections.h
>> @@ -8,6 +8,10 @@
>>
>>   #define arch_is_kernel_initmem_freed arch_is_kernel_initmem_freed
>>
>> +#ifdef PPC64_ELF_ABI_v1
>> +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +#endif
>> +
>>   #include <asm-generic/sections.h>
>>
>>   extern bool init_mem_is_free;
>> @@ -69,8 +73,6 @@ static inline int overlaps_kernel_text(unsigned long start, unsigned long end)
>>
>>   #ifdef PPC64_ELF_ABI_v1
>>
>> -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> -
>>   #undef dereference_function_descriptor
>>   static inline void *dereference_function_descriptor(void *ptr)
>>   {
>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> index d16302d3eb59..1db5cfd69817 100644
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -59,7 +59,8 @@ extern char __noinstr_text_start[], __noinstr_text_end[];
>>   extern __visible const void __nosave_begin, __nosave_end;
>>
>>   /* Function descriptor handling (if any).  Override in asm/sections.h */
>> -#ifndef dereference_function_descriptor
>> +#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
>> +#else
> 
> why not
> #ifndef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> instead of #if/#else ?

To avoid changing it again in patch 6, or getting an ugly #ifndef/#else 
at the end.

> 
>>   #define dereference_function_descriptor(p) ((void *)(p))
>>   #define dereference_kernel_function_descriptor(p) ((void *)(p))
>>   #endif
>>
> 

Re: [PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs

[Helge Deller]

* Christophe Leroy <christophe.leroy@csgroup.eu>:
>
>
> Le 12/10/2021 à 08:02, Helge Deller a écrit :
> > On 10/11/21 17:25, Christophe Leroy wrote:
> > > Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of 'dereference_function_descriptor'
> > > to know whether arch has function descriptors.
> > >
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > ---
> > >   arch/ia64/include/asm/sections.h    | 4 ++--
> > >   arch/parisc/include/asm/sections.h  | 6 ++++--
> > >   arch/powerpc/include/asm/sections.h | 6 ++++--
> > >   include/asm-generic/sections.h      | 3 ++-
> > >   4 files changed, 12 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
> > > index 35f24e52149a..80f5868afb06 100644
> > > --- a/arch/ia64/include/asm/sections.h
> > > +++ b/arch/ia64/include/asm/sections.h
> > > @@ -7,6 +7,8 @@
> > >    *	David Mosberger-Tang <davidm@hpl.hp.com>
> > >    */
> > >
> > > +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> > > +
> > >   #include <linux/elf.h>
> > >   #include <linux/uaccess.h>
> > >   #include <asm-generic/sections.h>
> > > @@ -27,8 +29,6 @@ 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[];
> > >
> > > -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> > > -
> > >   #undef dereference_function_descriptor
> > >   static inline void *dereference_function_descriptor(void *ptr)
> > >   {
> > > diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
> > > index bb52aea0cb21..2e781ee19b66 100644
> > > --- a/arch/parisc/include/asm/sections.h
> > > +++ b/arch/parisc/include/asm/sections.h
> > > @@ -2,6 +2,10 @@
> > >   #ifndef _PARISC_SECTIONS_H
> > >   #define _PARISC_SECTIONS_H
> > >
> > > +#ifdef CONFIG_64BIT
> > > +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> > > +#endif
> > > +
> > >   /* nothing to see, move along */
> > >   #include <asm-generic/sections.h>
> > >
> > > @@ -9,8 +13,6 @@ extern char __alt_instructions[], __alt_instructions_end[];
> > >
> > >   #ifdef CONFIG_64BIT
> > >
> > > -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> > > -
> > >   #undef dereference_function_descriptor
> > >   void *dereference_function_descriptor(void *);
> > >
> > > diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
> > > index 32e7035863ac..b7f1ba04e756 100644
> > > --- a/arch/powerpc/include/asm/sections.h
> > > +++ b/arch/powerpc/include/asm/sections.h
> > > @@ -8,6 +8,10 @@
> > >
> > >   #define arch_is_kernel_initmem_freed arch_is_kernel_initmem_freed
> > >
> > > +#ifdef PPC64_ELF_ABI_v1
> > > +#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> > > +#endif
> > > +
> > >   #include <asm-generic/sections.h>
> > >
> > >   extern bool init_mem_is_free;
> > > @@ -69,8 +73,6 @@ static inline int overlaps_kernel_text(unsigned long start, unsigned long end)
> > >
> > >   #ifdef PPC64_ELF_ABI_v1
> > >
> > > -#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> > > -
> > >   #undef dereference_function_descriptor
> > >   static inline void *dereference_function_descriptor(void *ptr)
> > >   {
> > > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> > > index d16302d3eb59..1db5cfd69817 100644
> > > --- a/include/asm-generic/sections.h
> > > +++ b/include/asm-generic/sections.h
> > > @@ -59,7 +59,8 @@ extern char __noinstr_text_start[], __noinstr_text_end[];
> > >   extern __visible const void __nosave_begin, __nosave_end;
> > >
> > >   /* Function descriptor handling (if any).  Override in asm/sections.h */
> > > -#ifndef dereference_function_descriptor
> > > +#ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> > > +#else
> >
> > why not
> > #ifndef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> > instead of #if/#else ?
>
> To avoid changing it again in patch 6, or getting an ugly #ifndef/#else at
> the end.

Ok.

Building on parisc fails at multiple files like this:
  CC      mm/filemap.o
In file included from /home/cvs/parisc/git-kernel/linus-linux-2.6/include/linux/interrupt.h:21,
                 from /home/cvs/parisc/git-kernel/linus-linux-2.6/include/linux/trace_recursion.h:5,
                 from /home/cvs/parisc/git-kernel/linus-linux-2.6/include/linux/ftrace.h:10,
                 from /home/cvs/parisc/git-kernel/linus-linux-2.6/include/linux/perf_event.h:49,
                 from /home/cvs/parisc/git-kernel/linus-linux-2.6/include/linux/trace_events.h:10,
                 from /home/cvs/parisc/git-kernel/linus-linux-2.6/include/trace/syscall.h:7,
                 from /home/cvs/parisc/git-kernel/linus-linux-2.6/include/linux/syscalls.h:87,
                 from /home/cvs/parisc/git-kernel/linus-linux-2.6/init/initramfs.c:11:
/home/cvs/parisc/git-kernel/linus-linux-2.6/arch/parisc/include/asm/sections.h:7:9: error: unknown type name ‘Elf64_Fdesc’
    7 | typedef Elf64_Fdesc funct_descr_t;
      |         ^~~~~~~~~~~


So, you still need e.g. this patch:

diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index b041fb32a300..177983490e7c 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -4,6 +4,8 @@

 #ifdef CONFIG_64BIT
 #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+#include <asm/elf.h>
+#include <linux/uaccess.h>
 typedef Elf64_Fdesc funct_descr_t;
 #endif

In general to save space I think it would be beneficial if the
dereference_kernel_function_descriptor() and
dereference_kernel_function_descriptor() functions would go as real
functions a c-file instead of just being inlined functions in the
asm-generic/sections.h header file.

Other than that it's a nice cleanup!

Helge

Re: [PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:31PM +0200, Christophe Leroy wrote:
> Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of 'dereference_function_descriptor'
> to know whether arch has function descriptors.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

I'd mention the intentionally-empty #if/#else in the commit log, as I,
like Helge, mentally tripped over it in the review. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v1 04/10] asm-generic: Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to define associated stubs

[Christophe Leroy]

Le 13/10/2021 à 09:00, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:31PM +0200, Christophe Leroy wrote:
>> Use HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR instead of 'dereference_function_descriptor'
>> to know whether arch has function descriptors.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> I'd mention the intentionally-empty #if/#else in the commit log, as I,
> like Helge, mentally tripped over it in the review. :)
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 

Ok, I did it in v2.

I also renamed it HAVE_FUNCTION_DESCRIPTORS because behind the fact that 
it has some functions to dereference function descriptor, the main fact 
is that they have and use function descriptors.

Christophe

[PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors

[Christophe Leroy]

We have three architectures using function descriptors, each with its
own 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.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/ia64/include/asm/sections.h    | 1 +
 arch/parisc/include/asm/sections.h  | 1 +
 arch/powerpc/include/asm/sections.h | 1 +
 include/asm-generic/sections.h      | 3 +++
 4 files changed, 6 insertions(+)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 80f5868afb06..929b5c535620 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -8,6 +8,7 @@
  */
 
 #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+typedef struct fdesc funct_descr_t;
 
 #include <linux/elf.h>
 #include <linux/uaccess.h>
diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
index 2e781ee19b66..329e80f7af0a 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -4,6 +4,7 @@
 
 #ifdef CONFIG_64BIT
 #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+typedef Elf64_Fdesc funct_descr_t;
 #endif
 
 /* nothing to see, move along */
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index b7f1ba04e756..d0d5287fa568 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -10,6 +10,7 @@
 
 #ifdef PPC64_ELF_ABI_v1
 #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
+typedef struct ppc64_opd_entry funct_descr_t;
 #endif
 
 #include <asm-generic/sections.h>
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 1db5cfd69817..436412d94054 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -63,6 +63,9 @@ extern __visible const void __nosave_begin, __nosave_end;
 #else
 #define dereference_function_descriptor(p) ((void *)(p))
 #define dereference_kernel_function_descriptor(p) ((void *)(p))
+typedef struct {
+	unsigned long addr;
+} funct_descr_t;
 #endif
 
 /* random extra sections (if any).  Override
-- 
2.31.1

Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
> We have three architectures using function descriptors, each with its
> own name.
> 
> Add a common typedef that can be used in generic code.
> 
> Also add a stub typedef for architecture without function descriptors,

nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:

Reviewed-by: Kees Cook <keescook@chromium.org>


> to avoid a forest of #ifdefs.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/ia64/include/asm/sections.h    | 1 +
>  arch/parisc/include/asm/sections.h  | 1 +
>  arch/powerpc/include/asm/sections.h | 1 +
>  include/asm-generic/sections.h      | 3 +++
>  4 files changed, 6 insertions(+)
> 
> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
> index 80f5868afb06..929b5c535620 100644
> --- a/arch/ia64/include/asm/sections.h
> +++ b/arch/ia64/include/asm/sections.h
> @@ -8,6 +8,7 @@
>   */
>  
>  #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef struct fdesc funct_descr_t;
>  
>  #include <linux/elf.h>
>  #include <linux/uaccess.h>
> diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
> index 2e781ee19b66..329e80f7af0a 100644
> --- a/arch/parisc/include/asm/sections.h
> +++ b/arch/parisc/include/asm/sections.h
> @@ -4,6 +4,7 @@
>  
>  #ifdef CONFIG_64BIT
>  #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef Elf64_Fdesc funct_descr_t;
>  #endif
>  
>  /* nothing to see, move along */
> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
> index b7f1ba04e756..d0d5287fa568 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -10,6 +10,7 @@
>  
>  #ifdef PPC64_ELF_ABI_v1
>  #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
> +typedef struct ppc64_opd_entry funct_descr_t;
>  #endif
>  
>  #include <asm-generic/sections.h>
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 1db5cfd69817..436412d94054 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -63,6 +63,9 @@ extern __visible const void __nosave_begin, __nosave_end;
>  #else
>  #define dereference_function_descriptor(p) ((void *)(p))
>  #define dereference_kernel_function_descriptor(p) ((void *)(p))
> +typedef struct {
> +	unsigned long addr;
> +} funct_descr_t;
>  #endif
>  
>  /* random extra sections (if any).  Override
> -- 
> 2.31.1
> 

-- 
Kees Cook

Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors

[Christophe Leroy]

Le 13/10/2021 à 09:01, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
>> We have three architectures using function descriptors, each with its
>> own name.
>>
>> Add a common typedef that can be used in generic code.
>>
>> Also add a stub typedef for architecture without function descriptors,
> 
> nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:

func_desc_t already exists in powerpc. I have a patch to remove it as it 
is redundant with struct ppc64_opd_entry, but I didnt' want to include 
it in this series.

But after all I can add it in this series, I'll add it in v2.

Christophe

> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> 
>> to avoid a forest of #ifdefs.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/ia64/include/asm/sections.h    | 1 +
>>   arch/parisc/include/asm/sections.h  | 1 +
>>   arch/powerpc/include/asm/sections.h | 1 +
>>   include/asm-generic/sections.h      | 3 +++
>>   4 files changed, 6 insertions(+)
>>
>> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
>> index 80f5868afb06..929b5c535620 100644
>> --- a/arch/ia64/include/asm/sections.h
>> +++ b/arch/ia64/include/asm/sections.h
>> @@ -8,6 +8,7 @@
>>    */
>>   
>>   #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +typedef struct fdesc funct_descr_t;
>>   
>>   #include <linux/elf.h>
>>   #include <linux/uaccess.h>
>> diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h
>> index 2e781ee19b66..329e80f7af0a 100644
>> --- a/arch/parisc/include/asm/sections.h
>> +++ b/arch/parisc/include/asm/sections.h
>> @@ -4,6 +4,7 @@
>>   
>>   #ifdef CONFIG_64BIT
>>   #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +typedef Elf64_Fdesc funct_descr_t;
>>   #endif
>>   
>>   /* nothing to see, move along */
>> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
>> index b7f1ba04e756..d0d5287fa568 100644
>> --- a/arch/powerpc/include/asm/sections.h
>> +++ b/arch/powerpc/include/asm/sections.h
>> @@ -10,6 +10,7 @@
>>   
>>   #ifdef PPC64_ELF_ABI_v1
>>   #define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
>> +typedef struct ppc64_opd_entry funct_descr_t;
>>   #endif
>>   
>>   #include <asm-generic/sections.h>
>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> index 1db5cfd69817..436412d94054 100644
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -63,6 +63,9 @@ extern __visible const void __nosave_begin, __nosave_end;
>>   #else
>>   #define dereference_function_descriptor(p) ((void *)(p))
>>   #define dereference_kernel_function_descriptor(p) ((void *)(p))
>> +typedef struct {
>> +	unsigned long addr;
>> +} funct_descr_t;
>>   #endif
>>   
>>   /* random extra sections (if any).  Override
>> -- 
>> 2.31.1
>>
> 

Re: [PATCH v1 05/10] asm-generic: Define 'funct_descr_t' to commonly describe function descriptors

[Kees Cook]

On Wed, Oct 13, 2021 at 09:23:56AM +0200, Christophe Leroy wrote:
> 
> 
> Le 13/10/2021 à 09:01, Kees Cook a écrit :
> > On Mon, Oct 11, 2021 at 05:25:32PM +0200, Christophe Leroy wrote:
> > > We have three architectures using function descriptors, each with its
> > > own name.
> > > 
> > > Add a common typedef that can be used in generic code.
> > > 
> > > Also add a stub typedef for architecture without function descriptors,
> > 
> > nit: funct_descr_t reads weird to me. why not func_desc_t ? Either way:
> 
> func_desc_t already exists in powerpc. I have a patch to remove it as it is
> redundant with struct ppc64_opd_entry, but I didnt' want to include it in
> this series.
> 
> But after all I can add it in this series, I'll add it in v2.

Ah-ha! That works for me. :) Thanks!

-Kees

-- 
Kees Cook

[PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()

[Christophe Leroy]

dereference_function_descriptor() and
dereference_kernel_function_descriptor() are identical on the
three architectures implementing them.

Make it common.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 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      | 18 ++++++++++++++++++
 5 files changed, 18 insertions(+), 72 deletions(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 929b5c535620..d9addaea8339 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 329e80f7af0a..b041fb32a300 100644
--- a/arch/parisc/include/asm/sections.h
+++ b/arch/parisc/include/asm/sections.h
@@ -12,13 +12,4 @@ typedef Elf64_Fdesc funct_descr_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 38ec4ae81239..7382576b52a8 100644
--- a/arch/parisc/kernel/process.c
+++ b/arch/parisc/kernel/process.c
@@ -266,27 +266,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 d0d5287fa568..8f8e95f234e2 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -72,29 +72,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 ppc64_opd_entry *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 436412d94054..5baaf9d7c671 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -60,6 +60,24 @@ extern __visible const void __nosave_begin, __nosave_end;
 
 /* Function descriptor handling (if any).  Override in asm/sections.h */
 #ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
+static inline void *dereference_function_descriptor(void *ptr)
+{
+	funct_descr_t *desc = ptr;
+	void *p;
+
+	if (!get_kernel_nofault(p, (void *)&desc->addr))
+		ptr = p;
+	return ptr;
+}
+
+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);
+}
+
 #else
 #define dereference_function_descriptor(p) ((void *)(p))
 #define dereference_kernel_function_descriptor(p) ((void *)(p))
-- 
2.31.1

Re: [PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:33PM +0200, Christophe Leroy wrote:
> dereference_function_descriptor() and
> dereference_kernel_function_descriptor() are identical on the
> three architectures implementing them.
> 
> Make it common.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  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      | 18 ++++++++++++++++++
>  5 files changed, 18 insertions(+), 72 deletions(-)

A diffstat to love. :)

Reviewed-by: Kees Cook <keescook@chromium.org>


> 
> diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
> index 929b5c535620..d9addaea8339 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 329e80f7af0a..b041fb32a300 100644
> --- a/arch/parisc/include/asm/sections.h
> +++ b/arch/parisc/include/asm/sections.h
> @@ -12,13 +12,4 @@ typedef Elf64_Fdesc funct_descr_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 38ec4ae81239..7382576b52a8 100644
> --- a/arch/parisc/kernel/process.c
> +++ b/arch/parisc/kernel/process.c
> @@ -266,27 +266,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 d0d5287fa568..8f8e95f234e2 100644
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -72,29 +72,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 ppc64_opd_entry *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 436412d94054..5baaf9d7c671 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -60,6 +60,24 @@ extern __visible const void __nosave_begin, __nosave_end;
>  
>  /* Function descriptor handling (if any).  Override in asm/sections.h */
>  #ifdef HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR
> +static inline void *dereference_function_descriptor(void *ptr)
> +{
> +	funct_descr_t *desc = ptr;
> +	void *p;
> +
> +	if (!get_kernel_nofault(p, (void *)&desc->addr))
> +		ptr = p;
> +	return ptr;
> +}
> +
> +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);
> +}
> +
>  #else
>  #define dereference_function_descriptor(p) ((void *)(p))
>  #define dereference_kernel_function_descriptor(p) ((void *)(p))
> -- 
> 2.31.1
> 

-- 
Kees Cook

Re: [PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()

[Christophe Leroy]

Le 13/10/2021 à 09:02, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:33PM +0200, Christophe Leroy wrote:
>> dereference_function_descriptor() and
>> dereference_kernel_function_descriptor() are identical on the
>> three architectures implementing them.
>>
>> Make it common.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   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      | 18 ++++++++++++++++++
>>   5 files changed, 18 insertions(+), 72 deletions(-)
> 
> A diffstat to love. :)
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>

Unless somebody minds, I will make them out of line as
suggested by Helge in he's comment to patch 4.

Allthough there is no spectacular size reduction, the functions
are not worth being inlined as they are not used in critical pathes.

Re: [PATCH v1 06/10] asm-generic: Refactor dereference_[kernel]_function_descriptor()

[Arnd Bergmann]

On Wed, Oct 13, 2021 at 1:20 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> Le 13/10/2021 à 09:02, Kees Cook a écrit :
> > On Mon, Oct 11, 2021 at 05:25:33PM +0200, Christophe Leroy wrote:
> >> dereference_function_descriptor() and
> >> dereference_kernel_function_descriptor() are identical on the
> >> three architectures implementing them.
> >>
> >> Make it common.
> >>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >> ---
> >>   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      | 18 ++++++++++++++++++
> >>   5 files changed, 18 insertions(+), 72 deletions(-)
> >
> > A diffstat to love. :)
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> Unless somebody minds, I will make them out of line as
> suggested by Helge in he's comment to patch 4.
>
> Allthough there is no spectacular size reduction, the functions
> are not worth being inlined as they are not used in critical pathes.

Sounds good to me.

      Arnd

[PATCH v1 07/10] lkdtm: Force do_nothing() out of line

[Christophe Leroy]

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.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 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.31.1

Re: [PATCH v1 07/10] lkdtm: Force do_nothing() out of line

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:34PM +0200, Christophe Leroy wrote:
> 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.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH v1 08/10] lkdtm: Really write into kernel text in WRITE_KERN

[Christophe Leroy]

WRITE_KERN is supposed to overwrite some kernel text, namely
do_overwritten() function.

But at the time being it overwrites do_overwritten() function
descriptor, not function text.

Fix it by dereferencing the function descriptor to obtain
function text pointer.

And make do_overwritten() noinline so that it is really
do_overwritten() which is called by lkdtm_WRITE_KERN().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 drivers/misc/lkdtm/perms.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 60b3b2fe929d..442d60ed25ef 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -5,6 +5,7 @@
  * even non-readable regions.
  */
 #include "lkdtm.h"
+#include <linux/kallsyms.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/mman.h>
@@ -37,7 +38,7 @@ static noinline void do_nothing(void)
 }
 
 /* Must immediately follow do_nothing for size calculuations to work out. */
-static void do_overwritten(void)
+static noinline void do_overwritten(void)
 {
 	pr_info("do_overwritten wasn't overwritten!\n");
 	return;
@@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)
 	size_t size;
 	volatile unsigned char *ptr;
 
-	size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
-	ptr = (unsigned char *)do_overwritten;
+	size = (unsigned long)dereference_symbol_descriptor(do_overwritten) -
+	       (unsigned long)dereference_symbol_descriptor(do_nothing);
+	ptr = dereference_symbol_descriptor(do_overwritten);
 
 	pr_info("attempting bad %zu byte write at %px\n", size, ptr);
 	memcpy((void *)ptr, (unsigned char *)do_nothing, size);
-- 
2.31.1

Re: [PATCH v1 08/10] lkdtm: Really write into kernel text in WRITE_KERN

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:35PM +0200, Christophe Leroy wrote:
> WRITE_KERN is supposed to overwrite some kernel text, namely
> do_overwritten() function.
> 
> But at the time being it overwrites do_overwritten() function
> descriptor, not function text.
> 
> Fix it by dereferencing the function descriptor to obtain
> function text pointer.
> 
> And make do_overwritten() noinline so that it is really
> do_overwritten() which is called by lkdtm_WRITE_KERN().
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  drivers/misc/lkdtm/perms.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 60b3b2fe929d..442d60ed25ef 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -5,6 +5,7 @@
>   * even non-readable regions.
>   */
>  #include "lkdtm.h"
> +#include <linux/kallsyms.h>

Why not #include <asm/sections.h> instead here?

>  #include <linux/slab.h>
>  #include <linux/vmalloc.h>
>  #include <linux/mman.h>
> @@ -37,7 +38,7 @@ static noinline void do_nothing(void)
>  }
>  
>  /* Must immediately follow do_nothing for size calculuations to work out. */
> -static void do_overwritten(void)
> +static noinline void do_overwritten(void)
>  {
>  	pr_info("do_overwritten wasn't overwritten!\n");
>  	return;
> @@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)
>  	size_t size;
>  	volatile unsigned char *ptr;
>  
> -	size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
> -	ptr = (unsigned char *)do_overwritten;
> +	size = (unsigned long)dereference_symbol_descriptor(do_overwritten) -
> +	       (unsigned long)dereference_symbol_descriptor(do_nothing);
> +	ptr = dereference_symbol_descriptor(do_overwritten);

But otherwise, yup, I expect there will be a bunch of things like this
to clean up in LKDTM. :| Sorry about that!

Acked-by: Kees Cook <keescook@chromium.org>

>  
>  	pr_info("attempting bad %zu byte write at %px\n", size, ptr);
>  	memcpy((void *)ptr, (unsigned char *)do_nothing, size);
> -- 
> 2.31.1
> 

-- 
Kees Cook

Re: [PATCH v1 08/10] lkdtm: Really write into kernel text in WRITE_KERN

[Christophe Leroy]

Le 13/10/2021 à 09:05, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:35PM +0200, Christophe Leroy wrote:
>> WRITE_KERN is supposed to overwrite some kernel text, namely
>> do_overwritten() function.
>>
>> But at the time being it overwrites do_overwritten() function
>> descriptor, not function text.
>>
>> Fix it by dereferencing the function descriptor to obtain
>> function text pointer.
>>
>> And make do_overwritten() noinline so that it is really
>> do_overwritten() which is called by lkdtm_WRITE_KERN().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   drivers/misc/lkdtm/perms.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index 60b3b2fe929d..442d60ed25ef 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -5,6 +5,7 @@
>>    * even non-readable regions.
>>    */
>>   #include "lkdtm.h"
>> +#include <linux/kallsyms.h>
> 
> Why not #include <asm/sections.h> instead here?

dereference_symbol_descriptor() is defined in linux/kallsyms.h

> 
>>   #include <linux/slab.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/mman.h>
>> @@ -37,7 +38,7 @@ static noinline void do_nothing(void)
>>   }
>>   
>>   /* Must immediately follow do_nothing for size calculuations to work out. */
>> -static void do_overwritten(void)
>> +static noinline void do_overwritten(void)
>>   {
>>   	pr_info("do_overwritten wasn't overwritten!\n");
>>   	return;
>> @@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)
>>   	size_t size;
>>   	volatile unsigned char *ptr;
>>   
>> -	size = (unsigned long)do_overwritten - (unsigned long)do_nothing;
>> -	ptr = (unsigned char *)do_overwritten;
>> +	size = (unsigned long)dereference_symbol_descriptor(do_overwritten) -
>> +	       (unsigned long)dereference_symbol_descriptor(do_nothing);
>> +	ptr = dereference_symbol_descriptor(do_overwritten);
> 
> But otherwise, yup, I expect there will be a bunch of things like this
> to clean up in LKDTM. :| Sorry about that!
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
>>   
>>   	pr_info("attempting bad %zu byte write at %px\n", size, ptr);
>>   	memcpy((void *)ptr, (unsigned char *)do_nothing, size);
>> -- 
>> 2.31.1
>>
> 

[PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()

[Christophe Leroy]

Behind a location, lkdtm_EXEC_RODATA() executes a real function,
not a copy of do_nothing().

So do it directly instead of using execute_location().

And fix displayed addresses by dereferencing the function descriptors.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 drivers/misc/lkdtm/perms.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 442d60ed25ef..da16564e1ecd 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
 
 void lkdtm_EXEC_RODATA(void)
 {
-	execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
+	pr_info("attempting ok execution at %px\n",
+		dereference_symbol_descriptor(do_nothing));
+	do_nothing();
+
+	pr_info("attempting bad execution at %px\n",
+		dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
+	lkdtm_rodata_do_nothing();
+	pr_err("FAIL: func returned\n");
 }
 
 void lkdtm_EXEC_USERSPACE(void)
-- 
2.31.1

Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
> not a copy of do_nothing().
> 
> So do it directly instead of using execute_location().

I don't understand this. Why does the next patch not fix this?

-Kees

> 
> And fix displayed addresses by dereferencing the function descriptors.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  drivers/misc/lkdtm/perms.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 442d60ed25ef..da16564e1ecd 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>  
>  void lkdtm_EXEC_RODATA(void)
>  {
> -	execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
> +	pr_info("attempting ok execution at %px\n",
> +		dereference_symbol_descriptor(do_nothing));
> +	do_nothing();
> +
> +	pr_info("attempting bad execution at %px\n",
> +		dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
> +	lkdtm_rodata_do_nothing();
> +	pr_err("FAIL: func returned\n");
>  }
>  
>  void lkdtm_EXEC_USERSPACE(void)
> -- 
> 2.31.1
> 

-- 
Kees Cook

Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()

[Christophe Leroy]

Le 13/10/2021 à 09:09, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
>> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
>> not a copy of do_nothing().
>>
>> So do it directly instead of using execute_location().
> 
> I don't understand this. Why does the next patch not fix this?

Well, probably it would, but it looked incorrect in my mind.

lkdtm_rodata_do_nothing() is a function which has its own function 
descriptor, it is not a copy of do_nothing().

If we use execute_location() as modified by next patch, then we will 
execute it using the function descriptor of do_nothing(). Allthough it 
most likely works (at least on powerpc as it uses the same TOC) it looks 
odd to me to do so.

Am I missing something ?

Christophe


> 
> -Kees
> 
>>
>> And fix displayed addresses by dereferencing the function descriptors.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   drivers/misc/lkdtm/perms.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index 442d60ed25ef..da16564e1ecd 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>>   
>>   void lkdtm_EXEC_RODATA(void)
>>   {
>> -	execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
>> +	pr_info("attempting ok execution at %px\n",
>> +		dereference_symbol_descriptor(do_nothing));
>> +	do_nothing();
>> +
>> +	pr_info("attempting bad execution at %px\n",
>> +		dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
>> +	lkdtm_rodata_do_nothing();
>> +	pr_err("FAIL: func returned\n");
>>   }
>>   
>>   void lkdtm_EXEC_USERSPACE(void)
>> -- 
>> 2.31.1
>>
> 

Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
> not a copy of do_nothing().
>
> So do it directly instead of using execute_location().
>
> And fix displayed addresses by dereferencing the function descriptors.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  drivers/misc/lkdtm/perms.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 442d60ed25ef..da16564e1ecd 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>
>  void lkdtm_EXEC_RODATA(void)
>  {
> -	execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
> +	pr_info("attempting ok execution at %px\n",
> +		dereference_symbol_descriptor(do_nothing));
> +	do_nothing();
> +
> +	pr_info("attempting bad execution at %px\n",
> +		dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
> +	lkdtm_rodata_do_nothing();
> +	pr_err("FAIL: func returned\n");
>  }

(In re-reading this more carefully, I see now why kallsyms.h is used
earlier: _function_ vs _symbol_ descriptor.)

In the next patch:

static noinline void execute_location(void *dst, bool write)
{
...
       func = setup_function_descriptor(&fdesc, dst);
       if (IS_ERR(func))
               return;

       pr_info("attempting bad execution at %px\n", dst);
       func();
       pr_err("FAIL: func returned\n");
}

What are the conditions for which dereference_symbol_descriptor works
but dereference _function_descriptor doesn't?

--
Kees Cook

Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()

[Christophe Leroy]

Le 13/10/2021 à 09:23, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
>> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
>> not a copy of do_nothing().
>>
>> So do it directly instead of using execute_location().
>>
>> And fix displayed addresses by dereferencing the function descriptors.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   drivers/misc/lkdtm/perms.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index 442d60ed25ef..da16564e1ecd 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>>
>>   void lkdtm_EXEC_RODATA(void)
>>   {
>> -	execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
>> +	pr_info("attempting ok execution at %px\n",
>> +		dereference_symbol_descriptor(do_nothing));
>> +	do_nothing();
>> +
>> +	pr_info("attempting bad execution at %px\n",
>> +		dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
>> +	lkdtm_rodata_do_nothing();
>> +	pr_err("FAIL: func returned\n");
>>   }
> 
> (In re-reading this more carefully, I see now why kallsyms.h is used
> earlier: _function_ vs _symbol_ descriptor.)
> 
> In the next patch:
> 
> static noinline void execute_location(void *dst, bool write)
> {
> ...
>         func = setup_function_descriptor(&fdesc, dst);
>         if (IS_ERR(func))
>                 return;
> 
>         pr_info("attempting bad execution at %px\n", dst);
>         func();
>         pr_err("FAIL: func returned\n");
> }
> 
> What are the conditions for which dereference_symbol_descriptor works
> but dereference _function_descriptor doesn't?
> 

When LKDTM is built as a module I guess ?

Christophe

Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()

[Christophe Leroy]

Le 13/10/2021 à 09:39, Christophe Leroy a écrit :
> 
> 
> Le 13/10/2021 à 09:23, Kees Cook a écrit :
>> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
>>> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
>>> not a copy of do_nothing().
>>>
>>> So do it directly instead of using execute_location().
>>>
>>> And fix displayed addresses by dereferencing the function descriptors.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>>   drivers/misc/lkdtm/perms.c | 9 ++++++++-
>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>>> index 442d60ed25ef..da16564e1ecd 100644
>>> --- a/drivers/misc/lkdtm/perms.c
>>> +++ b/drivers/misc/lkdtm/perms.c
>>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>>>
>>>   void lkdtm_EXEC_RODATA(void)
>>>   {
>>> -    execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
>>> +    pr_info("attempting ok execution at %px\n",
>>> +        dereference_symbol_descriptor(do_nothing));
>>> +    do_nothing();
>>> +
>>> +    pr_info("attempting bad execution at %px\n",
>>> +        dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
>>> +    lkdtm_rodata_do_nothing();
>>> +    pr_err("FAIL: func returned\n");
>>>   }
>>
>> (In re-reading this more carefully, I see now why kallsyms.h is used
>> earlier: _function_ vs _symbol_ descriptor.)
>>
>> In the next patch:
>>
>> static noinline void execute_location(void *dst, bool write)
>> {
>> ...
>>         func = setup_function_descriptor(&fdesc, dst);
>>         if (IS_ERR(func))
>>                 return;
>>
>>         pr_info("attempting bad execution at %px\n", dst);
>>         func();
>>         pr_err("FAIL: func returned\n");
>> }
>>
>> What are the conditions for which dereference_symbol_descriptor works
>> but dereference _function_descriptor doesn't?
>>
> 
> When LKDTM is built as a module I guess ?
> 

To be more precise, dereference_symbol_descriptor() calls either 
dereference_kernel_function_descriptor() or 
dereference_module_function_descriptor()

Both functions call dereference_function_descriptor() after checking 
that we want to dereference something that is in the OPD section.

If we call dereference_function_descriptor() directly instead of 
dereference_symbol_descriptor() we skip the range verification and may 
dereference something that is not a function descriptor.

Should we do that ?

Christophe

Re: [PATCH v1 09/10] lkdtm: Fix lkdtm_EXEC_RODATA()

[Christophe Leroy]

Le 13/10/2021 à 09:48, Christophe Leroy a écrit :
> 
> 
> Le 13/10/2021 à 09:39, Christophe Leroy a écrit :
>>
>>
>> Le 13/10/2021 à 09:23, Kees Cook a écrit :
>>> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote:
>>>> Behind a location, lkdtm_EXEC_RODATA() executes a real function,
>>>> not a copy of do_nothing().
>>>>
>>>> So do it directly instead of using execute_location().
>>>>
>>>> And fix displayed addresses by dereferencing the function descriptors.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>>   drivers/misc/lkdtm/perms.c | 9 ++++++++-
>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>>>> index 442d60ed25ef..da16564e1ecd 100644
>>>> --- a/drivers/misc/lkdtm/perms.c
>>>> +++ b/drivers/misc/lkdtm/perms.c
>>>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>>>>
>>>>   void lkdtm_EXEC_RODATA(void)
>>>>   {
>>>> -    execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
>>>> +    pr_info("attempting ok execution at %px\n",
>>>> +        dereference_symbol_descriptor(do_nothing));
>>>> +    do_nothing();
>>>> +
>>>> +    pr_info("attempting bad execution at %px\n",
>>>> +        dereference_symbol_descriptor(lkdtm_rodata_do_nothing));
>>>> +    lkdtm_rodata_do_nothing();
>>>> +    pr_err("FAIL: func returned\n");
>>>>   }
>>>
>>> (In re-reading this more carefully, I see now why kallsyms.h is used
>>> earlier: _function_ vs _symbol_ descriptor.)
>>>
>>> In the next patch:
>>>
>>> static noinline void execute_location(void *dst, bool write)
>>> {
>>> ...
>>>         func = setup_function_descriptor(&fdesc, dst);
>>>         if (IS_ERR(func))
>>>                 return;
>>>
>>>         pr_info("attempting bad execution at %px\n", dst);
>>>         func();
>>>         pr_err("FAIL: func returned\n");
>>> }
>>>
>>> What are the conditions for which dereference_symbol_descriptor works
>>> but dereference _function_descriptor doesn't?
>>>
>>
>> When LKDTM is built as a module I guess ?
>>
> 
> To be more precise, dereference_symbol_descriptor() calls either 
> dereference_kernel_function_descriptor() or 
> dereference_module_function_descriptor()
> 
> Both functions call dereference_function_descriptor() after checking 
> that we want to dereference something that is in the OPD section.
> 
> If we call dereference_function_descriptor() directly instead of 
> dereference_symbol_descriptor() we skip the range verification and may 
> dereference something that is not a function descriptor.
> 
> Should we do that ?
> 

Indeed we are using it only for well known functions so using 
dereference_function_descriptor() is good enough. I'll use that in v2.

[PATCH v1 10/10] lkdtm: Fix execute_[user]_location()

[Christophe Leroy]

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 <christophe.leroy@csgroup.eu>
---
 drivers/misc/lkdtm/perms.c | 45 +++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index da16564e1ecd..1d03cd44c21d 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -44,19 +44,42 @@ static noinline void do_overwritten(void)
 	return;
 }
 
+static void *setup_function_descriptor(funct_descr_t *fdesc, void *dst)
+{
+	int err;
+
+	if (!__is_defined(HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR))
+		return dst;
+
+	err = copy_from_kernel_nofault(fdesc, do_nothing, sizeof(*fdesc));
+	if (err < 0)
+		return ERR_PTR(err);
+
+	fdesc->addr = (unsigned long)dst;
+	barrier();
+
+	return fdesc;
+}
+
 static noinline void execute_location(void *dst, bool write)
 {
-	void (*func)(void) = dst;
+	void (*func)(void);
+	funct_descr_t fdesc;
+	void *do_nothing_text = dereference_symbol_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);
+	func = setup_function_descriptor(&fdesc, dst);
+	if (IS_ERR(func))
+		return;
+
+	pr_info("attempting bad execution at %px\n", dst);
 	func();
 	pr_err("FAIL: func returned\n");
 }
@@ -66,16 +89,22 @@ static void execute_user_location(void *dst)
 	int copied;
 
 	/* Intentionally crossing kernel/user memory boundary. */
-	void (*func)(void) = dst;
+	void (*func)(void);
+	funct_descr_t fdesc;
+	void *do_nothing_text = dereference_symbol_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);
+	func = setup_function_descriptor(&fdesc, dst);
+	if (IS_ERR(func))
+		return;
+
+	pr_info("attempting bad execution at %px\n", dst);
 	func();
 	pr_err("FAIL: func returned\n");
 }
-- 
2.31.1

Re: [PATCH v1 10/10] lkdtm: Fix execute_[user]_location()

[Kees Cook]

On Mon, Oct 11, 2021 at 05:25:37PM +0200, Christophe Leroy wrote:
> 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 <christophe.leroy@csgroup.eu>
> ---
>  drivers/misc/lkdtm/perms.c | 45 +++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index da16564e1ecd..1d03cd44c21d 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -44,19 +44,42 @@ static noinline void do_overwritten(void)
>  	return;
>  }
>  
> +static void *setup_function_descriptor(funct_descr_t *fdesc, void *dst)
> +{
> +	int err;
> +
> +	if (!__is_defined(HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR))
> +		return dst;
> +
> +	err = copy_from_kernel_nofault(fdesc, do_nothing, sizeof(*fdesc));
> +	if (err < 0)
> +		return ERR_PTR(err);
> +
> +	fdesc->addr = (unsigned long)dst;
> +	barrier();
> +
> +	return fdesc;
> +}
> +
>  static noinline void execute_location(void *dst, bool write)
>  {
> -	void (*func)(void) = dst;
> +	void (*func)(void);
> +	funct_descr_t fdesc;
> +	void *do_nothing_text = dereference_symbol_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);
> +	func = setup_function_descriptor(&fdesc, dst);
> +	if (IS_ERR(func))
> +		return;

I think this error case should complain with some details. :) Maybe:

	pr_err("FAIL: could not build function descriptor for %px\n", dst);

> +
> +	pr_info("attempting bad execution at %px\n", dst);

And then leave this pr_info() as it was, before the
setup_function_descriptor() call.

>  	func();
>  	pr_err("FAIL: func returned\n");
>  }
> @@ -66,16 +89,22 @@ static void execute_user_location(void *dst)
>  	int copied;
>  
>  	/* Intentionally crossing kernel/user memory boundary. */
> -	void (*func)(void) = dst;
> +	void (*func)(void);
> +	funct_descr_t fdesc;
> +	void *do_nothing_text = dereference_symbol_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);
> +	func = setup_function_descriptor(&fdesc, dst);
> +	if (IS_ERR(func))
> +		return;
> +
> +	pr_info("attempting bad execution at %px\n", dst);

Same here.

>  	func();
>  	pr_err("FAIL: func returned\n");
>  }
> -- 
> 2.31.1
> 

-- 
Kees Cook

Re: [PATCH v1 10/10] lkdtm: Fix execute_[user]_location()

[Christophe Leroy]

Le 13/10/2021 à 09:16, Kees Cook a écrit :
> On Mon, Oct 11, 2021 at 05:25:37PM +0200, Christophe Leroy wrote:
>> 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 <christophe.leroy@csgroup.eu>
>> ---
>>   drivers/misc/lkdtm/perms.c | 45 +++++++++++++++++++++++++++++++-------
>>   1 file changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
>> index da16564e1ecd..1d03cd44c21d 100644
>> --- a/drivers/misc/lkdtm/perms.c
>> +++ b/drivers/misc/lkdtm/perms.c
>> @@ -44,19 +44,42 @@ static noinline void do_overwritten(void)
>>   	return;
>>   }
>>   
>> +static void *setup_function_descriptor(funct_descr_t *fdesc, void *dst)
>> +{
>> +	int err;
>> +
>> +	if (!__is_defined(HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR))
>> +		return dst;
>> +
>> +	err = copy_from_kernel_nofault(fdesc, do_nothing, sizeof(*fdesc));
>> +	if (err < 0)
>> +		return ERR_PTR(err);
>> +
>> +	fdesc->addr = (unsigned long)dst;
>> +	barrier();
>> +
>> +	return fdesc;
>> +}
>> +
>>   static noinline void execute_location(void *dst, bool write)
>>   {
>> -	void (*func)(void) = dst;
>> +	void (*func)(void);
>> +	funct_descr_t fdesc;
>> +	void *do_nothing_text = dereference_symbol_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);
>> +	func = setup_function_descriptor(&fdesc, dst);
>> +	if (IS_ERR(func))
>> +		return;
> 
> I think this error case should complain with some details. :) Maybe:
> 
> 	pr_err("FAIL: could not build function descriptor for %px\n", dst);

Ok, I was going to add that in v2, but after one more thought I realise 
that there's no need to use copy_from_kernel_nofault() here, we are 
copying a static object into our stack, so a memcpy() will be good enough.

> 
>> +
>> +	pr_info("attempting bad execution at %px\n", dst);
> 
> And then leave this pr_info() as it was, before the
> setup_function_descriptor() call.
> 
>>   	func();
>>   	pr_err("FAIL: func returned\n");
>>   }
>> @@ -66,16 +89,22 @@ static void execute_user_location(void *dst)
>>   	int copied;
>>   
>>   	/* Intentionally crossing kernel/user memory boundary. */
>> -	void (*func)(void) = dst;
>> +	void (*func)(void);
>> +	funct_descr_t fdesc;
>> +	void *do_nothing_text = dereference_symbol_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);
>> +	func = setup_function_descriptor(&fdesc, dst);
>> +	if (IS_ERR(func))
>> +		return;
>> +
>> +	pr_info("attempting bad execution at %px\n", dst);
> 
> Same here.
> 
>>   	func();
>>   	pr_err("FAIL: func returned\n");
>>   }
>> -- 
>> 2.31.1
>>
>