* [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
@ 2021-10-17 17:19 Christophe Leroy
2022-02-23 17:17 ` Christophe Leroy
2022-04-08 3:42 ` Kees Cook
0 siblings, 2 replies; 4+ messages in thread
From: Christophe Leroy @ 2021-10-17 17:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Andrew Morton, James E.J. Bottomley, Helge Deller, Arnd Bergmann,
Kees Cook, Greg Kroah-Hartman
Cc: Christophe Leroy, linux-kernel, linuxppc-dev, linux-ia64,
linux-parisc, linux-arch, linux-mm
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
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". */
-}
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
2021-10-17 17:19 [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing() Christophe Leroy
@ 2022-02-23 17:17 ` Christophe Leroy
2022-04-07 18:32 ` Christophe Leroy
2022-04-08 3:42 ` Kees Cook
1 sibling, 1 reply; 4+ messages in thread
From: Christophe Leroy @ 2022-02-23 17:17 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Andrew Morton, James E.J. Bottomley, Helge Deller, Arnd Bergmann,
Kees Cook, Greg Kroah-Hartman
Cc: linux-kernel, linuxppc-dev, linux-ia64, linux-parisc, linux-arch,
linux-mm
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". */
> -}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
2022-02-23 17:17 ` Christophe Leroy
@ 2022-04-07 18:32 ` Christophe Leroy
0 siblings, 0 replies; 4+ messages in thread
From: Christophe Leroy @ 2022-04-07 18:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Andrew Morton, James E.J. Bottomley, Helge Deller, Arnd Bergmann,
Kees Cook, Greg Kroah-Hartman
Cc: linux-kernel, linuxppc-dev, linux-ia64, linux-parisc, linux-arch,
linux-mm
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". */
>> -}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
2021-10-17 17:19 [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing() Christophe Leroy
2022-02-23 17:17 ` Christophe Leroy
@ 2022-04-08 3:42 ` Kees Cook
1 sibling, 0 replies; 4+ messages in thread
From: Kees Cook @ 2022-04-08 3:42 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Andrew Morton, James E.J. Bottomley, Helge Deller, Arnd Bergmann,
Greg Kroah-Hartman, linux-kernel, linuxppc-dev, linux-ia64,
linux-parisc, linux-arch, linux-mm
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
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-08 3:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-17 17:19 [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing() Christophe Leroy
2022-02-23 17:17 ` Christophe Leroy
2022-04-07 18:32 ` Christophe Leroy
2022-04-08 3:42 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).