Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Wed, 20 Jul 2022 11:43:37 -0700 | Subject | Re: [patch 00/38] x86/retbleed: Call depth tracking mitigation |
| |
On Wed, Jul 20, 2022 at 11:31 AM Steven Rostedt <rostedt@goodmis.org> 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". */ -} | |