| Message ID | 86c393ce0a6f603f94e6d2ceca08d535f654bb23.1634457599.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix LKDTM for PPC64/IA64/PARISC | expand |
| Related | show |
Excerpts from Christophe Leroy's message of October 17, 2021 10:38 pm: > In preparation of making func_desc_t generic, change the ELFv2 > version to a struct containing 'addr' element. > > This allows using single helpers common to ELFv1 and ELFv2. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/kernel/module_64.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index a89da0ee25e2..b687ef88c4c4 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -33,19 +33,13 @@ > #ifdef PPC64_ELF_ABI_v2 > > /* An address is simply the address of the function. */ > -typedef unsigned long func_desc_t; > +typedef struct { > + unsigned long addr; > +} func_desc_t; I'm not quite following why this change is done. I guess it is so you can move this func_desc_t type into core code, but why do that? Is it just to avoid using the preprocessor? On its own this patch looks okay. Acked-by: Nicholas Piggin <npiggin@gmail.com>
Le 18/10/2021 à 08:27, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of October 17, 2021 10:38 pm: >> In preparation of making func_desc_t generic, change the ELFv2 >> version to a struct containing 'addr' element. >> >> This allows using single helpers common to ELFv1 and ELFv2. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >> --- >> arch/powerpc/kernel/module_64.c | 32 ++++++++++++++------------------ >> 1 file changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c >> index a89da0ee25e2..b687ef88c4c4 100644 >> --- a/arch/powerpc/kernel/module_64.c >> +++ b/arch/powerpc/kernel/module_64.c >> @@ -33,19 +33,13 @@ >> #ifdef PPC64_ELF_ABI_v2 >> >> /* An address is simply the address of the function. */ >> -typedef unsigned long func_desc_t; >> +typedef struct { >> + unsigned long addr; >> +} func_desc_t; > > I'm not quite following why this change is done. I guess it is so you > can move this func_desc_t type into core code, but why do that? Is it > just to avoid using the preprocessor? I explained it in patch 7 but yes it probably also deserves some more explanation here as well. That's right, it's to avoid having to spread #ifdefs everywhere. > > On its own this patch looks okay. > > Acked-by: Nicholas Piggin <npiggin@gmail.com> >
On Sun, Oct 17, 2021 at 02:38:17PM +0200, Christophe Leroy wrote: > In preparation of making func_desc_t generic, change the ELFv2 > version to a struct containing 'addr' element. > > This allows using single helpers common to ELFv1 and ELFv2. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/kernel/module_64.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index a89da0ee25e2..b687ef88c4c4 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -33,19 +33,13 @@ > #ifdef PPC64_ELF_ABI_v2 > > /* An address is simply the address of the function. */ > -typedef unsigned long func_desc_t; > +typedef struct { > + unsigned long addr; > +} func_desc_t; > > static func_desc_t func_desc(unsigned long addr) > { > - return addr; > -} > -static unsigned long func_addr(unsigned long addr) > -{ > - return addr; > -} > -static unsigned long stub_func_addr(func_desc_t func) > -{ > - return func; > + return (func_desc_t){addr}; There's only 1 element in the struct, so okay, but it hurt my eyes a little. I would have been happier with: return (func_desc_t){ .addr = addr; }; But of course that also looks bonkers because it starts with "return". So no matter what I do my eyes bug out. ;) So it's fine either way. :) Reviewed-by: Kees Cook <keescook@chromium.org> > } > > /* PowerPC64 specific values for the Elf64_Sym st_other field. */ > @@ -70,14 +64,6 @@ static func_desc_t func_desc(unsigned long addr) > { > return *(struct func_desc *)addr; > } > -static unsigned long func_addr(unsigned long addr) > -{ > - return func_desc(addr).addr; > -} > -static unsigned long stub_func_addr(func_desc_t func) > -{ > - return func.addr; > -} > static unsigned int local_entry_offset(const Elf64_Sym *sym) > { > return 0; > @@ -93,6 +79,16 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr) > } > #endif > > +static unsigned long func_addr(unsigned long addr) > +{ > + return func_desc(addr).addr; > +} > + > +static unsigned long stub_func_addr(func_desc_t func) > +{ > + return func.addr; > +} > + > #define STUB_MAGIC 0x73747562 /* stub */ > > /* Like PPC32, we need little trampolines to do > 24-bit jumps (into > -- > 2.31.1 >
On Thu, Feb 10, 2022 at 04:54:52PM -0800, Kees Cook wrote: > On Sun, Oct 17, 2021 at 02:38:17PM +0200, Christophe Leroy wrote: (edited:) > > +typedef struct { > > + unsigned long addr; > > +} func_desc_t; > > > > static func_desc_t func_desc(unsigned long addr) > > { > > + return (func_desc_t){addr}; > There's only 1 element in the struct, so okay, but it hurt my eyes a > little. I would have been happier with: > > return (func_desc_t){ .addr = addr; }; > > But of course that also looks bonkers because it starts with "return". > So no matter what I do my eyes bug out. ;) The usual way to avoid convoluted constructs is to name more factors. So: static func_desc_t func_desc(unsigned long addr) { func_desc_t desc = {}; desc.addr = addr; return desc; } Segher
Le 11/02/2022 à 01:54, Kees Cook a écrit : > On Sun, Oct 17, 2021 at 02:38:17PM +0200, Christophe Leroy wrote: >> In preparation of making func_desc_t generic, change the ELFv2 >> version to a struct containing 'addr' element. >> >> This allows using single helpers common to ELFv1 and ELFv2. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/kernel/module_64.c | 32 ++++++++++++++------------------ >> 1 file changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c >> index a89da0ee25e2..b687ef88c4c4 100644 >> --- a/arch/powerpc/kernel/module_64.c >> +++ b/arch/powerpc/kernel/module_64.c >> @@ -33,19 +33,13 @@ >> #ifdef PPC64_ELF_ABI_v2 >> >> /* An address is simply the address of the function. */ >> -typedef unsigned long func_desc_t; >> +typedef struct { >> + unsigned long addr; >> +} func_desc_t; >> >> static func_desc_t func_desc(unsigned long addr) >> { >> - return addr; >> -} >> -static unsigned long func_addr(unsigned long addr) >> -{ >> - return addr; >> -} >> -static unsigned long stub_func_addr(func_desc_t func) >> -{ >> - return func; >> + return (func_desc_t){addr}; > > There's only 1 element in the struct, so okay, but it hurt my eyes a > little. I would have been happier with: > > return (func_desc_t){ .addr = addr; }; > > But of course that also looks bonkers because it starts with "return". > So no matter what I do my eyes bug out. ;) > > So it's fine either way. :) > > Reviewed-by: Kees Cook <keescook@chromium.org> I am going for: static func_desc_t func_desc(unsigned long addr) { + func_desc_t desc = { + .addr = addr, + }; + + return desc; } Thanks Christophe
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index a89da0ee25e2..b687ef88c4c4 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -33,19 +33,13 @@ #ifdef PPC64_ELF_ABI_v2 /* An address is simply the address of the function. */ -typedef unsigned long func_desc_t; +typedef struct { + unsigned long addr; +} func_desc_t; static func_desc_t func_desc(unsigned long addr) { - return addr; -} -static unsigned long func_addr(unsigned long addr) -{ - return addr; -} -static unsigned long stub_func_addr(func_desc_t func) -{ - return func; + return (func_desc_t){addr}; } /* PowerPC64 specific values for the Elf64_Sym st_other field. */ @@ -70,14 +64,6 @@ static func_desc_t func_desc(unsigned long addr) { return *(struct func_desc *)addr; } -static unsigned long func_addr(unsigned long addr) -{ - return func_desc(addr).addr; -} -static unsigned long stub_func_addr(func_desc_t func) -{ - return func.addr; -} static unsigned int local_entry_offset(const Elf64_Sym *sym) { return 0; @@ -93,6 +79,16 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr) } #endif +static unsigned long func_addr(unsigned long addr) +{ + return func_desc(addr).addr; +} + +static unsigned long stub_func_addr(func_desc_t func) +{ + return func.addr; +} + #define STUB_MAGIC 0x73747562 /* stub */ /* Like PPC32, we need little trampolines to do > 24-bit jumps (into
In preparation of making func_desc_t generic, change the ELFv2 version to a struct containing 'addr' element. This allows using single helpers common to ELFv1 and ELFv2. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- arch/powerpc/kernel/module_64.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-)