| Message ID | 67f9545c9ad15048bfe0104278ef9595d051dbc8.1634457599.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix LKDTM for PPC64/IA64/PARISC | expand |
On Sun, Oct 17, 2021 at 02:38:25PM +0200, Christophe Leroy wrote: > Add WRITE_OPD to check that you can't modify function > descriptors. > > Gives the following result when function descriptors are > not protected: > > lkdtm: Performing direct entry WRITE_OPD > lkdtm: attempting bad 16 bytes write at c00000000269b358 > lkdtm: FAIL: survived bad write > lkdtm: do_nothing was hijacked! > > Looks like a standard compiler barrier() is not enough to force > GCC to use the modified function descriptor. Had to add a fake empty > inline assembly to force GCC to reload the function descriptor. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > drivers/misc/lkdtm/core.c | 1 + > drivers/misc/lkdtm/lkdtm.h | 1 + > drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++ > 3 files changed, 24 insertions(+) > > diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c > index fe6fd34b8caf..de092aa03b5d 100644 > --- a/drivers/misc/lkdtm/core.c > +++ b/drivers/misc/lkdtm/core.c > @@ -148,6 +148,7 @@ static const struct crashtype crashtypes[] = { > CRASHTYPE(WRITE_RO), > CRASHTYPE(WRITE_RO_AFTER_INIT), > CRASHTYPE(WRITE_KERN), > + CRASHTYPE(WRITE_OPD), > CRASHTYPE(REFCOUNT_INC_OVERFLOW), > CRASHTYPE(REFCOUNT_ADD_OVERFLOW), > CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW), > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h > index c212a253edde..188bd0fd6575 100644 > --- a/drivers/misc/lkdtm/lkdtm.h > +++ b/drivers/misc/lkdtm/lkdtm.h > @@ -105,6 +105,7 @@ void __init lkdtm_perms_init(void); > void lkdtm_WRITE_RO(void); > void lkdtm_WRITE_RO_AFTER_INIT(void); > void lkdtm_WRITE_KERN(void); > +void lkdtm_WRITE_OPD(void); > void lkdtm_EXEC_DATA(void); > void lkdtm_EXEC_STACK(void); > void lkdtm_EXEC_KMALLOC(void); > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c > index 1cf24c4a79e9..2c6aba3ff32b 100644 > --- a/drivers/misc/lkdtm/perms.c > +++ b/drivers/misc/lkdtm/perms.c > @@ -44,6 +44,11 @@ static noinline void do_overwritten(void) > return; > } > > +static noinline void do_almost_nothing(void) > +{ > + pr_info("do_nothing was hijacked!\n"); > +} > + > static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) > { > if (!have_function_descriptors()) > @@ -144,6 +149,23 @@ void lkdtm_WRITE_KERN(void) > do_overwritten(); > } > > +void lkdtm_WRITE_OPD(void) > +{ > + size_t size = sizeof(func_desc_t); > + void (*func)(void) = do_nothing; > + > + if (!have_function_descriptors()) { > + pr_info("XFAIL: Platform doesn't use function descriptors.\n"); > + return; > + } > + pr_info("attempting bad %zu bytes write at %px\n", size, do_nothing); > + memcpy(do_nothing, do_almost_nothing, size); > + pr_err("FAIL: survived bad write\n"); Non-function-descriptor architectures would successfully crash at the memcpy too, right? (i.e. for them this is just repeating WRITE_KERN) I'm pondering the utility of the XFAIL vs just letting is succeed, but I think it more accurate to say "hey, no OPD" as you have it. > + > + asm("" : "=m"(func)); > + func(); > +} > + > void lkdtm_EXEC_DATA(void) > { > execute_location(data_area, CODE_WRITE); > -- > 2.31.1 > One tiny suggestion, since I think you need to respin for the EXPORT_SYMBOL_GPL() anyway. Please update the selftests too: diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 6b36b7f5dcf9..243c781f0780 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -44,6 +44,7 @@ ACCESS_NULL WRITE_RO WRITE_RO_AFTER_INIT WRITE_KERN +WRITE_OPD REFCOUNT_INC_OVERFLOW REFCOUNT_ADD_OVERFLOW REFCOUNT_INC_NOT_ZERO_OVERFLOW (Though for the future I've been considering making the selftests an opt-out list so the "normal" stuff doesn't need to keep getting added there.) Thanks! Acked-by: Kees Cook <keescook@chromium.org> -Kees
Le 11/02/2022 à 02:09, Kees Cook a écrit : > On Sun, Oct 17, 2021 at 02:38:25PM +0200, Christophe Leroy wrote: >> Add WRITE_OPD to check that you can't modify function >> descriptors. >> >> Gives the following result when function descriptors are >> not protected: >> >> lkdtm: Performing direct entry WRITE_OPD >> lkdtm: attempting bad 16 bytes write at c00000000269b358 >> lkdtm: FAIL: survived bad write >> lkdtm: do_nothing was hijacked! >> >> Looks like a standard compiler barrier() is not enough to force >> GCC to use the modified function descriptor. Had to add a fake empty >> inline assembly to force GCC to reload the function descriptor. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> drivers/misc/lkdtm/core.c | 1 + >> drivers/misc/lkdtm/lkdtm.h | 1 + >> drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c >> index fe6fd34b8caf..de092aa03b5d 100644 >> --- a/drivers/misc/lkdtm/core.c >> +++ b/drivers/misc/lkdtm/core.c >> @@ -148,6 +148,7 @@ static const struct crashtype crashtypes[] = { >> CRASHTYPE(WRITE_RO), >> CRASHTYPE(WRITE_RO_AFTER_INIT), >> CRASHTYPE(WRITE_KERN), >> + CRASHTYPE(WRITE_OPD), >> CRASHTYPE(REFCOUNT_INC_OVERFLOW), >> CRASHTYPE(REFCOUNT_ADD_OVERFLOW), >> CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW), >> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h >> index c212a253edde..188bd0fd6575 100644 >> --- a/drivers/misc/lkdtm/lkdtm.h >> +++ b/drivers/misc/lkdtm/lkdtm.h >> @@ -105,6 +105,7 @@ void __init lkdtm_perms_init(void); >> void lkdtm_WRITE_RO(void); >> void lkdtm_WRITE_RO_AFTER_INIT(void); >> void lkdtm_WRITE_KERN(void); >> +void lkdtm_WRITE_OPD(void); >> void lkdtm_EXEC_DATA(void); >> void lkdtm_EXEC_STACK(void); >> void lkdtm_EXEC_KMALLOC(void); >> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >> index 1cf24c4a79e9..2c6aba3ff32b 100644 >> --- a/drivers/misc/lkdtm/perms.c >> +++ b/drivers/misc/lkdtm/perms.c >> @@ -44,6 +44,11 @@ static noinline void do_overwritten(void) >> return; >> } >> >> +static noinline void do_almost_nothing(void) >> +{ >> + pr_info("do_nothing was hijacked!\n"); >> +} >> + >> static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) >> { >> if (!have_function_descriptors()) >> @@ -144,6 +149,23 @@ void lkdtm_WRITE_KERN(void) >> do_overwritten(); >> } >> >> +void lkdtm_WRITE_OPD(void) >> +{ >> + size_t size = sizeof(func_desc_t); >> + void (*func)(void) = do_nothing; >> + >> + if (!have_function_descriptors()) { >> + pr_info("XFAIL: Platform doesn't use function descriptors.\n"); >> + return; >> + } >> + pr_info("attempting bad %zu bytes write at %px\n", size, do_nothing); >> + memcpy(do_nothing, do_almost_nothing, size); >> + pr_err("FAIL: survived bad write\n"); > > Non-function-descriptor architectures would successfully crash at the > memcpy too, right? (i.e. for them this is just repeating WRITE_KERN) Yes it should. But not for the good reason. > > I'm pondering the utility of the XFAIL vs just letting is succeed, but I > think it more accurate to say "hey, no OPD" as you have it. > >> + >> + asm("" : "=m"(func)); >> + func(); >> +} >> + >> void lkdtm_EXEC_DATA(void) >> { >> execute_location(data_area, CODE_WRITE); >> -- >> 2.31.1 >> > > One tiny suggestion, since I think you need to respin for the > EXPORT_SYMBOL_GPL() anyway. Please update the selftests too: > > diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt > index 6b36b7f5dcf9..243c781f0780 100644 > --- a/tools/testing/selftests/lkdtm/tests.txt > +++ b/tools/testing/selftests/lkdtm/tests.txt > @@ -44,6 +44,7 @@ ACCESS_NULL > WRITE_RO > WRITE_RO_AFTER_INIT > WRITE_KERN > +WRITE_OPD > REFCOUNT_INC_OVERFLOW > REFCOUNT_ADD_OVERFLOW > REFCOUNT_INC_NOT_ZERO_OVERFLOW > > (Though for the future I've been considering making the selftests an > opt-out list so the "normal" stuff doesn't need to keep getting added > there.) > > Thanks! > > Acked-by: Kees Cook <keescook@chromium.org> > Done. Thanks Christophe
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c index fe6fd34b8caf..de092aa03b5d 100644 --- a/drivers/misc/lkdtm/core.c +++ b/drivers/misc/lkdtm/core.c @@ -148,6 +148,7 @@ static const struct crashtype crashtypes[] = { CRASHTYPE(WRITE_RO), CRASHTYPE(WRITE_RO_AFTER_INIT), CRASHTYPE(WRITE_KERN), + CRASHTYPE(WRITE_OPD), CRASHTYPE(REFCOUNT_INC_OVERFLOW), CRASHTYPE(REFCOUNT_ADD_OVERFLOW), CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW), diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h index c212a253edde..188bd0fd6575 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -105,6 +105,7 @@ void __init lkdtm_perms_init(void); void lkdtm_WRITE_RO(void); void lkdtm_WRITE_RO_AFTER_INIT(void); void lkdtm_WRITE_KERN(void); +void lkdtm_WRITE_OPD(void); void lkdtm_EXEC_DATA(void); void lkdtm_EXEC_STACK(void); void lkdtm_EXEC_KMALLOC(void); diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c index 1cf24c4a79e9..2c6aba3ff32b 100644 --- a/drivers/misc/lkdtm/perms.c +++ b/drivers/misc/lkdtm/perms.c @@ -44,6 +44,11 @@ static noinline void do_overwritten(void) return; } +static noinline void do_almost_nothing(void) +{ + pr_info("do_nothing was hijacked!\n"); +} + static void *setup_function_descriptor(func_desc_t *fdesc, void *dst) { if (!have_function_descriptors()) @@ -144,6 +149,23 @@ void lkdtm_WRITE_KERN(void) do_overwritten(); } +void lkdtm_WRITE_OPD(void) +{ + size_t size = sizeof(func_desc_t); + void (*func)(void) = do_nothing; + + if (!have_function_descriptors()) { + pr_info("XFAIL: Platform doesn't use function descriptors.\n"); + return; + } + pr_info("attempting bad %zu bytes write at %px\n", size, do_nothing); + memcpy(do_nothing, do_almost_nothing, size); + pr_err("FAIL: survived bad write\n"); + + asm("" : "=m"(func)); + func(); +} + void lkdtm_EXEC_DATA(void) { execute_location(data_area, CODE_WRITE);
Add WRITE_OPD to check that you can't modify function descriptors. Gives the following result when function descriptors are not protected: lkdtm: Performing direct entry WRITE_OPD lkdtm: attempting bad 16 bytes write at c00000000269b358 lkdtm: FAIL: survived bad write lkdtm: do_nothing was hijacked! Looks like a standard compiler barrier() is not enough to force GCC to use the modified function descriptor. Had to add a fake empty inline assembly to force GCC to reload the function descriptor. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- drivers/misc/lkdtm/core.c | 1 + drivers/misc/lkdtm/lkdtm.h | 1 + drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++ 3 files changed, 24 insertions(+)