| Message ID | cbee30c66890994e116a8eae8094fa8c5336f90a.1634190022.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
| Series | Fix LKDTM for PPC64/IA64/PARISC | expand |
On Thu, Oct 14, 2021 at 07:50:01AM +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 | 25 +++++++++++++++++++++---- > include/asm-generic/sections.h | 5 +++++ > 2 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c > index 5266dc28df6e..96b3ebfcb8ed 100644 > --- a/drivers/misc/lkdtm/perms.c > +++ b/drivers/misc/lkdtm/perms.c > @@ -44,19 +44,32 @@ static noinline void do_overwritten(void) > return; > } > > +static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) > +{ > + memcpy(fdesc, do_nothing, sizeof(*fdesc)); > + fdesc->addr = (unsigned long)dst; > + barrier(); > + > + return fdesc; > +} How about collapsing the "have_function_descriptors()" check into setup_function_descriptor()? static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) { if (__is_defined(HAVE_FUNCTION_DESCRIPTORS)) { memcpy(fdesc, do_nothing, sizeof(*fdesc)); fdesc->addr = (unsigned long)dst; barrier(); return fdesc; } else { return dst; } } > + > static noinline void execute_location(void *dst, bool write) > { > void (*func)(void) = dst; > + func_desc_t fdesc; > + void *do_nothing_text = dereference_function_descriptor(do_nothing); > > - pr_info("attempting ok execution at %px\n", do_nothing); > + pr_info("attempting ok execution at %px\n", do_nothing_text); > do_nothing(); > > if (write == CODE_WRITE) { > - memcpy(dst, do_nothing, EXEC_SIZE); > + memcpy(dst, do_nothing_text, EXEC_SIZE); > flush_icache_range((unsigned long)dst, > (unsigned long)dst + EXEC_SIZE); > } > pr_info("attempting bad execution at %px\n", func); > + 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_t fdesc; > + void *do_nothing_text = dereference_function_descriptor(do_nothing); > > - pr_info("attempting ok execution at %px\n", do_nothing); > + pr_info("attempting ok execution at %px\n", do_nothing_text); > do_nothing(); > > - copied = access_process_vm(current, (unsigned long)dst, do_nothing, > + copied = access_process_vm(current, (unsigned long)dst, do_nothing_text, > EXEC_SIZE, FOLL_WRITE); > if (copied < EXEC_SIZE) > return; > pr_info("attempting bad execution at %px\n", func); > + if (have_function_descriptors()) > + func = setup_function_descriptor(&fdesc, dst); > func(); > pr_err("FAIL: func returned\n"); > } > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h > index 76163883c6ff..d225318538bd 100644 > --- a/include/asm-generic/sections.h > +++ b/include/asm-generic/sections.h > @@ -70,6 +70,11 @@ typedef struct { > } 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.
Le 15/10/2021 à 23:31, Kees Cook a écrit : > On Thu, Oct 14, 2021 at 07:50:01AM +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 | 25 +++++++++++++++++++++---- >> include/asm-generic/sections.h | 5 +++++ >> 2 files changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >> index 5266dc28df6e..96b3ebfcb8ed 100644 >> --- a/drivers/misc/lkdtm/perms.c >> +++ b/drivers/misc/lkdtm/perms.c >> @@ -44,19 +44,32 @@ static noinline void do_overwritten(void) >> return; >> } >> >> +static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) >> +{ >> + memcpy(fdesc, do_nothing, sizeof(*fdesc)); >> + fdesc->addr = (unsigned long)dst; >> + barrier(); >> + >> + return fdesc; >> +} > > How about collapsing the "have_function_descriptors()" check into > setup_function_descriptor()? > > static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) > { > if (__is_defined(HAVE_FUNCTION_DESCRIPTORS)) { > memcpy(fdesc, do_nothing, sizeof(*fdesc)); > fdesc->addr = (unsigned long)dst; > barrier(); > return fdesc; > } else { > return dst; > } > } Ok > >> + >> static noinline void execute_location(void *dst, bool write) >> { >> void (*func)(void) = dst; >> + func_desc_t fdesc; >> + void *do_nothing_text = dereference_function_descriptor(do_nothing); >> >> - pr_info("attempting ok execution at %px\n", do_nothing); >> + pr_info("attempting ok execution at %px\n", do_nothing_text); >> do_nothing(); >> >> if (write == CODE_WRITE) { >> - memcpy(dst, do_nothing, EXEC_SIZE); >> + memcpy(dst, do_nothing_text, EXEC_SIZE); >> flush_icache_range((unsigned long)dst, >> (unsigned long)dst + EXEC_SIZE); >> } >> pr_info("attempting bad execution at %px\n", func); >> + 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_t fdesc; >> + void *do_nothing_text = dereference_function_descriptor(do_nothing); >> >> - pr_info("attempting ok execution at %px\n", do_nothing); >> + pr_info("attempting ok execution at %px\n", do_nothing_text); >> do_nothing(); >> >> - copied = access_process_vm(current, (unsigned long)dst, do_nothing, >> + copied = access_process_vm(current, (unsigned long)dst, do_nothing_text, >> EXEC_SIZE, FOLL_WRITE); >> if (copied < EXEC_SIZE) >> return; >> pr_info("attempting bad execution at %px\n", func); >> + if (have_function_descriptors()) >> + func = setup_function_descriptor(&fdesc, dst); >> func(); >> pr_err("FAIL: func returned\n"); >> } > > >> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h >> index 76163883c6ff..d225318538bd 100644 >> --- a/include/asm-generic/sections.h >> +++ b/include/asm-generic/sections.h >> @@ -70,6 +70,11 @@ typedef struct { >> } 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.
Hi Kees, Le 16/10/2021 à 08:42, Christophe Leroy a écrit : > > > Le 15/10/2021 à 23:31, Kees Cook a écrit : >> On Thu, Oct 14, 2021 at 07:50:01AM +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 | 25 +++++++++++++++++++++---- >>> include/asm-generic/sections.h | 5 +++++ >>> 2 files changed, 26 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >>> index 5266dc28df6e..96b3ebfcb8ed 100644 >>> --- a/drivers/misc/lkdtm/perms.c >>> +++ b/drivers/misc/lkdtm/perms.c >>> @@ -44,19 +44,32 @@ static noinline void do_overwritten(void) >>> return; >>> } >>> +static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) >>> +{ >>> + memcpy(fdesc, do_nothing, sizeof(*fdesc)); >>> + fdesc->addr = (unsigned long)dst; >>> + barrier(); >>> + >>> + return fdesc; >>> +} >> >> How about collapsing the "have_function_descriptors()" check into >> setup_function_descriptor()? >> >> static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) >> { >> if (__is_defined(HAVE_FUNCTION_DESCRIPTORS)) { >> memcpy(fdesc, do_nothing, sizeof(*fdesc)); >> fdesc->addr = (unsigned long)dst; >> barrier(); >> return fdesc; >> } else { >> return dst; >> } >> } > > Ok > ... >> >>> diff --git a/include/asm-generic/sections.h >>> b/include/asm-generic/sections.h >>> index 76163883c6ff..d225318538bd 100644 >>> --- a/include/asm-generic/sections.h >>> +++ b/include/asm-generic/sections.h >>> @@ -70,6 +70,11 @@ typedef struct { >>> } 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
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c index 5266dc28df6e..96b3ebfcb8ed 100644 --- a/drivers/misc/lkdtm/perms.c +++ b/drivers/misc/lkdtm/perms.c @@ -44,19 +44,32 @@ static noinline void do_overwritten(void) return; } +static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) +{ + memcpy(fdesc, do_nothing, sizeof(*fdesc)); + fdesc->addr = (unsigned long)dst; + barrier(); + + return fdesc; +} + static noinline void execute_location(void *dst, bool write) { void (*func)(void) = dst; + func_desc_t fdesc; + void *do_nothing_text = dereference_function_descriptor(do_nothing); - pr_info("attempting ok execution at %px\n", do_nothing); + pr_info("attempting ok execution at %px\n", do_nothing_text); do_nothing(); if (write == CODE_WRITE) { - memcpy(dst, do_nothing, EXEC_SIZE); + memcpy(dst, do_nothing_text, EXEC_SIZE); flush_icache_range((unsigned long)dst, (unsigned long)dst + EXEC_SIZE); } pr_info("attempting bad execution at %px\n", func); + 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_t fdesc; + void *do_nothing_text = dereference_function_descriptor(do_nothing); - pr_info("attempting ok execution at %px\n", do_nothing); + pr_info("attempting ok execution at %px\n", do_nothing_text); do_nothing(); - copied = access_process_vm(current, (unsigned long)dst, do_nothing, + copied = access_process_vm(current, (unsigned long)dst, do_nothing_text, EXEC_SIZE, FOLL_WRITE); if (copied < EXEC_SIZE) return; pr_info("attempting bad execution at %px\n", func); + if (have_function_descriptors()) + func = setup_function_descriptor(&fdesc, dst); func(); pr_err("FAIL: func returned\n"); } diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index 76163883c6ff..d225318538bd 100644 --- a/include/asm-generic/sections.h +++ b/include/asm-generic/sections.h @@ -70,6 +70,11 @@ typedef struct { } 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
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 | 25 +++++++++++++++++++++---- include/asm-generic/sections.h | 5 +++++ 2 files changed, 26 insertions(+), 4 deletions(-)