livepatch,module: Remove .klp.arch and module_disable_ro()

[ ... snip ... ]

kernel/livepatch/core.c +4 -2

index 40cfac8156fd..5fda3afc0285 100644
 
[ ... snip ... ]
 
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+			  const char *shstrtab, const char *strtab,
+			  unsigned int symndx, struct module *pmod,
+			  const char *objname)
 {
 	int i, cnt, ret = 0;
-	const char *objname, *secname;
 	char sec_objname[MODULE_NAME_LEN];
 	Elf_Shdr *sec;
 
-	if (WARN_ON(!klp_is_object_loaded(obj)))

[ ... snip ... ]

kernel/module.c +0 -2

index 646f1e2330d2..d36ea8a8c3ec 100644
@@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
-		/* Livepatch relocation sections are applied by livepatch */
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			continue;

Quoted 21 lines · Josh Poimboeuf

[ ... snip ... ]

kernel/livepatch/core.c +4 -2

index 40cfac8156fd..5fda3afc0285 100644
 
[ ... snip ... ]
 
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+			  const char *shstrtab, const char *strtab,
+			  unsigned int symndx, struct module *pmod,
+			  const char *objname)
 {
 	int i, cnt, ret = 0;
-	const char *objname, *secname;
 	char sec_objname[MODULE_NAME_LEN];
 	Elf_Shdr *sec;
 
-	if (WARN_ON(!klp_is_object_loaded(obj)))

kernel/livepatch/core.c static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod) · line 283

Quoted 10 lines

kernel/livepatch/core.c +2 -7

-		return -EINVAL;
-
-	objname = klp_is_module(obj) ? obj->name : "vmlinux";
-
 	/* For each klp relocation section */
-	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
-		sec = pmod->klp_info->sechdrs + i;
-		secname = pmod->klp_info->secstrings + sec->sh_name;
+	for (i = 1; i < ehdr->e_shnum; i++) {
+		sec = sechdrs + i;

Hi Josh, minor bug:

Note the for loop through the section headers in klp_write_relocations(), but its calling function ...

Quoted 13 lines

[ ... snip ... ]

kernel/module.c +0 -2

index 646f1e2330d2..d36ea8a8c3ec 100644
@@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
-		/* Livepatch relocation sections are applied by livepatch */
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			continue;

kernel/module.c static int apply_relocations(struct module *mod, const struct load_info *info) · line 2345

Quoted 10 lines

kernel/module.c +5 -2

-
-		if (info->sechdrs[i].sh_type == SHT_REL)
+			err = klp_write_relocations(info->hdr, info->sechdrs,
+						    info->secstrings,
+						    info->strtab,
+						    info->index.sym, mod, NULL);
+		else if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);
 		else if (info->sechdrs[i].sh_type == SHT_RELA)

