| Message ID | 7da92e59e148bd23564d63bdd8bcfaba0ba6d1f1.1633964380.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix LKDTM for PPC64/IA64/PARISC | expand |
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote: > Behind a location, lkdtm_EXEC_RODATA() executes a real function, > not a copy of do_nothing(). > > So do it directly instead of using execute_location(). I don't understand this. Why does the next patch not fix this? -Kees > > And fix displayed addresses by dereferencing the function descriptors. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > drivers/misc/lkdtm/perms.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c > index 442d60ed25ef..da16564e1ecd 100644 > --- a/drivers/misc/lkdtm/perms.c > +++ b/drivers/misc/lkdtm/perms.c > @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void) > > void lkdtm_EXEC_RODATA(void) > { > - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS); > + pr_info("attempting ok execution at %px\n", > + dereference_symbol_descriptor(do_nothing)); > + do_nothing(); > + > + pr_info("attempting bad execution at %px\n", > + dereference_symbol_descriptor(lkdtm_rodata_do_nothing)); > + lkdtm_rodata_do_nothing(); > + pr_err("FAIL: func returned\n"); > } > > void lkdtm_EXEC_USERSPACE(void) > -- > 2.31.1 >
On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote: > Behind a location, lkdtm_EXEC_RODATA() executes a real function, > not a copy of do_nothing(). > > So do it directly instead of using execute_location(). > > And fix displayed addresses by dereferencing the function descriptors. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > drivers/misc/lkdtm/perms.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c > index 442d60ed25ef..da16564e1ecd 100644 > --- a/drivers/misc/lkdtm/perms.c > +++ b/drivers/misc/lkdtm/perms.c > @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void) > > void lkdtm_EXEC_RODATA(void) > { > - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS); > + pr_info("attempting ok execution at %px\n", > + dereference_symbol_descriptor(do_nothing)); > + do_nothing(); > + > + pr_info("attempting bad execution at %px\n", > + dereference_symbol_descriptor(lkdtm_rodata_do_nothing)); > + lkdtm_rodata_do_nothing(); > + pr_err("FAIL: func returned\n"); > } (In re-reading this more carefully, I see now why kallsyms.h is used earlier: _function_ vs _symbol_ descriptor.) In the next patch: static noinline void execute_location(void *dst, bool write) { ... func = setup_function_descriptor(&fdesc, dst); if (IS_ERR(func)) return; pr_info("attempting bad execution at %px\n", dst); func(); pr_err("FAIL: func returned\n"); } What are the conditions for which dereference_symbol_descriptor works but dereference _function_descriptor doesn't? -- Kees Cook
Le 13/10/2021 à 09:09, Kees Cook a écrit : > On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote: >> Behind a location, lkdtm_EXEC_RODATA() executes a real function, >> not a copy of do_nothing(). >> >> So do it directly instead of using execute_location(). > > I don't understand this. Why does the next patch not fix this? Well, probably it would, but it looked incorrect in my mind. lkdtm_rodata_do_nothing() is a function which has its own function descriptor, it is not a copy of do_nothing(). If we use execute_location() as modified by next patch, then we will execute it using the function descriptor of do_nothing(). Allthough it most likely works (at least on powerpc as it uses the same TOC) it looks odd to me to do so. Am I missing something ? Christophe > > -Kees > >> >> And fix displayed addresses by dereferencing the function descriptors. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> drivers/misc/lkdtm/perms.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >> index 442d60ed25ef..da16564e1ecd 100644 >> --- a/drivers/misc/lkdtm/perms.c >> +++ b/drivers/misc/lkdtm/perms.c >> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void) >> >> void lkdtm_EXEC_RODATA(void) >> { >> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS); >> + pr_info("attempting ok execution at %px\n", >> + dereference_symbol_descriptor(do_nothing)); >> + do_nothing(); >> + >> + pr_info("attempting bad execution at %px\n", >> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing)); >> + lkdtm_rodata_do_nothing(); >> + pr_err("FAIL: func returned\n"); >> } >> >> void lkdtm_EXEC_USERSPACE(void) >> -- >> 2.31.1 >> >
Le 13/10/2021 à 09:23, Kees Cook a écrit : > On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote: >> Behind a location, lkdtm_EXEC_RODATA() executes a real function, >> not a copy of do_nothing(). >> >> So do it directly instead of using execute_location(). >> >> And fix displayed addresses by dereferencing the function descriptors. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> drivers/misc/lkdtm/perms.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >> index 442d60ed25ef..da16564e1ecd 100644 >> --- a/drivers/misc/lkdtm/perms.c >> +++ b/drivers/misc/lkdtm/perms.c >> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void) >> >> void lkdtm_EXEC_RODATA(void) >> { >> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS); >> + pr_info("attempting ok execution at %px\n", >> + dereference_symbol_descriptor(do_nothing)); >> + do_nothing(); >> + >> + pr_info("attempting bad execution at %px\n", >> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing)); >> + lkdtm_rodata_do_nothing(); >> + pr_err("FAIL: func returned\n"); >> } > > (In re-reading this more carefully, I see now why kallsyms.h is used > earlier: _function_ vs _symbol_ descriptor.) > > In the next patch: > > static noinline void execute_location(void *dst, bool write) > { > ... > func = setup_function_descriptor(&fdesc, dst); > if (IS_ERR(func)) > return; > > pr_info("attempting bad execution at %px\n", dst); > func(); > pr_err("FAIL: func returned\n"); > } > > What are the conditions for which dereference_symbol_descriptor works > but dereference _function_descriptor doesn't? > When LKDTM is built as a module I guess ? Christophe
Le 13/10/2021 à 09:39, Christophe Leroy a écrit : > > > Le 13/10/2021 à 09:23, Kees Cook a écrit : >> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote: >>> Behind a location, lkdtm_EXEC_RODATA() executes a real function, >>> not a copy of do_nothing(). >>> >>> So do it directly instead of using execute_location(). >>> >>> And fix displayed addresses by dereferencing the function descriptors. >>> >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>> --- >>> drivers/misc/lkdtm/perms.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >>> index 442d60ed25ef..da16564e1ecd 100644 >>> --- a/drivers/misc/lkdtm/perms.c >>> +++ b/drivers/misc/lkdtm/perms.c >>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void) >>> >>> void lkdtm_EXEC_RODATA(void) >>> { >>> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS); >>> + pr_info("attempting ok execution at %px\n", >>> + dereference_symbol_descriptor(do_nothing)); >>> + do_nothing(); >>> + >>> + pr_info("attempting bad execution at %px\n", >>> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing)); >>> + lkdtm_rodata_do_nothing(); >>> + pr_err("FAIL: func returned\n"); >>> } >> >> (In re-reading this more carefully, I see now why kallsyms.h is used >> earlier: _function_ vs _symbol_ descriptor.) >> >> In the next patch: >> >> static noinline void execute_location(void *dst, bool write) >> { >> ... >> func = setup_function_descriptor(&fdesc, dst); >> if (IS_ERR(func)) >> return; >> >> pr_info("attempting bad execution at %px\n", dst); >> func(); >> pr_err("FAIL: func returned\n"); >> } >> >> What are the conditions for which dereference_symbol_descriptor works >> but dereference _function_descriptor doesn't? >> > > When LKDTM is built as a module I guess ? > To be more precise, dereference_symbol_descriptor() calls either dereference_kernel_function_descriptor() or dereference_module_function_descriptor() Both functions call dereference_function_descriptor() after checking that we want to dereference something that is in the OPD section. If we call dereference_function_descriptor() directly instead of dereference_symbol_descriptor() we skip the range verification and may dereference something that is not a function descriptor. Should we do that ? Christophe
Le 13/10/2021 à 09:48, Christophe Leroy a écrit : > > > Le 13/10/2021 à 09:39, Christophe Leroy a écrit : >> >> >> Le 13/10/2021 à 09:23, Kees Cook a écrit : >>> On Mon, Oct 11, 2021 at 05:25:36PM +0200, Christophe Leroy wrote: >>>> Behind a location, lkdtm_EXEC_RODATA() executes a real function, >>>> not a copy of do_nothing(). >>>> >>>> So do it directly instead of using execute_location(). >>>> >>>> And fix displayed addresses by dereferencing the function descriptors. >>>> >>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>>> --- >>>> drivers/misc/lkdtm/perms.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >>>> index 442d60ed25ef..da16564e1ecd 100644 >>>> --- a/drivers/misc/lkdtm/perms.c >>>> +++ b/drivers/misc/lkdtm/perms.c >>>> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void) >>>> >>>> void lkdtm_EXEC_RODATA(void) >>>> { >>>> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS); >>>> + pr_info("attempting ok execution at %px\n", >>>> + dereference_symbol_descriptor(do_nothing)); >>>> + do_nothing(); >>>> + >>>> + pr_info("attempting bad execution at %px\n", >>>> + dereference_symbol_descriptor(lkdtm_rodata_do_nothing)); >>>> + lkdtm_rodata_do_nothing(); >>>> + pr_err("FAIL: func returned\n"); >>>> } >>> >>> (In re-reading this more carefully, I see now why kallsyms.h is used >>> earlier: _function_ vs _symbol_ descriptor.) >>> >>> In the next patch: >>> >>> static noinline void execute_location(void *dst, bool write) >>> { >>> ... >>> func = setup_function_descriptor(&fdesc, dst); >>> if (IS_ERR(func)) >>> return; >>> >>> pr_info("attempting bad execution at %px\n", dst); >>> func(); >>> pr_err("FAIL: func returned\n"); >>> } >>> >>> What are the conditions for which dereference_symbol_descriptor works >>> but dereference _function_descriptor doesn't? >>> >> >> When LKDTM is built as a module I guess ? >> > > To be more precise, dereference_symbol_descriptor() calls either > dereference_kernel_function_descriptor() or > dereference_module_function_descriptor() > > Both functions call dereference_function_descriptor() after checking > that we want to dereference something that is in the OPD section. > > If we call dereference_function_descriptor() directly instead of > dereference_symbol_descriptor() we skip the range verification and may > dereference something that is not a function descriptor. > > Should we do that ? > Indeed we are using it only for well known functions so using dereference_function_descriptor() is good enough. I'll use that in v2.
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c index 442d60ed25ef..da16564e1ecd 100644 --- a/drivers/misc/lkdtm/perms.c +++ b/drivers/misc/lkdtm/perms.c @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void) void lkdtm_EXEC_RODATA(void) { - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS); + pr_info("attempting ok execution at %px\n", + dereference_symbol_descriptor(do_nothing)); + do_nothing(); + + pr_info("attempting bad execution at %px\n", + dereference_symbol_descriptor(lkdtm_rodata_do_nothing)); + lkdtm_rodata_do_nothing(); + pr_err("FAIL: func returned\n"); } void lkdtm_EXEC_USERSPACE(void)
Behind a location, lkdtm_EXEC_RODATA() executes a real function, not a copy of do_nothing(). So do it directly instead of using execute_location(). And fix displayed addresses by dereferencing the function descriptors. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- drivers/misc/lkdtm/perms.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)