Re: [patch 00/38] x86/retbleed: Call depth tracking mitigation

[Linus Torvalds]

On Wed, Jul 20, 2022 at 11:31 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> Thus, is there a way to keep this file from being entered into the
> return_sites section?

I think the whole concept is broken.

Testing known-broken code on the expectation that "this won't work
anyway, so we can jump off to code that is broken" is not acceptable.

*If* the test were to fail, it would start executing random code that
hasn't been relocated or fixed up properly.

So I think the whole concept is broken. It relies on the compiler
generating code that can work in a read-only data section, and it's
not clear that that is even physically possible (ie the data section
might be far enough away from a code section that any relocation just
fundamentally cannot happen).

I think it worked purely by mistake, because the code was simple
enough that it didn't need any relocation at all before. But even
without RETHUNK, that was never guaranteed, because any random tracing
or debug code or whatever could have made even that empty function
have code in it that just fundamentally wouldn't work in a non-text
section.

So honestly, I think that test should be removed as a "we used this,
it happened to work almost by mistake, but it doesn't work any more
and it is unfixably broken".

Maybe somebody can come up with an entirely different way to do that
test that isn't so broken, but if so, I think it's going to be using
some other machinery (eg bpf and explicitly marking it read-only and
non-executable), and removing this broken model is the right thing
regardless.

So unless somebody has some one-liner workaround, I really suspect the
fix is to remove all this. The amount of hackery to make it work in
the first place is kind of disgusting anyway.

Since this was a WARN_ONCE(), can you make sure that with this case
removed, nothing else triggers?

                Linus
 drivers/misc/lkdtm/Makefile | 11 -----------
 drivers/misc/lkdtm/lkdtm.h  |  3 ---
 drivers/misc/lkdtm/perms.c  |  7 -------
 drivers/misc/lkdtm/rodata.c | 11 -----------
 4 files changed, 32 deletions(-)

diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
index 2e0aa74ac185..4f1059f0cae9 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_64S_HASH_MMU)	+= 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 015e0484026b..e58f69077fcd 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -94,7 +94,4 @@ void __init lkdtm_perms_init(void);
 void __init lkdtm_usercopy_init(void);
 void __exit lkdtm_usercopy_exit(void);
 
-/* Special declaration for function-in-rodata. */
-void lkdtm_rodata_do_nothing(void);
-
 #endif
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index b93404d65650..d1a69ef865c2 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -191,12 +191,6 @@ static void lkdtm_EXEC_VMALLOC(void)
 	vfree(vmalloc_area);
 }
 
-static void lkdtm_EXEC_RODATA(void)
-{
-	execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing),
-			 CODE_AS_IS);
-}
-
 static void lkdtm_EXEC_USERSPACE(void)
 {
 	unsigned long user_addr;
@@ -280,7 +274,6 @@ static struct crashtype crashtypes[] = {
 	CRASHTYPE(EXEC_STACK),
 	CRASHTYPE(EXEC_KMALLOC),
 	CRASHTYPE(EXEC_VMALLOC),
-	CRASHTYPE(EXEC_RODATA),
 	CRASHTYPE(EXEC_USERSPACE),
 	CRASHTYPE(EXEC_NULL),
 	CRASHTYPE(ACCESS_USERSPACE),
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". */
-}