All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Andrew Morton <akpm@linux-foundation.org>,
	"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
	Helge Deller <deller@gmx.de>, Arnd Bergmann <arnd@arndb.de>,
	Kees Cook <keescook@chromium.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>,
	linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	linux-ia64@vger.kernel.org, linux-parisc@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-mm@kvack.org
Subject: [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
Date: Sun, 17 Oct 2021 19:19:47 +0200	[thread overview]
Message-ID: <fe36bf23fb14e7eff92a95a1092ed38edb01d5f5.1634491011.git.christophe.leroy@csgroup.eu> (raw)

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


WARNING: multiple messages have this Message-ID (diff)
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Andrew Morton <akpm@linux-foundation.org>,
	"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
	Helge Deller <deller@gmx.de>, Arnd Bergmann <arnd@arndb.de>,
	Kees Cook <keescook@chromium.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-arch@vger.kernel.org, linux-ia64@vger.kernel.org,
	linux-parisc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linuxppc-dev@lists.ozlabs.org
Subject: [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
Date: Sun, 17 Oct 2021 19:19:47 +0200	[thread overview]
Message-ID: <fe36bf23fb14e7eff92a95a1092ed38edb01d5f5.1634491011.git.christophe.leroy@csgroup.eu> (raw)

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


WARNING: multiple messages have this Message-ID (diff)
From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Andrew Morton <akpm@linux-foundation.org>,
	"James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>,
	Helge Deller <deller@gmx.de>, Arnd Bergmann <arnd@arndb.de>,
	Kees Cook <keescook@chromium.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>,
	linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	linux-ia64@vger.kernel.org, linux-parisc@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-mm@kvack.org
Subject: [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
Date: Sun, 17 Oct 2021 17:19:47 +0000	[thread overview]
Message-ID: <fe36bf23fb14e7eff92a95a1092ed38edb01d5f5.1634491011.git.christophe.leroy@csgroup.eu> (raw)

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

             reply	other threads:[~2021-10-17 17:20 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-17 17:19 Christophe Leroy [this message]
2021-10-17 17:19 ` [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing() Christophe Leroy
2021-10-17 17:19 ` Christophe Leroy
2022-02-23 17:17 ` Christophe Leroy
2022-02-23 17:17   ` Christophe Leroy
2022-02-23 17:17   ` Christophe Leroy
2022-04-07 18:32   ` Christophe Leroy
2022-04-07 18:32     ` Christophe Leroy
2022-04-07 18:32     ` Christophe Leroy
2022-04-08  3:42 ` Kees Cook
2022-04-08  3:42   ` Kees Cook
2022-04-08  3:42   ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fe36bf23fb14e7eff92a95a1092ed38edb01d5f5.1634491011.git.christophe.leroy@csgroup.eu \
    --to=christophe.leroy@csgroup.eu \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=benh@kernel.crashing.org \
    --cc=deller@gmx.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=keescook@chromium.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-ia64@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-parisc@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=paulus@samba.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.