| Message ID | a33107c5b82580862510cc20af0d61e33a2b841d.1634457599.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix LKDTM for PPC64/IA64/PARISC | expand |
Excerpts from Christophe Leroy's message of October 17, 2021 10:38 pm: > We have three architectures using function descriptors, each with its > own type and name. > > Add a common typedef that can be used in generic code. > > Also add a stub typedef for architecture without function descriptors, > to avoid a forest of #ifdefs. > > It replaces the similar 'func_desc_t' previously defined in > arch/powerpc/kernel/module_64.c > > Reviewed-by: Kees Cook <keescook@chromium.org> > Acked-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- [...] > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h > index a918388d9bf6..33b51efe3a24 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; > +} func_desc_t; > #endif > I think that deserves a comment. If it's just to allow ifdef to be avoided, I guess that's okay with a comment. Would be nice if you could cause it to generate a link time error if it was ever used like undefined functions, but I guess you can't. It's not a necessity though. Thanks, Nick
Le 18/10/2021 à 08:29, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of October 17, 2021 10:38 pm: >> We have three architectures using function descriptors, each with its >> own type and name. >> >> Add a common typedef that can be used in generic code. >> >> Also add a stub typedef for architecture without function descriptors, >> to avoid a forest of #ifdefs. >> >> It replaces the similar 'func_desc_t' previously defined in >> arch/powerpc/kernel/module_64.c >> >> Reviewed-by: Kees Cook <keescook@chromium.org> >> Acked-by: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- > > [...] > >> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h >> index a918388d9bf6..33b51efe3a24 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; >> +} func_desc_t; >> #endif >> > > I think that deserves a comment. If it's just to allow ifdef to be > avoided, I guess that's okay with a comment. Would be nice if you could > cause it to generate a link time error if it was ever used like > undefined functions, but I guess you can't. It's not a necessity though. > I tried to explain it in the commit message, but I can add a comment here in addition for sure. By the way, it IS used in powerpc's module_64.c: static func_desc_t func_desc(unsigned long addr) { return (func_desc_t){addr}; } static unsigned long func_addr(unsigned long addr) { return func_desc(addr).addr; }
Excerpts from Christophe Leroy's message of October 18, 2021 5:07 pm: > > > Le 18/10/2021 à 08:29, Nicholas Piggin a écrit : >> Excerpts from Christophe Leroy's message of October 17, 2021 10:38 pm: >>> We have three architectures using function descriptors, each with its >>> own type and name. >>> >>> Add a common typedef that can be used in generic code. >>> >>> Also add a stub typedef for architecture without function descriptors, >>> to avoid a forest of #ifdefs. >>> >>> It replaces the similar 'func_desc_t' previously defined in >>> arch/powerpc/kernel/module_64.c >>> >>> Reviewed-by: Kees Cook <keescook@chromium.org> >>> Acked-by: Arnd Bergmann <arnd@arndb.de> >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>> --- >> >> [...] >> >>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h >>> index a918388d9bf6..33b51efe3a24 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; >>> +} func_desc_t; >>> #endif >>> >> >> I think that deserves a comment. If it's just to allow ifdef to be >> avoided, I guess that's okay with a comment. Would be nice if you could >> cause it to generate a link time error if it was ever used like >> undefined functions, but I guess you can't. It's not a necessity though. >> > > I tried to explain it in the commit message, but I can add a comment > here in addition for sure. Thanks. > > By the way, it IS used in powerpc's module_64.c: Ah yes of course. I guess the point is function descriptors don't exist so it should not be used (in general). powerpc module code knows what it is doing, I guess it's okay for it to use it. Thanks, Nick
diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h index 2460d365a057..3abe0562b01a 100644 --- a/arch/ia64/include/asm/sections.h +++ b/arch/ia64/include/asm/sections.h @@ -9,6 +9,9 @@ #include <linux/elf.h> #include <linux/uaccess.h> + +typedef struct fdesc func_desc_t; + #include <asm-generic/sections.h> extern char __phys_per_cpu_start[]; diff --git a/arch/parisc/include/asm/sections.h b/arch/parisc/include/asm/sections.h index c8092e4d94de..ace1d4047a0b 100644 --- a/arch/parisc/include/asm/sections.h +++ b/arch/parisc/include/asm/sections.h @@ -2,6 +2,11 @@ #ifndef _PARISC_SECTIONS_H #define _PARISC_SECTIONS_H +#ifdef CONFIG_HAVE_FUNCTION_DESCRIPTORS +#include <asm/elf.h> +typedef Elf64_Fdesc func_desc_t; +#endif + /* nothing to see, move along */ #include <asm-generic/sections.h> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h index fb11544d7e6a..1e6b6e732fb3 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 CONFIG_HAVE_FUNCTION_DESCRIPTORS +typedef struct func_desc func_desc_t; +#endif + #include <asm-generic/sections.h> extern bool init_mem_is_free; diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index b687ef88c4c4..3d06b996d504 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -32,11 +32,6 @@ #ifdef PPC64_ELF_ABI_v2 -/* An address is simply the address of the function. */ -typedef struct { - unsigned long addr; -} func_desc_t; - static func_desc_t func_desc(unsigned long addr) { 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. */ -typedef struct func_desc func_desc_t; - static func_desc_t func_desc(unsigned long addr) { return *(struct func_desc *)addr; diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index a918388d9bf6..33b51efe3a24 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; +} func_desc_t; #endif static inline bool have_function_descriptors(void)