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 8 is not absolutely necessary but it is a good trivial cleanup.
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: Move 'struct ppc64_opd_entry' back into asm/elf.h
powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'
powerpc: Remove func_descr_t
powerpc: Prepare func_desc_t for refactorisation
ia64: Rename 'ip' to 'addr' in 'struct fdesc'
asm-generic: Use HAVE_FUNCTION_DESCRIPTORS to define associated stubs
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 lkdtm_EXEC_RODATA()
lkdtm: Fix execute_[user]_location()
lkdtm: Add a test for function descriptors protection
arch/ia64/include/asm/elf.h | 2 +-
arch/ia64/include/asm/sections.h | 25 ++-------
arch/ia64/kernel/module.c | 6 +--
arch/parisc/include/asm/sections.h | 17 +++---
arch/parisc/kernel/process.c | 21 --------
arch/powerpc/include/asm/code-patching.h | 2 +-
arch/powerpc/include/asm/elf.h | 6 +++
arch/powerpc/include/asm/sections.h | 30 ++---------
arch/powerpc/include/asm/types.h | 6 ---
arch/powerpc/include/uapi/asm/elf.h | 8 ---
arch/powerpc/kernel/module_64.c | 38 +++++--------
arch/powerpc/kernel/signal_64.c | 8 +--
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/lkdtm.h | 1 +
drivers/misc/lkdtm/perms.c | 68 ++++++++++++++++++++----
include/asm-generic/sections.h | 13 ++++-
include/linux/kallsyms.h | 2 +-
kernel/extable.c | 23 +++++++-
18 files changed, 138 insertions(+), 139 deletions(-)
--
2.31.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 <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
drivers/misc/lkdtm/perms.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -21,7 +21,7 @@/* This is non-const, so it will end up in the .data section. */staticu8data_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. */staticconstunsignedlongrodata=0xAA55AA55;/* This is marked __ro_after_init, so it should ultimately be .rodata. */
Behind its location, lkdtm_EXEC_RODATA() executes
lkdtm_rodata_do_nothing() which is a real function,
not a copy of do_nothing().
So executes it directly instead of using execute_location().
This is necessary because following patch will fix execute_location()
to use a copy of the function descriptor of do_nothing() and
function descriptor of lkdtm_rodata_do_nothing() might be different.
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy <redacted>
---
drivers/misc/lkdtm/perms.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
@@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)voidlkdtm_EXEC_RODATA(void){-execute_location(lkdtm_rodata_do_nothing,CODE_AS_IS);+pr_info("attempting ok execution at %px\n",+dereference_function_descriptor(do_nothing));+do_nothing();++pr_info("attempting bad execution at %px\n",+dereference_function_descriptor(lkdtm_rodata_do_nothing));+lkdtm_rodata_do_nothing();+pr_err("FAIL: func returned\n");}voidlkdtm_EXEC_USERSPACE(void)
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().
Acked-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
drivers/misc/lkdtm/perms.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
@@ -10,6 +10,7 @@#include<linux/mman.h>#include<linux/uaccess.h>#include<asm/cacheflush.h>+#include<asm/sections.h>/* Whether or not to fill the target memory area with do_nothing(). */#define CODE_WRITE true
@@ -37,7 +38,7 @@ static noinline void do_nothing(void)}/* Must immediately follow do_nothing for size calculuations to work out. */-staticvoiddo_overwritten(void)+staticnoinlinevoiddo_overwritten(void){pr_info("do_overwritten wasn't overwritten!\n");return;
@@ -113,8 +114,9 @@ void lkdtm_WRITE_KERN(void)size_tsize;volatileunsignedchar*ptr;-size=(unsignedlong)do_overwritten-(unsignedlong)do_nothing;-ptr=(unsignedchar*)do_overwritten;+size=(unsignedlong)dereference_function_descriptor(do_overwritten)-+(unsignedlong)dereference_function_descriptor(do_nothing);+ptr=dereference_function_descriptor(do_overwritten);pr_info("attempting bad %zu byte write at %px\n",size,ptr);memcpy((void*)ptr,(unsignedchar*)do_nothing,size);
'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")
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/include/asm/elf.h | 6 ++++++
arch/powerpc/include/uapi/asm/elf.h | 8 --------
2 files changed, 6 insertions(+), 8 deletions(-)
@@ -176,4 +176,10 @@ do { \/* Relocate the kernel image to @final_address */voidrelocate(unsignedlongfinal_address);+/* There's actually a third entry here, but it's unused */+structppc64_opd_entry{+unsignedlongfuncaddr;+unsignedlongr2;+};+#endif /* _ASM_POWERPC_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 */-structppc64_opd_entry-{-unsignedlongfuncaddr;-unsignedlongr2;-};--#endif /* _UAPI_ASM_POWERPC_ELF_H */
In preparation of making func_desc_t generic, change it to
a struct containing 'addr' element.
In addition this allows using single helpers common to ELFv1 and ELFv2.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/kernel/module_64.c | 34 +++++++++++++++------------------
1 file changed, 15 insertions(+), 19 deletions(-)
@@ -33,19 +33,13 @@#ifdef PPC64_ELF_ABI_v2/* An address is simply the address of the function. */-typedefunsignedlongfunc_desc_t;+typedefstruct{+unsignedlongaddr;+}func_desc_t;staticfunc_desc_tfunc_desc(unsignedlongaddr){-returnaddr;-}-staticunsignedlongfunc_addr(unsignedlongaddr)-{-returnaddr;-}-staticunsignedlongstub_func_addr(func_desc_tfunc)-{-returnfunc;+return(func_desc_t){addr};}/* PowerPC64 specific values for the Elf64_Sym st_other field. */
@@ -93,6 +79,16 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr)}#endif+staticunsignedlongfunc_addr(unsignedlongaddr)+{+returnfunc_desc(addr).addr;+}++staticunsignedlongstub_func_addr(func_desc_tfunc)+{+returnfunc.addr;+}+#define STUB_MAGIC 0x73747562 /* stub *//* Like PPC32, we need little trampolines to do > 24-bit jumps (into
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.
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
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(-)
@@ -602,15 +602,15 @@ get_fdesc (struct module *mod, uint64_t value, int *okp)returnvalue;/* 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;}
@@ -933,11 +933,11 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,*descriptoristheentryaddressofsignalandthesecond*entryistheTOCvalueweneedtouse.*/-func_descr_t__user*funct_desc_ptr=-(func_descr_t__user*)ksig->ka.sa.sa_handler;+structppc64_opd_entry__user*funct_desc_ptr=+(structppc64_opd_entry__user*)ksig->ka.sa.sa_handler;-err|=get_user(regs->ctr,&funct_desc_ptr->entry);-err|=get_user(regs->gpr[2],&funct_desc_ptr->toc);+err|=get_user(regs->ctr,&funct_desc_ptr->addr);+err|=get_user(regs->gpr[2],&funct_desc_ptr->r2);}/* enter the signal handler in native-endian mode */
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.
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
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(-)
@@ -178,7 +178,7 @@ void relocate(unsigned long final_address);/* There's actually a third entry here, but it's unused */structppc64_opd_entry{-unsignedlongfuncaddr;+unsignedlongaddr;unsignedlongr2;};
@@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr)}staticunsignedlongfunc_addr(unsignedlongaddr){-returnfunc_desc(addr).funcaddr;+returnfunc_desc(addr).addr;}staticunsignedlongstub_func_addr(func_desc_tfunc){-returnfunc.funcaddr;+returnfunc.addr;}staticunsignedintlocal_entry_offset(constElf64_Sym*sym){
@@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y)staticunsignedlongget_stubs_size(constElf64_Ehdr*hdr,constElf64_Shdr*sechdrs){-/* One extra reloc so it's always 0-funcaddr terminated */+/* One extra reloc so it's always 0-addr terminated */unsignedlongrelocs=1;unsignedi;
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. Add to add a fake empty
inline assembly to force GCC to reload the function descriptor.
Signed-off-by: Christophe Leroy <redacted>
---
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/lkdtm.h | 1 +
drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++
3 files changed, 24 insertions(+)
@@ -44,6 +44,11 @@ static noinline void do_overwritten(void)return;}+staticnoinlinevoiddo_almost_nothing(void)+{+pr_info("do_nothing was hijacked!\n");+}+staticvoid*setup_function_descriptor(func_desc_t*fdesc,void*dst){memcpy(fdesc,do_nothing,sizeof(*fdesc));
@@ -143,6 +148,23 @@ void lkdtm_WRITE_KERN(void)do_overwritten();}+voidlkdtm_WRITE_OPD(void)+{+size_tsize=sizeof(func_desc_t);+void(*func)(void)=do_nothing;++if(!have_function_descriptors()){+pr_info("Platform doesn't have 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();+}+voidlkdtm_EXEC_DATA(void){execute_location(data_area,CODE_WRITE);
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.
It replaces the similar func_desc_t previously defined in
arch/powerpc/kernel/module_64.c
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
arch/ia64/include/asm/sections.h | 1 +
arch/parisc/include/asm/sections.h | 2 ++
arch/powerpc/include/asm/sections.h | 1 +
arch/powerpc/kernel/module_64.c | 8 --------
include/asm-generic/sections.h | 3 +++
5 files changed, 7 insertions(+), 8 deletions(-)
@@ -3,7 +3,9 @@#define _PARISC_SECTIONS_H#ifdef CONFIG_64BIT+#include<asm/elf.h>#define HAVE_FUNCTION_DESCRIPTORS 1+typedefElf64_Fdescfunc_desc_t;#endif/* nothing to see, move along */
@@ -32,11 +32,6 @@#ifdef PPC64_ELF_ABI_v2-/* An address is simply the address of the function. */-typedefstruct{-unsignedlongaddr;-}func_desc_t;-staticfunc_desc_tfunc_desc(unsignedlongaddr){return(func_desc_t){addr};
@@ -57,9 +52,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. */-typedefstructppc64_opd_entryfunc_desc_t;-staticfunc_desc_tfunc_desc(unsignedlongaddr){return*(func_desc_t*)addr;
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 <redacted>
---
drivers/misc/lkdtm/perms.c | 25 +++++++++++++++++++++----
include/asm-generic/sections.h | 5 +++++
2 files changed, 26 insertions(+), 4 deletions(-)
@@ -44,19 +44,32 @@ static noinline void do_overwritten(void)return;}+staticvoid*setup_function_descriptor(func_desc_t*fdesc,void*dst)+{+memcpy(fdesc,do_nothing,sizeof(*fdesc));+fdesc->addr=(unsignedlong)dst;+barrier();++returnfdesc;+}+staticnoinlinevoidexecute_location(void*dst,boolwrite){void(*func)(void)=dst;+func_desc_tfdesc;+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((unsignedlong)dst,(unsignedlong)dst+EXEC_SIZE);}pr_info("attempting bad execution at %px\n",func);+if(have_function_descriptors())+func=setup_function_descriptor(&fdesc,dst);func();pr_err("FAIL: func returned\n");}
@@ -67,15 +80,19 @@ static void execute_user_location(void *dst)/* Intentionally crossing kernel/user memory boundary. */void(*func)(void)=dst;+func_desc_tfdesc;+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,(unsignedlong)dst,do_nothing,+copied=access_process_vm(current,(unsignedlong)dst,do_nothing_text,EXEC_SIZE,FOLL_WRITE);if(copied<EXEC_SIZE)return;pr_info("attempting bad execution at %px\n",func);+if(have_function_descriptors())+func=setup_function_descriptor(&fdesc,dst);func();pr_err("FAIL: func returned\n");}
Replace HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR by
HAVE_FUNCTION_DESCRIPTORS and use it instead of
'dereference_function_descriptor' macro to know
whether an arch has function descriptors.
To limit churn in one of the following patches, use
an #ifdef/#else construct with empty first part
instead of an #ifndef in asm-generic/sections.h
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
arch/ia64/include/asm/sections.h | 5 +++--
arch/parisc/include/asm/sections.h | 6 ++++--
arch/powerpc/include/asm/sections.h | 6 ++++--
include/asm-generic/sections.h | 3 ++-
include/linux/kallsyms.h | 2 +-
5 files changed, 14 insertions(+), 8 deletions(-)
@@ -2,6 +2,10 @@#ifndef _PARISC_SECTIONS_H#define _PARISC_SECTIONS_H+#ifdef CONFIG_64BIT+#define HAVE_FUNCTION_DESCRIPTORS 1+#endif+/* nothing to see, move along */#include<asm-generic/sections.h>
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 <redacted>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Christophe Leroy <redacted>
---
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(-)
@@ -159,12 +160,32 @@ int kernel_text_address(unsigned long addr)}/*-*Onsomearchitectures(PPC64,IA64)functionpointers+*Onsomearchitectures(PPC64,IA64,PARISC)functionpointers*areactuallyonlytokenstosomedatathatthenholdsthe*realfunctionaddress.Asaresult,tofindifafunction*pointerispartofthekerneltext,weneedtodosome*specialdereferencingfirst.*/+#ifdef 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;+returnptr;+}++void*dereference_kernel_function_descriptor(void*ptr)+{+if(ptr<(void*)__start_opd||ptr>=(void*)__end_opd)+returnptr;++returndereference_function_descriptor(ptr);+}+#endif+intfunc_ptr_is_kernel_text(void*ptr){unsignedlongaddr;
On Thu, Oct 14, 2021 at 7:49 AM Christophe Leroy
[off-list ref] 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,
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 <redacted>
Signed-off-by: Christophe Leroy <redacted>
From: Daniel Axtens <hidden> Date: 2021-10-14 21:26:30
Hi Christophe,
'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
As Michael said on v1, I'm a little nervous about moving it out of uAPI
after so long, although I do take the points of Arnd and Kees that we're
not breaking compiled binaries, nor should people be using this struct
to begin with...
I've cc:ed the linux-api@ list.
Kind regards,
Daniel
quoted hunk
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")
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/include/asm/elf.h | 6 ++++++
arch/powerpc/include/uapi/asm/elf.h | 8 --------
2 files changed, 6 insertions(+), 8 deletions(-)
@@ -176,4 +176,10 @@ do { \/* Relocate the kernel image to @final_address */voidrelocate(unsignedlongfinal_address);+/* There's actually a third entry here, but it's unused */+structppc64_opd_entry{+unsignedlongfuncaddr;+unsignedlongr2;+};+#endif /* _ASM_POWERPC_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 */-structppc64_opd_entry-{-unsignedlongfuncaddr;-unsignedlongr2;-};--#endif /* _UAPI_ASM_POWERPC_ELF_H */
From: Daniel Axtens <hidden> Date: 2021-10-14 21:36:05
Christophe Leroy [off-list ref] writes:
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 8 is not absolutely necessary but it is a good trivial cleanup.
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: Move 'struct ppc64_opd_entry' back into asm/elf.h
powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'
powerpc: Remove func_descr_t
powerpc: Prepare func_desc_t for refactorisation
ia64: Rename 'ip' to 'addr' in 'struct fdesc'
asm-generic: Use HAVE_FUNCTION_DESCRIPTORS to define associated stubs
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 lkdtm_EXEC_RODATA()
lkdtm: Fix execute_[user]_location()
lkdtm: Add a test for function descriptors protection
arch/ia64/include/asm/elf.h | 2 +-
arch/ia64/include/asm/sections.h | 25 ++-------
arch/ia64/kernel/module.c | 6 +--
arch/parisc/include/asm/sections.h | 17 +++---
arch/parisc/kernel/process.c | 21 --------
arch/powerpc/include/asm/code-patching.h | 2 +-
arch/powerpc/include/asm/elf.h | 6 +++
arch/powerpc/include/asm/sections.h | 30 ++---------
arch/powerpc/include/asm/types.h | 6 ---
arch/powerpc/include/uapi/asm/elf.h | 8 ---
arch/powerpc/kernel/module_64.c | 38 +++++--------
arch/powerpc/kernel/signal_64.c | 8 +--
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/lkdtm.h | 1 +
drivers/misc/lkdtm/perms.c | 68 ++++++++++++++++++++----
include/asm-generic/sections.h | 13 ++++-
include/linux/kallsyms.h | 2 +-
kernel/extable.c | 23 +++++++-
18 files changed, 138 insertions(+), 139 deletions(-)
--
2.31.1
From: Daniel Axtens <hidden> Date: 2021-10-14 21:45:29
Christophe Leroy [off-list ref] writes:
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.
I would have picked 'funcaddr', but at least 'addr' is better than 'ip'!
And I agree that consistency, and then making things generic is worthwhile.
I grepped the latest powerpc/next for uses of 'funcaddr'. There were 5,
your patch changes all 5.
The series passes build tests and this patch has no checkpatch or other
style concerns.
On that basis:
Reviewed-by: Daniel Axtens <redacted>
Kind regards,
Daniel
@@ -178,7 +178,7 @@ void relocate(unsigned long final_address);/* There's actually a third entry here, but it's unused */structppc64_opd_entry{-unsignedlongfuncaddr;+unsignedlongaddr;unsignedlongr2;};
@@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr)}staticunsignedlongfunc_addr(unsignedlongaddr){-returnfunc_desc(addr).funcaddr;+returnfunc_desc(addr).addr;}staticunsignedlongstub_func_addr(func_desc_tfunc){-returnfunc.funcaddr;+returnfunc.addr;}staticunsignedintlocal_entry_offset(constElf64_Sym*sym){
@@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y)staticunsignedlongget_stubs_size(constElf64_Ehdr*hdr,constElf64_Shdr*sechdrs){-/* One extra reloc so it's always 0-funcaddr terminated */+/* One extra reloc so it's always 0-addr terminated */unsignedlongrelocs=1;unsignedi;
From: Daniel Axtens <hidden> Date: 2021-10-14 22:17:40
Christophe Leroy [off-list ref] writes:
'func_descr_t' is redundant with 'struct ppc64_opd_entry'
So, if I understand the overall direction of the series, you're
consolidating powerpc around one single type for function descriptors,
and then you're creating a generic typedef so that generic code can
always do ((func_desc_t)x)->addr to get the address of a function out of
a function descriptor regardless of arch. (And regardless of whether the
arch uses function descriptors or not.)
So:
- why pick ppc64_opd_entry over func_descr_t?
- Why not make our struct just called func_desc_t - why have a
ppc64_opd_entry type or a func_descr_t typedef?
- Should this patch wait until after you've made the generic
func_desc_t change and move directly to that new interface? (rather
than move from func_descr_t -> ppc64_opd_entry -> ...) Or is there a
particular reason arch specific code should use an arch-specific
struct or named type?
I was a little concerned about going from a 3-element struct to a
2-element struct (as ppc64_opd_entry doesn't have an element for env) -
but we don't seem to take the sizeof this anywhere, nor do we use env
anywhere, nor do we do funky macro stuff with it in the signal handling
code that might implictly use the 3rd element, so I guess this will
work. Still, func_descr_t seems to describe the underlying ABI better
than ppc64_opd_entry...
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.
I would have picked 'funcaddr', but at least 'addr' is better than 'ip'!
And I agree that consistency, and then making things generic is worthwhile.
It's a function descriptor, there is only one address field, I don't
think there is any ambiguïty here, and I prefer modifying the least
impacted architectures.
Changing addr to funcaddr in PARISC would result in the following
changes, on an architecture I know nothing about. It's more changes than
we have on powerpc.
arch/parisc/include/asm/elf.h | 4 ++--
arch/parisc/kernel/kexec.c | 2 +-
arch/parisc/kernel/module.c | 12 ++++++------
arch/parisc/kernel/process.c | 2 +-
arch/parisc/kernel/signal.c | 4 ++--
5 files changed, 12 insertions(+), 12 deletions(-)
I grepped the latest powerpc/next for uses of 'funcaddr'. There were 5,
your patch changes all 5.
The series passes build tests and this patch has no checkpatch or other
style concerns.
On that basis:
Reviewed-by: Daniel Axtens <redacted>
Kind regards,
Daniel
@@ -178,7 +178,7 @@ void relocate(unsigned long final_address);/* There's actually a third entry here, but it's unused */structppc64_opd_entry{-unsignedlongfuncaddr;+unsignedlongaddr;unsignedlongr2;};
@@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr)}staticunsignedlongfunc_addr(unsignedlongaddr){-returnfunc_desc(addr).funcaddr;+returnfunc_desc(addr).addr;}staticunsignedlongstub_func_addr(func_desc_tfunc){-returnfunc.funcaddr;+returnfunc.addr;}staticunsignedintlocal_entry_offset(constElf64_Sym*sym){
@@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y)staticunsignedlongget_stubs_size(constElf64_Ehdr*hdr,constElf64_Shdr*sechdrs){-/* One extra reloc so it's always 0-funcaddr terminated */+/* One extra reloc so it's always 0-addr terminated */unsignedlongrelocs=1;unsignedi;
'func_descr_t' is redundant with 'struct ppc64_opd_entry'
So, if I understand the overall direction of the series, you're
consolidating powerpc around one single type for function descriptors,
and then you're creating a generic typedef so that generic code can
always do ((func_desc_t)x)->addr to get the address of a function out of
a function descriptor regardless of arch. (And regardless of whether the
arch uses function descriptors or not.)
An architecture not using function descriptors won't do much with
((func_desc_t *)x)->addr. This is just done to allow building stuff
regardless.
I prefer something like
if (have_function_descriptors())
addr = (func_desc_t *)ptr)->addr;
else
addr = ptr;
over
#ifdef HAVE_FUNCTION_DESCRIPTORS
addr = (func_desc_t *)ptr)->addr;
#else
addr = ptr;
#endif
So:
- why pick ppc64_opd_entry over func_descr_t?
Good question. At the begining it was because it was in UAPI headers,
and also because it was the one used in our
dereference_function_descriptor().
But at the end maybe that's not the more logical choice. I need to look
a bit more.
- Why not make our struct just called func_desc_t - why have a
ppc64_opd_entry type or a func_descr_t typedef?
Well ... you usually don't flag a struct name with _t, _t will most of
the time refer to a typedef.
If I want to avoid typedef (I know they are deprecated in kernel coding
stype), it means the name of the struct must be changed in every
architecture and it becomes tricky and it adds more churn in them, which
is what I want to avoid.
At the end we risk to end-up with a messy set of #ifdefs.
Maybe this can be done as a second step, but I would like to minimise
impact in this series and focus on fixing lkdtm.
- Should this patch wait until after you've made the generic
func_desc_t change and move directly to that new interface? (rather
than move from func_descr_t -> ppc64_opd_entry -> ...) Or is there a
particular reason arch specific code should use an arch-specific
struct or named type?
I was a little concerned about going from a 3-element struct to a
2-element struct (as ppc64_opd_entry doesn't have an element for env) -
but we don't seem to take the sizeof this anywhere, nor do we use env
anywhere, nor do we do funky macro stuff with it in the signal handling
code that might implictly use the 3rd element, so I guess this will
work. Still, func_descr_t seems to describe the underlying ABI better
than ppc64_opd_entry...
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-10-15 05:58:05
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm:
quoted hunk
'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")
Reviewed-by: Kees Cook <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/include/asm/elf.h | 6 ++++++
arch/powerpc/include/uapi/asm/elf.h | 8 --------
2 files changed, 6 insertions(+), 8 deletions(-)
@@ -176,4 +176,10 @@ do { \/* Relocate the kernel image to @final_address */voidrelocate(unsignedlongfinal_address);+/* There's actually a third entry here, but it's unused */+structppc64_opd_entry{+unsignedlongfuncaddr;+unsignedlongr2;+};
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
I wonder if we should add that third entry, just for completeness. And
'r2' isn't a good name should probably be toc. And should it be packed?
At any rate that's not for your series, a cleanup I might think about
for later.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-10-15 06:01:36
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm:
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.
It is the "address of the entry point of the function" according to
powerpc ELF spec, so addr seems fine.
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
@@ -178,7 +178,7 @@ void relocate(unsigned long final_address);/* There's actually a third entry here, but it's unused */structppc64_opd_entry{-unsignedlongfuncaddr;+unsignedlongaddr;unsignedlongr2;};
@@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr)}staticunsignedlongfunc_addr(unsignedlongaddr){-returnfunc_desc(addr).funcaddr;+returnfunc_desc(addr).addr;}staticunsignedlongstub_func_addr(func_desc_tfunc){-returnfunc.funcaddr;+returnfunc.addr;}staticunsignedintlocal_entry_offset(constElf64_Sym*sym){
@@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y)staticunsignedlongget_stubs_size(constElf64_Ehdr*hdr,constElf64_Shdr*sechdrs){-/* One extra reloc so it's always 0-funcaddr terminated */+/* One extra reloc so it's always 0-addr terminated */unsignedlongrelocs=1;unsignedi;
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-10-15 06:11:42
Excerpts from Christophe Leroy's message of October 15, 2021 3:19 pm:
Le 15/10/2021 à 00:17, Daniel Axtens a écrit :
quoted
Christophe Leroy [off-list ref] writes:
quoted
'func_descr_t' is redundant with 'struct ppc64_opd_entry'
So, if I understand the overall direction of the series, you're
consolidating powerpc around one single type for function descriptors,
and then you're creating a generic typedef so that generic code can
always do ((func_desc_t)x)->addr to get the address of a function out of
a function descriptor regardless of arch. (And regardless of whether the
arch uses function descriptors or not.)
An architecture not using function descriptors won't do much with
((func_desc_t *)x)->addr. This is just done to allow building stuff
regardless.
I prefer something like
if (have_function_descriptors())
addr = (func_desc_t *)ptr)->addr;
else
addr = ptr;
If you make a generic data type for architectures without function
descriptors as such
typedef struct func_desc {
char addr[0];
} func_desc_t;
Then you can do that with no if. The downside is your addr has to be
char * and it's maybe not helpful to be so "clever".
quoted
- why pick ppc64_opd_entry over func_descr_t?
Good question. At the begining it was because it was in UAPI headers,
and also because it was the one used in our
dereference_function_descriptor().
But at the end maybe that's not the more logical choice. I need to look
a bit more.
I would prefer the func_descr_t (with 'toc' and 'env') if you're going
to change it.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-10-15 06:16:23
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm:
Replace HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR by
HAVE_FUNCTION_DESCRIPTORS and use it instead of
'dereference_function_descriptor' macro to know
whether an arch has function descriptors.
To limit churn in one of the following patches, use
an #ifdef/#else construct with empty first part
instead of an #ifndef in asm-generic/sections.h
Is it worth putting this into Kconfig if you're going to
change it? In any case
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
@@ -2,6 +2,10 @@#ifndef _PARISC_SECTIONS_H#define _PARISC_SECTIONS_H+#ifdef CONFIG_64BIT+#define HAVE_FUNCTION_DESCRIPTORS 1+#endif+/* nothing to see, move along */#include<asm-generic/sections.h>
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm:
quoted
Replace HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR by
HAVE_FUNCTION_DESCRIPTORS and use it instead of
'dereference_function_descriptor' macro to know
whether an arch has function descriptors.
To limit churn in one of the following patches, use
an #ifdef/#else construct with empty first part
instead of an #ifndef in asm-generic/sections.h
Is it worth putting this into Kconfig if you're going to
change it? In any case
That was what I wanted to do in the begining but how can I do that in
Kconfig ?
#ifdef __powerpc64__
#if defined(_CALL_ELF) && _CALL_ELF == 2
#define PPC64_ELF_ABI_v2
#else
#define PPC64_ELF_ABI_v1
#endif
#endif /* __powerpc64__ */
#ifdef PPC64_ELF_ABI_v1
#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
Christophe
@@ -2,6 +2,10 @@#ifndef _PARISC_SECTIONS_H#define _PARISC_SECTIONS_H+#ifdef CONFIG_64BIT+#define HAVE_FUNCTION_DESCRIPTORS 1+#endif+/* nothing to see, move along */#include<asm-generic/sections.h>
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-10-15 07:00:21
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm:
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.
We should be moving more stuff out of extable.c (including all the
kernel address tests). lib/kimage.c or kelf.c or something.
It could be after your series though.
@@ -159,12 +160,32 @@ int kernel_text_address(unsigned long addr)}/*-*Onsomearchitectures(PPC64,IA64)functionpointers+*Onsomearchitectures(PPC64,IA64,PARISC)functionpointers*areactuallyonlytokenstosomedatathatthenholdsthe*realfunctionaddress.Asaresult,tofindifafunction*pointerispartofthekerneltext,weneedtodosome*specialdereferencingfirst.*/+#ifdef 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;
I know you're just copying existing code. This seems a bit risky though.
I don't think anything good could come of just treating the descriptor
address like a function entry address if we failed to load from it for
whatever reason.
Existing callers might be benign but the API is not good. It should
give a nice fail return or BUG. If we change that then we should also
change the name and pass the correct type to it too.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-10-15 08:03:47
Excerpts from Christophe Leroy's message of October 15, 2021 4:24 pm:
Le 15/10/2021 à 08:16, Nicholas Piggin a écrit :
quoted
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm:
quoted
Replace HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR by
HAVE_FUNCTION_DESCRIPTORS and use it instead of
'dereference_function_descriptor' macro to know
whether an arch has function descriptors.
To limit churn in one of the following patches, use
an #ifdef/#else construct with empty first part
instead of an #ifndef in asm-generic/sections.h
Is it worth putting this into Kconfig if you're going to
change it? In any case
That was what I wanted to do in the begining but how can I do that in
Kconfig ?
#ifdef __powerpc64__
#if defined(_CALL_ELF) && _CALL_ELF == 2
#define PPC64_ELF_ABI_v2
#else
#define PPC64_ELF_ABI_v1
#endif
#endif /* __powerpc64__ */
#ifdef PPC64_ELF_ABI_v1
#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
We have ELFv2 ABI / function descriptors iff big-endian so you could
just select based on that.
I have a patch that makes the ABI version configurable which cleans
some of this up a bit, but that can be rebased on your series if we
ever merge it. Maybe just add BUILD_BUG_ONs in the above ifdef block
to ensure CONFIG_HAVE_FUNCTION_DESCRIPTORS was set the right way, so
I don't forget.
Thanks,
Nick
From: Nicholas Piggin <npiggin@gmail.com> Date: 2021-10-15 11:52:54
Excerpts from Nicholas Piggin's message of October 15, 2021 6:02 pm:
Excerpts from Christophe Leroy's message of October 15, 2021 4:24 pm:
quoted
Le 15/10/2021 à 08:16, Nicholas Piggin a écrit :
quoted
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm:
quoted
Replace HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR by
HAVE_FUNCTION_DESCRIPTORS and use it instead of
'dereference_function_descriptor' macro to know
whether an arch has function descriptors.
To limit churn in one of the following patches, use
an #ifdef/#else construct with empty first part
instead of an #ifndef in asm-generic/sections.h
Is it worth putting this into Kconfig if you're going to
change it? In any case
That was what I wanted to do in the begining but how can I do that in
Kconfig ?
#ifdef __powerpc64__
#if defined(_CALL_ELF) && _CALL_ELF == 2
#define PPC64_ELF_ABI_v2
#else
#define PPC64_ELF_ABI_v1
#endif
#endif /* __powerpc64__ */
#ifdef PPC64_ELF_ABI_v1
#define HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR 1
We have ELFv2 ABI / function descriptors iff big-endian so you could
just select based on that.
Of course that should read ELFv1. To be clearer: BE is ELFv1 ABI and
LE is ELFv2 ABI.
Thanks,
Nick
On Thu, Oct 14, 2021 at 07:50:01AM +0200, Christophe Leroy wrote:
quoted hunk
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 <redacted>
---
drivers/misc/lkdtm/perms.c | 25 +++++++++++++++++++++----
include/asm-generic/sections.h | 5 +++++
2 files changed, 26 insertions(+), 4 deletions(-)
On Thu, Oct 14, 2021 at 07:50:00AM +0200, Christophe Leroy wrote:
Behind its location, lkdtm_EXEC_RODATA() executes
lkdtm_rodata_do_nothing() which is a real function,
not a copy of do_nothing().
So executes it directly instead of using execute_location().
This is necessary because following patch will fix execute_location()
to use a copy of the function descriptor of do_nothing() and
function descriptor of lkdtm_rodata_do_nothing() might be different.
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy <redacted>
I still don't understand this -- it doesn't look needed at all given the
changes in patch 12. (i.e. everything is using
dereference_function_descriptor() now)
Can't this patch be dropped?
-Kees
@@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)voidlkdtm_EXEC_RODATA(void){-execute_location(lkdtm_rodata_do_nothing,CODE_AS_IS);+pr_info("attempting ok execution at %px\n",+dereference_function_descriptor(do_nothing));+do_nothing();++pr_info("attempting bad execution at %px\n",+dereference_function_descriptor(lkdtm_rodata_do_nothing));+lkdtm_rodata_do_nothing();+pr_err("FAIL: func returned\n");}voidlkdtm_EXEC_USERSPACE(void)
On Thu, Oct 14, 2021 at 07:50:02AM +0200, Christophe Leroy wrote:
quoted hunk
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. Add to add a fake empty
inline assembly to force GCC to reload the function descriptor.
Signed-off-by: Christophe Leroy <redacted>
---
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/lkdtm.h | 1 +
drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++
3 files changed, 24 insertions(+)
@@ -44,6 +44,11 @@ static noinline void do_overwritten(void)return;}+staticnoinlinevoiddo_almost_nothing(void)+{+pr_info("do_nothing was hijacked!\n");+}+staticvoid*setup_function_descriptor(func_desc_t*fdesc,void*dst){memcpy(fdesc,do_nothing,sizeof(*fdesc));
@@ -143,6 +148,23 @@ void lkdtm_WRITE_KERN(void)do_overwritten();}+voidlkdtm_WRITE_OPD(void)+{+size_tsize=sizeof(func_desc_t);+void(*func)(void)=do_nothing;++if(!have_function_descriptors()){+pr_info("Platform doesn't have function descriptors.\n");
This should be more explicit ('xfail'):
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));
Since this is a descriptor, I assume no icache flush is needed. Are
function descriptors strictly dcache? (Is anything besides just a
barrier needed?)
On Thu, Oct 14, 2021 at 07:50:02AM +0200, Christophe Leroy wrote:
quoted
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. Add to add a fake empty
inline assembly to force GCC to reload the function descriptor.
Signed-off-by: Christophe Leroy <redacted>
---
drivers/misc/lkdtm/core.c | 1 +
drivers/misc/lkdtm/lkdtm.h | 1 +
drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++
3 files changed, 24 insertions(+)
@@ -44,6 +44,11 @@ static noinline void do_overwritten(void)return;}+staticnoinlinevoiddo_almost_nothing(void)+{+pr_info("do_nothing was hijacked!\n");+}+staticvoid*setup_function_descriptor(func_desc_t*fdesc,void*dst){memcpy(fdesc,do_nothing,sizeof(*fdesc));
@@ -143,6 +148,23 @@ void lkdtm_WRITE_KERN(void)do_overwritten();}+voidlkdtm_WRITE_OPD(void)+{+size_tsize=sizeof(func_desc_t);+void(*func)(void)=do_nothing;++if(!have_function_descriptors()){+pr_info("Platform doesn't have function descriptors.\n");
This should be more explicit ('xfail'):
pr_info("XFAIL: platform doesn't use function descriptors.\n");
Ok
quoted
+ 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));
Since this is a descriptor, I assume no icache flush is needed. Are
function descriptors strictly dcache? (Is anything besides just a
barrier needed?)
No flush is needed, the code just loads the function address from memory
into CTR, loads R2 and branch to CTR:
19c: e9 21 00 70 ld r9,112(r1)
1a0: e9 49 00 00 ld r10,0(r9)
1a4: 7d 49 03 a6 mtctr r10
1a8: e8 49 00 08 ld r2,8(r9)
1ac: 4e 80 04 21 bctrl
On Thu, Oct 14, 2021 at 07:50:00AM +0200, Christophe Leroy wrote:
quoted
Behind its location, lkdtm_EXEC_RODATA() executes
lkdtm_rodata_do_nothing() which is a real function,
not a copy of do_nothing().
So executes it directly instead of using execute_location().
This is necessary because following patch will fix execute_location()
to use a copy of the function descriptor of do_nothing() and
function descriptor of lkdtm_rodata_do_nothing() might be different.
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy <redacted>
I still don't understand this -- it doesn't look needed at all given the
changes in patch 12. (i.e. everything is using
dereference_function_descriptor() now)
dereference_function_descriptor() only deals with the function address,
not the function TOC.
do_nothing() is a function. It has a function descriptor with a given
address (address of .do_nothing) and a given TOC, say TOC1.
lkdtm_rodata_do_nothing() is another function. It has its own function
descriptor with a given address (address of .lkdtm_rodata_do_nothing)
and a given TOC, say TOC2.
If we use execute_location(), it will copy do_nothing() function
descriptor and change the function address to the address of
lkdtm_rodata_do_nothing(). So it will call lkdtm_rodata_do_nothing()
with TOC1 instead of calling it with TOC2.
Can't this patch be dropped?
It is likely that the TOC will be the same for both functions, and
anyway those functions are so simple that they don't use the TOC at all,
so yes it would likely work without this patch but from my point of view
it is incorrect to call one function with the TOC from the descriptor of
another function.
If you thing we can take the risk, then I'm happy to drop the patch and
replace it by
execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing), CODE_AS_IS)
Christophe
On Thu, Oct 14, 2021 at 07:50:01AM +0200, Christophe Leroy wrote:
quoted
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 <redacted>
---
drivers/misc/lkdtm/perms.c | 25 +++++++++++++++++++++----
include/asm-generic/sections.h | 5 +++++
2 files changed, 26 insertions(+), 4 deletions(-)
On Thu, Oct 14, 2021 at 07:50:00AM +0200, Christophe Leroy wrote:
quoted
Behind its location, lkdtm_EXEC_RODATA() executes
lkdtm_rodata_do_nothing() which is a real function,
not a copy of do_nothing().
So executes it directly instead of using execute_location().
This is necessary because following patch will fix execute_location()
to use a copy of the function descriptor of do_nothing() and
function descriptor of lkdtm_rodata_do_nothing() might be different.
And fix displayed addresses by dereferencing the function descriptors.
Signed-off-by: Christophe Leroy <redacted>
I still don't understand this -- it doesn't look needed at all given the
changes in patch 12. (i.e. everything is using
dereference_function_descriptor() now)
dereference_function_descriptor() only deals with the function address,
not the function TOC.
do_nothing() is a function. It has a function descriptor with a given
address (address of .do_nothing) and a given TOC, say TOC1.
lkdtm_rodata_do_nothing() is another function. It has its own function
descriptor with a given address (address of .lkdtm_rodata_do_nothing)
and a given TOC, say TOC2.
If we use execute_location(), it will copy do_nothing() function
descriptor and change the function address to the address of
lkdtm_rodata_do_nothing(). So it will call lkdtm_rodata_do_nothing()
with TOC1 instead of calling it with TOC2.
quoted
Can't this patch be dropped?
It is likely that the TOC will be the same for both functions, and
anyway those functions are so simple that they don't use the TOC at all,
so yes it would likely work without this patch but from my point of view
it is incorrect to call one function with the TOC from the descriptor of
another function.
If you thing we can take the risk, then I'm happy to drop the patch and
replace it by
execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing), CODE_AS_IS)
Once we have patch 12 EXEC_RODATA works well on powerpc without this
patch so I will drop this patch for now and will propose something else
as a follow-up to my series.
Christophe
Hi Kees,
Le 16/10/2021 à 08:42, Christophe Leroy a écrit :
Le 15/10/2021 à 23:31, Kees Cook a écrit :
quoted
On Thu, Oct 14, 2021 at 07:50:01AM +0200, Christophe Leroy wrote:
quoted
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 <redacted>
---
drivers/misc/lkdtm/perms.c | 25 +++++++++++++++++++++----
include/asm-generic/sections.h | 5 +++++
2 files changed, 26 insertions(+), 4 deletions(-)
} func_desc_t;
#endif
+static inline bool have_function_descriptors(void)
+{
+ return __is_defined(HAVE_FUNCTION_DESCRIPTORS);
+}
+
/* random extra sections (if any). Override
* in asm/sections.h */
#ifndef arch_is_kernel_text
This hunk seems like it should live in a separate patch.
Ok I move it in a previous patch.
Do you have any additional feedback or comment on series v3 ?
What's the way forward, should it go via LKDTM tree or via powerpc tree
or another tree ? I see there are neither Ack-by nor Reviewed-by for the
last 2 patches.
Thanks
Christophe