| Message ID | fe36bf23fb14e7eff92a95a1092ed38edb01d5f5.1634491011.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing() | expand |
Hi Kees, Le 17/10/2021 à 19:19, Christophe Leroy a écrit : > All EXEC tests are based on running a copy of do_nothing() > except lkdtm_EXEC_RODATA which uses a different function > called lkdtm_rodata_do_nothing(). > > On architectures using function descriptors, EXEC tests are > performed using execute_location() which is a function > that most of the time copies do_nothing() at the tested > location then duplicates do_nothing() function descriptor > and updates it with the address of the copy of do_nothing(). > > But for EXEC_RODATA test, execute_location() uses > lkdtm_rodata_do_nothing() which is already in rodata section > at build time instead of using a copy of do_nothing(). However > it still uses the function descriptor of do_nothing(). There > is a risk that running lkdtm_rodata_do_nothing() with the > function descriptor of do_thing() is wrong. > > To remove the above risk, change the approach and do the same > as for other EXEC tests: use a copy of do_nothing(). The copy > cannot be done during the test because RODATA area is write > protected. Do the copy during init, before RODATA becomes > write protected. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> Any opinion on this patch ? Thanks Christophe > --- > This applies on top of series v3 "Fix LKDTM for PPC64/IA64/PARISC" > > drivers/misc/lkdtm/Makefile | 11 ----------- > drivers/misc/lkdtm/lkdtm.h | 3 --- > drivers/misc/lkdtm/perms.c | 9 +++++++-- > drivers/misc/lkdtm/rodata.c | 11 ----------- > 4 files changed, 7 insertions(+), 27 deletions(-) > delete mode 100644 drivers/misc/lkdtm/rodata.c > > diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile > index e2984ce51fe4..3d45a2b3007d 100644 > --- a/drivers/misc/lkdtm/Makefile > +++ b/drivers/misc/lkdtm/Makefile > @@ -6,21 +6,10 @@ lkdtm-$(CONFIG_LKDTM) += bugs.o > lkdtm-$(CONFIG_LKDTM) += heap.o > lkdtm-$(CONFIG_LKDTM) += perms.o > lkdtm-$(CONFIG_LKDTM) += refcount.o > -lkdtm-$(CONFIG_LKDTM) += rodata_objcopy.o > lkdtm-$(CONFIG_LKDTM) += usercopy.o > lkdtm-$(CONFIG_LKDTM) += stackleak.o > lkdtm-$(CONFIG_LKDTM) += cfi.o > lkdtm-$(CONFIG_LKDTM) += fortify.o > lkdtm-$(CONFIG_PPC_BOOK3S_64) += powerpc.o > > -KASAN_SANITIZE_rodata.o := n > KASAN_SANITIZE_stackleak.o := n > -KCOV_INSTRUMENT_rodata.o := n > -CFLAGS_REMOVE_rodata.o += $(CC_FLAGS_LTO) > - > -OBJCOPYFLAGS := > -OBJCOPYFLAGS_rodata_objcopy.o := \ > - --rename-section .noinstr.text=.rodata,alloc,readonly,load,contents > -targets += rodata.o rodata_objcopy.o > -$(obj)/rodata_objcopy.o: $(obj)/rodata.o FORCE > - $(call if_changed,objcopy) > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h > index 188bd0fd6575..905555d4c2cf 100644 > --- a/drivers/misc/lkdtm/lkdtm.h > +++ b/drivers/misc/lkdtm/lkdtm.h > @@ -137,9 +137,6 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void); > void lkdtm_REFCOUNT_TIMING(void); > void lkdtm_ATOMIC_TIMING(void); > > -/* rodata.c */ > -void lkdtm_rodata_do_nothing(void); > - > /* usercopy.c */ > void __init lkdtm_usercopy_init(void); > void __exit lkdtm_usercopy_exit(void); > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c > index 2c6aba3ff32b..9b951ca48363 100644 > --- a/drivers/misc/lkdtm/perms.c > +++ b/drivers/misc/lkdtm/perms.c > @@ -27,6 +27,7 @@ static const unsigned long rodata = 0xAA55AA55; > > /* This is marked __ro_after_init, so it should ultimately be .rodata. */ > static unsigned long ro_after_init __ro_after_init = 0x55AA5500; > +static u8 rodata_area[EXEC_SIZE] __ro_after_init; > > /* > * This just returns to the caller. It is designed to be copied into > @@ -193,8 +194,7 @@ void lkdtm_EXEC_VMALLOC(void) > > void lkdtm_EXEC_RODATA(void) > { > - execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing), > - CODE_AS_IS); > + execute_location(rodata_area, CODE_AS_IS); > } > > void lkdtm_EXEC_USERSPACE(void) > @@ -269,4 +269,9 @@ void __init lkdtm_perms_init(void) > { > /* Make sure we can write to __ro_after_init values during __init */ > ro_after_init |= 0xAA; > + > + memcpy(rodata_area, dereference_function_descriptor(do_nothing), > + EXEC_SIZE); > + flush_icache_range((unsigned long)rodata_area, > + (unsigned long)rodata_area + EXEC_SIZE); > } > diff --git a/drivers/misc/lkdtm/rodata.c b/drivers/misc/lkdtm/rodata.c > deleted file mode 100644 > index baacb876d1d9..000000000000 > --- a/drivers/misc/lkdtm/rodata.c > +++ /dev/null > @@ -1,11 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * This includes functions that are meant to live entirely in .rodata > - * (via objcopy tricks), to validate the non-executability of .rodata. > - */ > -#include "lkdtm.h" > - > -void noinstr lkdtm_rodata_do_nothing(void) > -{ > - /* Does nothing. We just want an architecture agnostic "return". */ > -}
Hi Kees, Le 23/02/2022 à 18:17, Christophe Leroy a écrit : > Hi Kees, > > Le 17/10/2021 à 19:19, Christophe Leroy a écrit : >> All EXEC tests are based on running a copy of do_nothing() >> except lkdtm_EXEC_RODATA which uses a different function >> called lkdtm_rodata_do_nothing(). >> >> On architectures using function descriptors, EXEC tests are >> performed using execute_location() which is a function >> that most of the time copies do_nothing() at the tested >> location then duplicates do_nothing() function descriptor >> and updates it with the address of the copy of do_nothing(). >> >> But for EXEC_RODATA test, execute_location() uses >> lkdtm_rodata_do_nothing() which is already in rodata section >> at build time instead of using a copy of do_nothing(). However >> it still uses the function descriptor of do_nothing(). There >> is a risk that running lkdtm_rodata_do_nothing() with the >> function descriptor of do_thing() is wrong. >> >> To remove the above risk, change the approach and do the same >> as for other EXEC tests: use a copy of do_nothing(). The copy >> cannot be done during the test because RODATA area is write >> protected. Do the copy during init, before RODATA becomes >> write protected. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > Any opinion on this patch ? Any opinion ? Thanks Christophe > > Thanks > Christophe > >> --- >> This applies on top of series v3 "Fix LKDTM for PPC64/IA64/PARISC" >> >> drivers/misc/lkdtm/Makefile | 11 ----------- >> drivers/misc/lkdtm/lkdtm.h | 3 --- >> drivers/misc/lkdtm/perms.c | 9 +++++++-- >> drivers/misc/lkdtm/rodata.c | 11 ----------- >> 4 files changed, 7 insertions(+), 27 deletions(-) >> delete mode 100644 drivers/misc/lkdtm/rodata.c >> >> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile >> index e2984ce51fe4..3d45a2b3007d 100644 >> --- a/drivers/misc/lkdtm/Makefile >> +++ b/drivers/misc/lkdtm/Makefile >> @@ -6,21 +6,10 @@ lkdtm-$(CONFIG_LKDTM) += bugs.o >> lkdtm-$(CONFIG_LKDTM) += heap.o >> lkdtm-$(CONFIG_LKDTM) += perms.o >> lkdtm-$(CONFIG_LKDTM) += refcount.o >> -lkdtm-$(CONFIG_LKDTM) += rodata_objcopy.o >> lkdtm-$(CONFIG_LKDTM) += usercopy.o >> lkdtm-$(CONFIG_LKDTM) += stackleak.o >> lkdtm-$(CONFIG_LKDTM) += cfi.o >> lkdtm-$(CONFIG_LKDTM) += fortify.o >> lkdtm-$(CONFIG_PPC_BOOK3S_64) += powerpc.o >> -KASAN_SANITIZE_rodata.o := n >> KASAN_SANITIZE_stackleak.o := n >> -KCOV_INSTRUMENT_rodata.o := n >> -CFLAGS_REMOVE_rodata.o += $(CC_FLAGS_LTO) >> - >> -OBJCOPYFLAGS := >> -OBJCOPYFLAGS_rodata_objcopy.o := \ >> - --rename-section >> .noinstr.text=.rodata,alloc,readonly,load,contents >> -targets += rodata.o rodata_objcopy.o >> -$(obj)/rodata_objcopy.o: $(obj)/rodata.o FORCE >> - $(call if_changed,objcopy) >> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h >> index 188bd0fd6575..905555d4c2cf 100644 >> --- a/drivers/misc/lkdtm/lkdtm.h >> +++ b/drivers/misc/lkdtm/lkdtm.h >> @@ -137,9 +137,6 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void); >> void lkdtm_REFCOUNT_TIMING(void); >> void lkdtm_ATOMIC_TIMING(void); >> -/* rodata.c */ >> -void lkdtm_rodata_do_nothing(void); >> - >> /* usercopy.c */ >> void __init lkdtm_usercopy_init(void); >> void __exit lkdtm_usercopy_exit(void); >> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c >> index 2c6aba3ff32b..9b951ca48363 100644 >> --- a/drivers/misc/lkdtm/perms.c >> +++ b/drivers/misc/lkdtm/perms.c >> @@ -27,6 +27,7 @@ static const unsigned long rodata = 0xAA55AA55; >> /* This is marked __ro_after_init, so it should ultimately be >> .rodata. */ >> static unsigned long ro_after_init __ro_after_init = 0x55AA5500; >> +static u8 rodata_area[EXEC_SIZE] __ro_after_init; >> /* >> * This just returns to the caller. It is designed to be copied into >> @@ -193,8 +194,7 @@ void lkdtm_EXEC_VMALLOC(void) >> void lkdtm_EXEC_RODATA(void) >> { >> - >> execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing), >> >> - CODE_AS_IS); >> + execute_location(rodata_area, CODE_AS_IS); >> } >> void lkdtm_EXEC_USERSPACE(void) >> @@ -269,4 +269,9 @@ void __init lkdtm_perms_init(void) >> { >> /* Make sure we can write to __ro_after_init values during >> __init */ >> ro_after_init |= 0xAA; >> + >> + memcpy(rodata_area, dereference_function_descriptor(do_nothing), >> + EXEC_SIZE); >> + flush_icache_range((unsigned long)rodata_area, >> + (unsigned long)rodata_area + EXEC_SIZE); >> } >> diff --git a/drivers/misc/lkdtm/rodata.c b/drivers/misc/lkdtm/rodata.c >> deleted file mode 100644 >> index baacb876d1d9..000000000000 >> --- a/drivers/misc/lkdtm/rodata.c >> +++ /dev/null >> @@ -1,11 +0,0 @@ >> -// SPDX-License-Identifier: GPL-2.0 >> -/* >> - * This includes functions that are meant to live entirely in .rodata >> - * (via objcopy tricks), to validate the non-executability of .rodata. >> - */ >> -#include "lkdtm.h" >> - >> -void noinstr lkdtm_rodata_do_nothing(void) >> -{ >> - /* Does nothing. We just want an architecture agnostic "return". */ >> -}
On Sun, Oct 17, 2021 at 07:19:47PM +0200, Christophe Leroy wrote: > But for EXEC_RODATA test, execute_location() uses > lkdtm_rodata_do_nothing() which is already in rodata section > at build time instead of using a copy of do_nothing(). However > it still uses the function descriptor of do_nothing(). There > is a risk that running lkdtm_rodata_do_nothing() with the > function descriptor of do_thing() is wrong. Wrong how? (Could there be two descriptors?) > To remove the above risk, change the approach and do the same > as for other EXEC tests: use a copy of do_nothing(). The copy > cannot be done during the test because RODATA area is write > protected. Do the copy during init, before RODATA becomes > write protected. Hmm, hmm. This is a nice way to handle it, but I'm not sure which "weird" way is better. I kind of prefer the code going through all the "regular" linking goo to end up in .rodata, but is it really any different from doing this via the ro_after_init section? It makes me nervous because they can technically be handled differently. For example, .rodata is mapped differently on some architectures compared to ro_after_init. Honestly, I actually this this patch should be modified to _add_ a new test for EXEC_RO_AFTER_INIT, and leave the existing .rodata one alone... -Kees
diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile index e2984ce51fe4..3d45a2b3007d 100644 --- a/drivers/misc/lkdtm/Makefile +++ b/drivers/misc/lkdtm/Makefile @@ -6,21 +6,10 @@ lkdtm-$(CONFIG_LKDTM) += bugs.o lkdtm-$(CONFIG_LKDTM) += heap.o lkdtm-$(CONFIG_LKDTM) += perms.o lkdtm-$(CONFIG_LKDTM) += refcount.o -lkdtm-$(CONFIG_LKDTM) += rodata_objcopy.o lkdtm-$(CONFIG_LKDTM) += usercopy.o lkdtm-$(CONFIG_LKDTM) += stackleak.o lkdtm-$(CONFIG_LKDTM) += cfi.o lkdtm-$(CONFIG_LKDTM) += fortify.o lkdtm-$(CONFIG_PPC_BOOK3S_64) += powerpc.o -KASAN_SANITIZE_rodata.o := n KASAN_SANITIZE_stackleak.o := n -KCOV_INSTRUMENT_rodata.o := n -CFLAGS_REMOVE_rodata.o += $(CC_FLAGS_LTO) - -OBJCOPYFLAGS := -OBJCOPYFLAGS_rodata_objcopy.o := \ - --rename-section .noinstr.text=.rodata,alloc,readonly,load,contents -targets += rodata.o rodata_objcopy.o -$(obj)/rodata_objcopy.o: $(obj)/rodata.o FORCE - $(call if_changed,objcopy) diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h index 188bd0fd6575..905555d4c2cf 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -137,9 +137,6 @@ void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void); void lkdtm_REFCOUNT_TIMING(void); void lkdtm_ATOMIC_TIMING(void); -/* rodata.c */ -void lkdtm_rodata_do_nothing(void); - /* usercopy.c */ void __init lkdtm_usercopy_init(void); void __exit lkdtm_usercopy_exit(void); diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c index 2c6aba3ff32b..9b951ca48363 100644 --- a/drivers/misc/lkdtm/perms.c +++ b/drivers/misc/lkdtm/perms.c @@ -27,6 +27,7 @@ static const unsigned long rodata = 0xAA55AA55; /* This is marked __ro_after_init, so it should ultimately be .rodata. */ static unsigned long ro_after_init __ro_after_init = 0x55AA5500; +static u8 rodata_area[EXEC_SIZE] __ro_after_init; /* * This just returns to the caller. It is designed to be copied into @@ -193,8 +194,7 @@ void lkdtm_EXEC_VMALLOC(void) void lkdtm_EXEC_RODATA(void) { - execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing), - CODE_AS_IS); + execute_location(rodata_area, CODE_AS_IS); } void lkdtm_EXEC_USERSPACE(void) @@ -269,4 +269,9 @@ void __init lkdtm_perms_init(void) { /* Make sure we can write to __ro_after_init values during __init */ ro_after_init |= 0xAA; + + memcpy(rodata_area, dereference_function_descriptor(do_nothing), + EXEC_SIZE); + flush_icache_range((unsigned long)rodata_area, + (unsigned long)rodata_area + EXEC_SIZE); } diff --git a/drivers/misc/lkdtm/rodata.c b/drivers/misc/lkdtm/rodata.c deleted file mode 100644 index baacb876d1d9..000000000000 --- a/drivers/misc/lkdtm/rodata.c +++ /dev/null @@ -1,11 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -/* - * This includes functions that are meant to live entirely in .rodata - * (via objcopy tricks), to validate the non-executability of .rodata. - */ -#include "lkdtm.h" - -void noinstr lkdtm_rodata_do_nothing(void) -{ - /* Does nothing. We just want an architecture agnostic "return". */ -}
All EXEC tests are based on running a copy of do_nothing() except lkdtm_EXEC_RODATA which uses a different function called lkdtm_rodata_do_nothing(). On architectures using function descriptors, EXEC tests are performed using execute_location() which is a function that most of the time copies do_nothing() at the tested location then duplicates do_nothing() function descriptor and updates it with the address of the copy of do_nothing(). But for EXEC_RODATA test, execute_location() uses lkdtm_rodata_do_nothing() which is already in rodata section at build time instead of using a copy of do_nothing(). However it still uses the function descriptor of do_nothing(). There is a risk that running lkdtm_rodata_do_nothing() with the function descriptor of do_thing() is wrong. To remove the above risk, change the approach and do the same as for other EXEC tests: use a copy of do_nothing(). The copy cannot be done during the test because RODATA area is write protected. Do the copy during init, before RODATA becomes write protected. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- This applies on top of series v3 "Fix LKDTM for PPC64/IA64/PARISC" drivers/misc/lkdtm/Makefile | 11 ----------- drivers/misc/lkdtm/lkdtm.h | 3 --- drivers/misc/lkdtm/perms.c | 9 +++++++-- drivers/misc/lkdtm/rodata.c | 11 ----------- 4 files changed, 7 insertions(+), 27 deletions(-) delete mode 100644 drivers/misc/lkdtm/rodata.c