... apply_relocations() is also iterating over the section headers (the diff context doesn't show it here, but i is an incrementing index over sechdrs[]).

So if there is more than one KLP relocation section, we'll process them multiple times. At least the x86 relocation code will detect this and fail the module load with an invalid relocation (existing value not zero).

Quoted 66 lines · Joe Lawrence

Quoted 21 lines · Josh Poimboeuf

[ ... snip ... ]

kernel/livepatch/core.c +4 -2

index 40cfac8156fd..5fda3afc0285 100644
 
[ ... snip ... ]
 
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+			  const char *shstrtab, const char *strtab,
+			  unsigned int symndx, struct module *pmod,
+			  const char *objname)
 {
 	int i, cnt, ret = 0;
-	const char *objname, *secname;
 	char sec_objname[MODULE_NAME_LEN];
 	Elf_Shdr *sec;
 
-	if (WARN_ON(!klp_is_object_loaded(obj)))

kernel/livepatch/core.c

Quoted 10 lines

kernel/livepatch/core.c +2 -7

-		return -EINVAL;
-
-	objname = klp_is_module(obj) ? obj->name : "vmlinux";
-
 	/* For each klp relocation section */
-	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
-		sec = pmod->klp_info->sechdrs + i;
-		secname = pmod->klp_info->secstrings + sec->sh_name;
+	for (i = 1; i < ehdr->e_shnum; i++) {
+		sec = sechdrs + i;

Hi Josh, minor bug:

Note the for loop through the section headers in klp_write_relocations(), but its calling function ...

Quoted 13 lines

[ ... snip ... ]

kernel/module.c +0 -2

index 646f1e2330d2..d36ea8a8c3ec 100644
@@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
-		/* Livepatch relocation sections are applied by livepatch */
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			continue;

kernel/module.c static int apply_relocations(struct module *mod, const struct load_info *info) · line 2345

Quoted 10 lines

kernel/module.c +5 -2

-
-		if (info->sechdrs[i].sh_type == SHT_REL)
+			err = klp_write_relocations(info->hdr, info->sechdrs,
+						    info->secstrings,
+						    info->strtab,
+						    info->index.sym, mod, NULL);
+		else if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);
 		else if (info->sechdrs[i].sh_type == SHT_RELA)

... apply_relocations() is also iterating over the section headers (the diff context doesn't show it here, but i is an incrementing index over sechdrs[]).

Quoted 4 lines

So if there is more than one KLP relocation section, we'll process them multiple times. At least the x86 relocation code will detect this and fail the module load with an invalid relocation (existing value not zero).

Ah, yes, good catch!

Quoted 9 lines

Quoted 8 lines

... apply_relocations() is also iterating over the section headers (the diff context doesn't show it here, but i is an incrementing index over sechdrs[]).

So if there is more than one KLP relocation section, we'll process them multiple times. At least the x86 relocation code will detect this and fail the module load with an invalid relocation (existing value not zero).

Quoted 2 lines

Ah, yes, good catch!

The same test case passed with a small modification to push the foreach KLP section part to a kernel/livepatch/core.c local function and exposing the klp_resolve_symbols() + apply_relocate_add() for a given section to kernel/module.c. Something like following...

Quoted 9 lines · Joe Lawrence

Quoted 9 lines

Quoted 8 lines

... apply_relocations() is also iterating over the section headers (the diff context doesn't show it here, but i is an incrementing index over sechdrs[]).

So if there is more than one KLP relocation section, we'll process them multiple times. At least the x86 relocation code will detect this and fail the module load with an invalid relocation (existing value not zero).

Quoted 7 lines

Quoted 2 lines

Ah, yes, good catch!

The same test case passed with a small modification to push the foreach KLP section part to a kernel/livepatch/core.c local function and exposing the klp_resolve_symbols() + apply_relocate_add() for a given section to kernel/module.c. Something like following...

I came up with something very similar, though I named them klp_apply_object_relocs() and klp_apply_section_relocs() and changed the argument order a bit (module first). Since it sounds like you have a test, could you try this one?

Quoted 18 lines · Josh Poimboeuf

Quoted 16 lines · Joe Lawrence

Quoted 11 lines

Quoted 8 lines

... apply_relocations() is also iterating over the section headers (the diff context doesn't show it here, but i is an incrementing index over sechdrs[]).

So if there is more than one KLP relocation section, we'll process them multiple times. At least the x86 relocation code will detect this and fail the module load with an invalid relocation (existing value not zero).

Ah, yes, good catch!

The same test case passed with a small modification to push the foreach KLP section part to a kernel/livepatch/core.c local function and exposing the klp_resolve_symbols() + apply_relocate_add() for a given section to kernel/module.c. Something like following...

Quoted 5 lines

I came up with something very similar, though I named them klp_apply_object_relocs() and klp_apply_section_relocs() and changed the argument order a bit (module first). Since it sounds like you have a test, could you try this one?

LGTM. I have a few klp-convert selftests that I've been slowly tinkering on and they all load/run successfully with this version. :)

Quoted 9 lines · Joe Lawrence

Quoted 9 lines

Quoted 8 lines

... apply_relocations() is also iterating over the section headers (the diff context doesn't show it here, but i is an incrementing index over sechdrs[]).

So if there is more than one KLP relocation section, we'll process them multiple times. At least the x86 relocation code will detect this and fail the module load with an invalid relocation (existing value not zero).

Quoted 7 lines

Quoted 2 lines

Ah, yes, good catch!

The same test case passed with a small modification to push the foreach KLP section part to a kernel/livepatch/core.c local function and exposing the klp_resolve_symbols() + apply_relocate_add() for a given section to kernel/module.c. Something like following...

I came up with something very similar, though I named them klp_apply_object_relocs() and klp_apply_section_relocs() and changed the argument order a bit (module first). Since it sounds like you have a test, could you try this one?

include/linux/livepatch.h +0 -0

index 533359e48c39..fb1a3de39726 100644
 
[ ... snip ... ]

@@ -245,10 +245,10 @@ static inline void klp_update_patch_state(struct task_struct *task) {}
 static inline void klp_copy_process(struct task_struct *child) {}

Quoted 21 lines · Josh Poimboeuf

[ ... snip ... ]

kernel/livepatch/core.c +4 -2

index 40cfac8156fd..5fda3afc0285 100644
 
[ ... snip ... ]
 
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+			  const char *shstrtab, const char *strtab,
+			  unsigned int symndx, struct module *pmod,
+			  const char *objname)
 {
 	int i, cnt, ret = 0;
-	const char *objname, *secname;
 	char sec_objname[MODULE_NAME_LEN];
 	Elf_Shdr *sec;
 
-	if (WARN_ON(!klp_is_object_loaded(obj)))

kernel/livepatch/core.c static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod) · line 283

Quoted 10 lines

kernel/livepatch/core.c +2 -7

-		return -EINVAL;
-
-	objname = klp_is_module(obj) ? obj->name : "vmlinux";
-
 	/* For each klp relocation section */
-	for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
-		sec = pmod->klp_info->sechdrs + i;
-		secname = pmod->klp_info->secstrings + sec->sh_name;
+	for (i = 1; i < ehdr->e_shnum; i++) {
+		sec = sechdrs + i;

Hi Josh, minor bug:

Note the for loop through the section headers in klp_write_relocations(), but its calling function ...

Quoted 13 lines

[ ... snip ... ]

kernel/module.c +0 -2

index 646f1e2330d2..d36ea8a8c3ec 100644
@@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
-		/* Livepatch relocation sections are applied by livepatch */
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
-			continue;

kernel/module.c static int apply_relocations(struct module *mod, const struct load_info *info) · line 2345

Quoted 10 lines

kernel/module.c +5 -2

-
-		if (info->sechdrs[i].sh_type == SHT_REL)
+			err = klp_write_relocations(info->hdr, info->sechdrs,
+						    info->secstrings,
+						    info->strtab,
+						    info->index.sym, mod, NULL);
+		else if (info->sechdrs[i].sh_type == SHT_REL)
 			err = apply_relocate(info->sechdrs, info->strtab,
 					     info->index.sym, i, mod);
 		else if (info->sechdrs[i].sh_type == SHT_RELA)

... apply_relocations() is also iterating over the section headers (the diff context doesn't show it here, but i is an incrementing index over sechdrs[]).

So if there is more than one KLP relocation section, we'll process them multiple times. At least the x86 relocation code will detect this and fail the module load with an invalid relocation (existing value not zero).

arch/s390/kernel/module.c +11 -0

+int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
+		       unsigned int symindex, unsigned int relsec,
+		       struct module *me)
+{
+	int ret;
+	bool early = me->state == MODULE_STATE_UNFORMED;
+	void *(*write)(void *, const void *, size_t) = memcpy;
+
+	if (!early) {
+		write = s390_kernel_write;
+		mutex_lock(&text_mutex);

Quoted 11 lines

arch/s390/kernel/module.c +11 -0

+int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
+		       unsigned int symindex, unsigned int relsec,
+		       struct module *me)
+{
+	int ret;
+	bool early = me->state == MODULE_STATE_UNFORMED;
+	void *(*write)(void *, const void *, size_t) = memcpy;
+
+	if (!early) {
+		write = s390_kernel_write;
+		mutex_lock(&text_mutex);

arch/s390/kernel/module.c int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab, · line 471

Quoted 10 lines

arch/s390/kernel/module.c +10 -0

+	}
+
+	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				   write);
+
+	if (!early)
+		mutex_unlock(&text_mutex);
+
+	return ret;
+}

It means that you can take text_mutex the second time here because it is still taken in klp_init_object_loaded(). It is removed later in the series though. The same applies for x86 patch.

Also, s390_kernel_write() grabs s390_kernel_write_lock spinlock before writing anything, so maybe text_mutex is not really needed as long as s390_kernel_write is called everywhere for text patching.

From: Peter Zijlstra <[email protected]>

Because of late module patching, a livepatch module needs to be able to apply some of its relocations well after it has been loaded. Instead of playing games with module_{dis,en}able_ro(), use existing text poking mechanisms to apply relocations after module loading.

So far only x86, s390 and Power have HAVE_LIVEPATCH but only the first two also have STRICT_MODULE_RWX.

This will allow removal of the last module_disable_ro() usage in livepatch. The ultimate goal is to completely disallow making executable mappings writable.

Also, for the late patching case, use text_mutex, which is supposed to be held for all runtime text patching operations.

[ jpoimboe: Split up patches. Use mod state to determine whether
memcpy() can be used. Add text_mutex. Make it build. ]

Cc [email protected]

Cc [email protected]

Suggested-by Josh Poimboeuf <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/s390/kernel/module.c | 125 ++++++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 46 deletions(-)

Quoted 20 lines · On Fri, 17 Apr 2020 09:04:31 -0500 Josh Poimboeuf <[email protected]> wrote:

From: Peter Zijlstra <[email protected]>

Because of late module patching, a livepatch module needs to be able to apply some of its relocations well after it has been loaded. Instead of playing games with module_{dis,en}able_ro(), use existing text poking mechanisms to apply relocations after module loading.

So far only x86, s390 and Power have HAVE_LIVEPATCH but only the first two also have STRICT_MODULE_RWX.

This will allow removal of the last module_disable_ro() usage in livepatch. The ultimate goal is to completely disallow making executable mappings writable.

Also, for the late patching case, use text_mutex, which is supposed to be held for all runtime text patching operations.

[ jpoimboe: Split up patches. Use mod state to determine whether
memcpy() can be used. Add text_mutex. Make it build. ]

Quoted 8 lines

Cc [email protected]

Cc [email protected]

Suggested-by Josh Poimboeuf <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

---
 arch/s390/kernel/module.c | 125 ++++++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 46 deletions(-)

Sorry, just noticed this. Heiko will return next month, and I'm not really familiar with s390 livepatching. Adding Vasily, he might have some more insight.

So, I might be completely wrong here, but using s390_kernel_write() for writing to anything other than 1:1 mapped kernel, should go horribly wrong, as that runs w/o DAT. It would allow to bypass DAT write protection, which I assume is why you want to use it, but it should not work on module text section, as that would be in vmalloc space and not 1:1 mapped kernel memory.

Not quite sure how to test / trigger this, did this really work for you on s390?

Quoted 11 lines

Sorry, just noticed this. Heiko will return next month, and I'm not really familiar with s390 livepatching. Adding Vasily, he might have some more insight.

So, I might be completely wrong here, but using s390_kernel_write() for writing to anything other than 1:1 mapped kernel, should go horribly wrong, as that runs w/o DAT. It would allow to bypass DAT write protection, which I assume is why you want to use it, but it should not work on module text section, as that would be in vmalloc space and not 1:1 mapped kernel memory.

Quoted 2 lines

Not quite sure how to test / trigger this, did this really work for you on s390?

OK, using s390_kernel_write() as default write function for module relocation seems to work fine for me, so apparently I am missing / mixing up something. Sorry for the noise, please ignore my concern.

Quoted 11 lines · Gerald Schaefer

Quoted 11 lines

Sorry, just noticed this. Heiko will return next month, and I'm not really familiar with s390 livepatching. Adding Vasily, he might have some more insight.

So, I might be completely wrong here, but using s390_kernel_write() for writing to anything other than 1:1 mapped kernel, should go horribly wrong, as that runs w/o DAT. It would allow to bypass DAT write protection, which I assume is why you want to use it, but it should not work on module text section, as that would be in vmalloc space and not 1:1 mapped kernel memory.

Quoted 6 lines

Quoted 2 lines

Not quite sure how to test / trigger this, did this really work for you on s390?

OK, using s390_kernel_write() as default write function for module relocation seems to work fine for me, so apparently I am missing / mixing up something. Sorry for the noise, please ignore my concern.

Hi Gerald,

I think you were right. Joe found the below panic with his klp-convert tests.

Your test was probably the early module loading case (normal relocations before write protection), rather than the late case. Not sure why that would work, but calling s390_kernel_write() late definitely seems to be broken.

Is there some other way to write vmalloc'ed s390 text without using module_disable_ro()?

[   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
[   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
[   50.294480] Fault in home space mode while using kernel ASCE.
[   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
[   50.294557] Oops: 0004 ilc:3 [#1] SMP
[   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
[   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
[   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
[   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
[   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
[   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
[   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
[   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
[   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
[   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
[   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
[   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
[   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
[   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
[   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
[   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
[   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
[   50.295369] Call Trace:
[   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
[   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
[   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
[   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
[   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
[   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
[   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]

[   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
[   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
[   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
[   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
[   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
[   50.295416] Last Breaking-Event-Address:
[   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
[   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops

Quoted 65 lines · On Wed, 22 Apr 2020 14:46:05 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 11 lines · Gerald Schaefer

Quoted 11 lines

Sorry, just noticed this. Heiko will return next month, and I'm not really familiar with s390 livepatching. Adding Vasily, he might have some more insight.

So, I might be completely wrong here, but using s390_kernel_write() for writing to anything other than 1:1 mapped kernel, should go horribly wrong, as that runs w/o DAT. It would allow to bypass DAT write protection, which I assume is why you want to use it, but it should not work on module text section, as that would be in vmalloc space and not 1:1 mapped kernel memory.

Quoted 6 lines

Quoted 2 lines

Not quite sure how to test / trigger this, did this really work for you on s390?

OK, using s390_kernel_write() as default write function for module relocation seems to work fine for me, so apparently I am missing / mixing up something. Sorry for the noise, please ignore my concern.

Hi Gerald,

I think you were right. Joe found the below panic with his klp-convert tests.

Your test was probably the early module loading case (normal relocations before write protection), rather than the late case. Not sure why that would work, but calling s390_kernel_write() late definitely seems to be broken.

Is there some other way to write vmalloc'ed s390 text without using module_disable_ro()?

[   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
[   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
[   50.294480] Fault in home space mode while using kernel ASCE.
[   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
[   50.294557] Oops: 0004 ilc:3 [#1] SMP
[   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
[   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
[   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
[   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
[   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
[   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
[   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
[   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
[   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
[   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
[   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
[   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
[   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
[   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
[   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
[   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
[   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
[   50.295369] Call Trace:
[   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
[   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
[   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
[   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
[   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
[   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
[   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]

Quoted 9 lines

[   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
[   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
[   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
[   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
[   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
[   50.295416] Last Breaking-Event-Address:
[   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
[   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops

Hi Josh,

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

There is also some good news. While thinking about how to use "sturg" for vmalloc addresses, I came up with the idea to use "lra" (load real address) before that. Then I found out that we already do exactly that in the inline assembly, so all should be fine. Well, maybe the comment for s390_kernel_write() could be improved...

Vasily also found out that we apparently already use s390_kernel_write() for module text, for alternatives, so I guess we can safely assume that it should work fine in principle.

Quoted 48 lines

Quoted 33 lines

[   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
[   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
[   50.294480] Fault in home space mode while using kernel ASCE.
[   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
[   50.294557] Oops: 0004 ilc:3 [#1] SMP
[   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
[   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
[   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
[   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
[   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
[   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
[   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
[   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
[   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
[   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
[   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
[   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
[   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
[   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
[   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
[   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
[   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
[   50.295369] Call Trace:
[   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
[   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
[   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
[   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
[   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
[   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
[   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]

Quoted 8 lines

[   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
[   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
[   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
[   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
[   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
[   50.295416] Last Breaking-Event-Address:
[   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
[   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

Quoted 5 lines

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

The call trace above also suggests that it is not a late relocation, no? The path is from KLP module init function through klp_enable_patch. It should mean that the to-be-patched object is loaded (it must be a module thanks to a check klp_init_object_loaded(), vmlinux relocations were processed earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() should be used. What are we missing?

Quoted 12 lines

Quoted 11 lines

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

Quoted 8 lines

The call trace above also suggests that it is not a late relocation, no? The path is from KLP module init function through klp_enable_patch. It should mean that the to-be-patched object is loaded (it must be a module thanks to a check klp_init_object_loaded(), vmlinux relocations were processed earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() should be used. What are we missing?

I'm also scratching my head. It _should_ be using s390_kernel_write() based on the module state, but I don't see that on the stack trace.

This trace (and Gerald's comment) seem to imply it's using __builtin_memcpy(), which might expected for UNFORMED state.

Weird...

Quoted 21 lines · Josh Poimboeuf

Quoted 12 lines

Quoted 11 lines

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

Quoted 8 lines

The call trace above also suggests that it is not a late relocation, no? The path is from KLP module init function through klp_enable_patch. It should mean that the to-be-patched object is loaded (it must be a module thanks to a check klp_init_object_loaded(), vmlinux relocations were processed earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() should be used. What are we missing?

Quoted 7 lines

I'm also scratching my head. It _should_ be using s390_kernel_write() based on the module state, but I don't see that on the stack trace.

This trace (and Gerald's comment) seem to imply it's using __builtin_memcpy(), which might expected for UNFORMED state.

Weird...

Mystery solved:

  $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
  apply_rela+0x16a/0x520:
  apply_rela at arch/s390/kernel/module.c:336

which corresponds to the following code in apply_rela():

	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
		if (info->plt_initialized == 0) {
			unsigned int *ip;
			ip = me->core_layout.base + me->arch.plt_offset +
				info->plt_offset;
			ip[0] = 0x0d10e310;	/* basr 1,0  */
			ip[1] = 0x100a0004;	/* lg	1,10(1) */

Notice how it's writing directly to text... oops.

Quoted 18 lines · Josh Poimboeuf

Mystery solved:

  $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
  apply_rela+0x16a/0x520:
  apply_rela at arch/s390/kernel/module.c:336

which corresponds to the following code in apply_rela():

	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
		if (info->plt_initialized == 0) {
			unsigned int *ip;
			ip = me->core_layout.base + me->arch.plt_offset +
				info->plt_offset;
			ip[0] = 0x0d10e310;	/* basr 1,0  */
			ip[1] = 0x100a0004;	/* lg	1,10(1) */

Quoted 1 line

Notice how it's writing directly to text... oops.

Here's a fix, using write() for the PLT and the GOT.

arch/s390/kernel/module.c +1 -1

index 2798329ebb74..fe446f42818f 100644
@@ -297,7 +297,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 
 			gotent = me->core_layout.base + me->arch.got_offset +
 				info->got_offset;
-			*gotent = val;
+			write(gotent, &val, sizeof(*gotent));
 			info->got_initialized = 1;
 		}
 		val = info->got_offset + rela->r_addend;

Quoted 14 lines

arch/s390/kernel/module.c +1 -1

index 2798329ebb74..fe446f42818f 100644
@@ -297,7 +297,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 
 			gotent = me->core_layout.base + me->arch.got_offset +
 				info->got_offset;
-			*gotent = val;
+			write(gotent, &val, sizeof(*gotent));
 			info->got_initialized = 1;
 		}
 		val = info->got_offset + rela->r_addend;

arch/s390/kernel/module.c static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab, · line 336

Quoted 8 lines

arch/s390/kernel/module.c +2 -1

@@ -330,25 +330,29 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	case R_390_PLTOFF32:	/* 32 bit offset from GOT to PLT. */
 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
 		if (info->plt_initialized == 0) {
-			unsigned int *ip;
+			unsigned int *ip, insn[5];
+
 			ip = me->core_layout.base + me->arch.plt_offset +
 				info->plt_offset;

Would it be too paranoid to declare:

const unsigned int *ip = me->core_layout.base +

						 me->arch.plt_offset +

info->plt_offset;

That would trip an assignment to read-only error if someone were tempted to write directly through the pointer in the future. Ditto for Elf_Addr *gotent pointer in the R_390_GOTPLTENT case.

Quoted 48 lines · Josh Poimboeuf

Quoted 21 lines · Josh Poimboeuf

Quoted 12 lines

Quoted 11 lines

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

Quoted 8 lines

The call trace above also suggests that it is not a late relocation, no? The path is from KLP module init function through klp_enable_patch. It should mean that the to-be-patched object is loaded (it must be a module thanks to a check klp_init_object_loaded(), vmlinux relocations were processed earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() should be used. What are we missing?

Quoted 7 lines

I'm also scratching my head. It _should_ be using s390_kernel_write() based on the module state, but I don't see that on the stack trace.

This trace (and Gerald's comment) seem to imply it's using __builtin_memcpy(), which might expected for UNFORMED state.

Weird...

Mystery solved:

  $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
  apply_rela+0x16a/0x520:
  apply_rela at arch/s390/kernel/module.c:336

which corresponds to the following code in apply_rela():

	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
		if (info->plt_initialized == 0) {
			unsigned int *ip;
			ip = me->core_layout.base + me->arch.plt_offset +
				info->plt_offset;
			ip[0] = 0x0d10e310;	/* basr 1,0  */
			ip[1] = 0x100a0004;	/* lg	1,10(1) */

Quoted 1 line

Notice how it's writing directly to text... oops.

Here's a fix, using write() for the PLT and the GOT.

arch/s390/kernel/module.c +12 -8

index 2798329ebb74..fe446f42818f 100644
@@ -297,7 +297,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 
 			gotent = me->core_layout.base + me->arch.got_offset +
 				info->got_offset;
-			*gotent = val;
+			write(gotent, &val, sizeof(*gotent));
 			info->got_initialized = 1;
 		}
 		val = info->got_offset + rela->r_addend;

@@ -330,25 +330,29 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	case R_390_PLTOFF32:	/* 32 bit offset from GOT to PLT. */
 	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
 		if (info->plt_initialized == 0) {
-			unsigned int *ip;
+			unsigned int *ip, insn[5];
+
 			ip = me->core_layout.base + me->arch.plt_offset +
 				info->plt_offset;
-			ip[0] = 0x0d10e310;	/* basr 1,0  */
-			ip[1] = 0x100a0004;	/* lg	1,10(1) */
+
+			insn[0] = 0x0d10e310;	/* basr 1,0  */
+			insn[1] = 0x100a0004;	/* lg	1,10(1) */
 			if (IS_ENABLED(CONFIG_EXPOLINE) && !nospec_disable) {
 				unsigned int *ij;
 				ij = me->core_layout.base +
 					me->arch.plt_offset +
 					me->arch.plt_size - PLT_ENTRY_SIZE;
-				ip[2] = 0xa7f40000 +	/* j __jump_r1 */
+				insn[2] = 0xa7f40000 +	/* j __jump_r1 */
 					(unsigned int)(u16)
 					(((unsigned long) ij - 8 -
 					  (unsigned long) ip) / 2);
 			} else {
-				ip[2] = 0x07f10000;	/* br %r1 */
+				insn[2] = 0x07f10000;	/* br %r1 */
 			}
-			ip[3] = (unsigned int) (val >> 32);
-			ip[4] = (unsigned int) val;
+			insn[3] = (unsigned int) (val >> 32);
+			insn[4] = (unsigned int) val;
+
+			write(ip, insn, sizeof(insn));
 			info->plt_initialized = 1;
 		}
 		if (r_type == R_390_PLTOFF16 ||

Quoted 1 line

Here's a fix, using write() for the PLT and the GOT.

Are you going to provide a proper patch?

Quoted 21 lines · Josh Poimboeuf

Quoted 12 lines

Quoted 11 lines

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

Quoted 8 lines

The call trace above also suggests that it is not a late relocation, no? The path is from KLP module init function through klp_enable_patch. It should mean that the to-be-patched object is loaded (it must be a module thanks to a check klp_init_object_loaded(), vmlinux relocations were processed earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() should be used. What are we missing?

Quoted 7 lines

I'm also scratching my head. It _should_ be using s390_kernel_write() based on the module state, but I don't see that on the stack trace.

This trace (and Gerald's comment) seem to imply it's using __builtin_memcpy(), which might expected for UNFORMED state.

Weird...

Mystery solved:

  $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
  apply_rela+0x16a/0x520:
  apply_rela at arch/s390/kernel/module.c:336

which corresponds to the following code in apply_rela():

	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
		if (info->plt_initialized == 0) {
			unsigned int *ip;
			ip = me->core_layout.base + me->arch.plt_offset +
				info->plt_offset;
			ip[0] = 0x0d10e310;	/* basr 1,0  */
			ip[1] = 0x100a0004;	/* lg	1,10(1) */

Notice how it's writing directly to text... oops.

Quoted 48 lines · Josh Poimboeuf

Quoted 21 lines · Josh Poimboeuf

Quoted 12 lines

Quoted 11 lines

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

Quoted 8 lines

The call trace above also suggests that it is not a late relocation, no? The path is from KLP module init function through klp_enable_patch. It should mean that the to-be-patched object is loaded (it must be a module thanks to a check klp_init_object_loaded(), vmlinux relocations were processed earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() should be used. What are we missing?

Quoted 7 lines

I'm also scratching my head. It _should_ be using s390_kernel_write() based on the module state, but I don't see that on the stack trace.

This trace (and Gerald's comment) seem to imply it's using __builtin_memcpy(), which might expected for UNFORMED state.

Weird...

Mystery solved:

  $ CROSS_COMPILE=s390x-linux-gnu- scripts/faddr2line vmlinux apply_rela+0x16a/0x520
  apply_rela+0x16a/0x520:
  apply_rela at arch/s390/kernel/module.c:336

which corresponds to the following code in apply_rela():

	case R_390_PLTOFF64:	/* 16 bit offset from GOT to PLT. */
		if (info->plt_initialized == 0) {
			unsigned int *ip;
			ip = me->core_layout.base + me->arch.plt_offset +
				info->plt_offset;
			ip[0] = 0x0d10e310;	/* basr 1,0  */
			ip[1] = 0x100a0004;	/* lg	1,10(1) */

Quoted 2 lines

Notice how it's writing directly to text... oops.

This is more of note for the future, but when/if we add livepatch support on arm64 we'll need to make the very same adjustment there as well. See the following pattern:

arch/arm64/kernel/module.c:

reloc_insn_movw()
reloc_insn_imm()
reloc_insn_adrp()

    *place = cpu_to_le32(insn);

maybe something like aarch64_insn_patch_text_nosync() could be used there, I dunno. (It looks like ftrace and jump_labels are using that interface.)

This is outside the scope of the patchset, but I thought I'd mention it as I was curious to see how other arches were currently handling their relocation updates.

Quoted 17 lines · Joe Lawrence

On Thu, Apr 23, 2020 at 01:10:30PM -0500, Josh Poimboeuf wrote:
This is more of note for the future, but when/if we add livepatch support on arm64 we'll need to make the very same adjustment there as well. See the following pattern:

arch/arm64/kernel/module.c:

   reloc_insn_movw()
   reloc_insn_imm()
   reloc_insn_adrp()

     *place = cpu_to_le32(insn);

maybe something like aarch64_insn_patch_text_nosync() could be used there, I dunno. (It looks like ftrace and jump_labels are using that interface.)

Quoted 3 lines

This is outside the scope of the patchset, but I thought I'd mention it as I was curious to see how other arches were currently handling their relocation updates.

True... I suspect your klp-convert selftests will catch that?

Quoted 86 lines · Gerald Schaefer

Quoted 65 lines · On Wed, 22 Apr 2020 14:46:05 -0500 Josh Poimboeuf <[email protected]> wrote:

Quoted 11 lines · Gerald Schaefer

Quoted 11 lines

Sorry, just noticed this. Heiko will return next month, and I'm not really familiar with s390 livepatching. Adding Vasily, he might have some more insight.

So, I might be completely wrong here, but using s390_kernel_write() for writing to anything other than 1:1 mapped kernel, should go horribly wrong, as that runs w/o DAT. It would allow to bypass DAT write protection, which I assume is why you want to use it, but it should not work on module text section, as that would be in vmalloc space and not 1:1 mapped kernel memory.

Quoted 6 lines

Quoted 2 lines

Not quite sure how to test / trigger this, did this really work for you on s390?

OK, using s390_kernel_write() as default write function for module relocation seems to work fine for me, so apparently I am missing / mixing up something. Sorry for the noise, please ignore my concern.

Hi Gerald,

I think you were right. Joe found the below panic with his klp-convert tests.

Your test was probably the early module loading case (normal relocations before write protection), rather than the late case. Not sure why that would work, but calling s390_kernel_write() late definitely seems to be broken.

Is there some other way to write vmalloc'ed s390 text without using module_disable_ro()?

[   50.294476] Unable to handle kernel pointer dereference in virtual kernel address space
[   50.294479] Failing address: 000003ff8015b000 TEID: 000003ff8015b407
[   50.294480] Fault in home space mode while using kernel ASCE.
[   50.294483] AS:000000006cef0007 R3:000000007e2c4007 S:0000000003ccb800 P:0000 00000257321d
[   50.294557] Oops: 0004 ilc:3 [#1] SMP
[   50.294561] Modules linked in: test_klp_convert1(K+) test_klp_convert_mod ghash_s390 prng xts aes_s390 des_s390 libdes sha512_s390 vmur zcrypt_cex4 ip_tables xfs libcrc32c dasd_fba_mod qeth_l2 dasd_eckd_mod dasd_mod qeth lcs ctcm qdio cc
wgroup fsm dm_mirror dm_region_hash dm_log dm_mod pkey zcrypt [last unloaded: test_klp_atomic_replace]
[   50.294576] CPU: 0 PID: 1743 Comm: modprobe Tainted: G              K   5.6.0 + #2
[   50.294579] Hardware name: IBM 2964 N96 400 (z/VM 6.4.0)
[   50.294583] Krnl PSW : 0704e00180000000 000000006bf6be0a (apply_rela+0x2ba/0x 4e0)
[   50.294589]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI: 0 EA:3
[   50.294684] Krnl GPRS: 000003ff80147010 000003e0001b9588 000003ff8015c168 000 003ff8015b19a
[   50.294686]            000003ff8015b07c 0d10e310100a0004 000003ff80147010 000 00000000000a0
[   50.294687]            000003ff8015e588 000003ff8015e5e8 000003ff8015d300 000 0003b00000014
[   50.294698]            000000007a663000 000000006c6bbb80 000003e0009a7918 000 003e0009a78b8
[   50.294707] Krnl Code: 000000006bf6bdf8: e350d0080004        lg      %r5,8(%r 13)
[   50.294707]            000000006bf6bdfe: e34010080008        ag      %r4,8(%r 1)
[   50.294707]           #000000006bf6be04: e340a2000008        ag      %r4,512( %r10)
[   50.294707]           >000000006bf6be0a: e35040000024        stg     %r5,0(%r 4)
[   50.294707]            000000006bf6be10: c050007c6136        larl    %r5,0000 00006cef807c
[   50.294707]            000000006bf6be16: e35050000012        lt      %r5,0(%r 5)
[   50.294707]            000000006bf6be1c: a78400a6            brc     8,000000 006bf6bf68
[   50.294707]            000000006bf6be20: a55e07f1            llilh   %r5,2033
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 01.
01: HCPGSP2629I The virtual machine is placed in CP mode due to a SIGP stop from CPU 00.
[   50.295369] Call Trace:
[   50.295372]  [<000000006bf6be0a>] apply_rela+0x2ba/0x4e0
[   50.295376]  [<000000006bf6c5c8>] apply_relocate_add+0xe0/0x138
[   50.295378]  [<000000006c0229a0>] klp_apply_section_relocs+0xe8/0x128
[   50.295380]  [<000000006c022b4c>] klp_apply_object_relocs+0x9c/0xd0
[   50.295382]  [<000000006c022bb0>] klp_init_object_loaded+0x30/0x138
[   50.295384]  [<000000006c023052>] klp_enable_patch+0x39a/0x870
[   50.295387]  [<000003ff8015b0da>] test_klp_convert_init+0x22/0x50 [test_klp_convert1]

Quoted 9 lines

[   50.295389]  [<000000006bf54838>] do_one_initcall+0x40/0x1f0
[   50.295391]  [<000000006c04d610>] do_init_module+0x70/0x280
[   50.295392]  [<000000006c05002a>] load_module+0x1aba/0x1d10
[   50.295394]  [<000000006c0504c4>] __do_sys_finit_module+0xa4/0xe8
[   50.295416]  [<000000006c6b5742>] system_call+0x2aa/0x2c8
[   50.295416] Last Breaking-Event-Address:
[   50.295418]  [<000000006c6b6aa0>] __s390_indirect_jump_r4+0x0/0xc
[   50.295421] Kernel panic - not syncing: Fatal exception: panic_on_oops

Hi Josh,

this is strange. While I would have expected an exception similar to this, it really should have happened on the "sturg" instruction which does the DAT-off store in s390_kernel_write(), and certainly not with an ID of 0004 (protection). However, in your case, it happens on a normal store instruction, with 0004 indicating a protection exception.

Quoted 5 lines

This is more like what I would expect e.g. in the case where you do _not_ use the s390_kernel_write() function for RO module text patching, but rather normal memory access. So I am pretty sure that this is not related to the s390_kernel_write(), but some other issue, maybe some place left where you still use normal memory access?

The call trace above also suggests that it is not a late relocation, no?

The path is from KLP module init function through klp_enable_patch. It should mean that the to-be-patched object is loaded (it must be a module thanks to a check klp_init_object_loaded(), vmlinux relocations were processed earlier in apply_relocations()).

However, the KLP module state here must be COMING, so s390_kernel_write() should be used. What are we missing?

Joe, could you debug this a bit, please?

arch/x86/kernel/module.c +11 -0

+int apply_relocate_add(Elf64_Shdr *sechdrs,
+		   const char *strtab,
+		   unsigned int symindex,
+		   unsigned int relsec,
+		   struct module *me)
+{
+	int ret;
+	bool early = me->state == MODULE_STATE_UNFORMED;
+	void *(*write)(void *, const void *, size_t) = memcpy;
+
+	if (!early) {

arch/x86/kernel/module.c +5 -0

+	}
+
+	return ret;
+}
+
 #endif
 
 int module_finalize(const Elf_Ehdr *hdr,

Signature · 1 line

2.21.1

Quoted 17 lines · Josh Poimboeuf

(quoted diff) +17 -0

+int apply_relocate_add(Elf64_Shdr *sechdrs,
+		   const char *strtab,
+		   unsigned int symindex,
+		   unsigned int relsec,
+		   struct module *me)
+{
+	int ret;
+	bool early = me->state == MODULE_STATE_UNFORMED;
+	void *(*write)(void *, const void *, size_t) = memcpy;
+
+	if (!early) {
+		write = text_poke;
+		mutex_lock(&text_mutex);
+	}
+
+	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				   write);

Review

Quoted 4 lines

(quoted diff) +4 -0

+
+	if (!early) {
+		mutex_unlock(&text_mutex);
+		text_poke_sync();

I'm thinking text_poke_sync() wants to be inside text_mutex. Although given that nothing should be running that text, it really doesn't matter.