Messages in this thread | | | Date | Tue, 16 Nov 2021 16:07:47 +0100 | Subject | Re: [PATCH v2 12/13] lkdtm: Fix execute_[user]_location() | From | Christophe Leroy <> |
| |
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
| |