Messages in this thread | | | Subject | Re: [PATCH v1 10/10] lkdtm: Fix execute_[user]_location() | From | Christophe Leroy <> | Date | Wed, 13 Oct 2021 14:00:50 +0200 |
| |
Le 13/10/2021 à 09:16, Kees Cook a écrit : > On Mon, Oct 11, 2021 at 05:25:37PM +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 | 45 +++++++++++++++++++++++++++++++------- >> 1 file changed, 37 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >> index da16564e1ecd..1d03cd44c21d 100644 >> --- a/drivers/misc/lkdtm/perms.c >> +++ b/drivers/misc/lkdtm/perms.c >> @@ -44,19 +44,42 @@ static noinline void do_overwritten(void) >> return; >> } >> >> +static void *setup_function_descriptor(funct_descr_t *fdesc, void *dst) >> +{ >> + int err; >> + >> + if (!__is_defined(HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR)) >> + return dst; >> + >> + err = copy_from_kernel_nofault(fdesc, do_nothing, sizeof(*fdesc)); >> + if (err < 0) >> + return ERR_PTR(err); >> + >> + fdesc->addr = (unsigned long)dst; >> + barrier(); >> + >> + return fdesc; >> +} >> + >> static noinline void execute_location(void *dst, bool write) >> { >> - void (*func)(void) = dst; >> + void (*func)(void); >> + funct_descr_t fdesc; >> + void *do_nothing_text = dereference_symbol_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); >> + func = setup_function_descriptor(&fdesc, dst); >> + if (IS_ERR(func)) >> + return; > > I think this error case should complain with some details. :) Maybe: > > pr_err("FAIL: could not build function descriptor for %px\n", dst);
Ok, I was going to add that in v2, but after one more thought I realise that there's no need to use copy_from_kernel_nofault() here, we are copying a static object into our stack, so a memcpy() will be good enough.
> >> + >> + pr_info("attempting bad execution at %px\n", dst); > > And then leave this pr_info() as it was, before the > setup_function_descriptor() call. > >> func(); >> pr_err("FAIL: func returned\n"); >> } >> @@ -66,16 +89,22 @@ static void execute_user_location(void *dst) >> int copied; >> >> /* Intentionally crossing kernel/user memory boundary. */ >> - void (*func)(void) = dst; >> + void (*func)(void); >> + funct_descr_t fdesc; >> + void *do_nothing_text = dereference_symbol_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); >> + func = setup_function_descriptor(&fdesc, dst); >> + if (IS_ERR(func)) >> + return; >> + >> + pr_info("attempting bad execution at %px\n", dst); > > Same here. > >> func(); >> pr_err("FAIL: func returned\n"); >> } >> -- >> 2.31.1 >> >
| |