[PATCH 4/7] s390/module: Use s390_kernel_write() for relocations

[PATCH 4/7] s390/module: Use s390_kernel_write() for relocations

[Josh Poimboeuf]

From: Peter Zijlstra <peterz@infradead.org>

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.

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

Cc: linux-s390@vger.kernel.org
Cc: heiko.carstens@de.ibm.com
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/s390/kernel/module.c | 106 ++++++++++++++++++++++----------------
 1 file changed, 61 insertions(+), 45 deletions(-)

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index ba8f19bb438b..e85e378f876e 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -174,7 +174,8 @@ int module_frob_arch_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
 }
 
 static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
-			   int sign, int bits, int shift)
+			   int sign, int bits, int shift,
+			   void (*write)(void *dest, const void *src, size_t len))
 {
 	unsigned long umax;
 	long min, max;
@@ -194,26 +195,29 @@ static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
 			return -ENOEXEC;
 	}
 
-	if (bits == 8)
-		*(unsigned char *) loc = val;
-	else if (bits == 12)
-		*(unsigned short *) loc = (val & 0xfff) |
+	if (bits == 8) {
+		write(loc, &val, 1);
+	} else if (bits == 12) {
+		unsigned short tmp = (val & 0xfff) |
 			(*(unsigned short *) loc & 0xf000);
-	else if (bits == 16)
-		*(unsigned short *) loc = val;
-	else if (bits == 20)
-		*(unsigned int *) loc = (val & 0xfff) << 16 |
-			(val & 0xff000) >> 4 |
-			(*(unsigned int *) loc & 0xf00000ff);
-	else if (bits == 32)
-		*(unsigned int *) loc = val;
-	else if (bits == 64)
-		*(unsigned long *) loc = val;
+		write(loc, &tmp, 2);
+	} else if (bits == 16) {
+		write(loc, &val, 2);
+	} else if (bits == 20) {
+		unsigned int tmp = (val & 0xfff) << 16 |
+			(val & 0xff000) >> 4 | (*(unsigned int *) loc & 0xf00000ff);
+		write(loc, &tmp, 4);
+	} else if (bits == 32) {
+		write(loc, &val, 4);
+	} else if (bits == 64) {
+		write(loc, &val, 8);
+	}
 	return 0;
 }
 
 static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
-		      const char *strtab, struct module *me)
+		      const char *strtab, struct module *me,
+		      void (*write)(void *dest, const void *src, size_t len))
 {
 	struct mod_arch_syminfo *info;
 	Elf_Addr loc, val;
@@ -241,17 +245,17 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	case R_390_64:		/* Direct 64 bit.  */
 		val += rela->r_addend;
 		if (r_type == R_390_8)
-			rc = apply_rela_bits(loc, val, 0, 8, 0);
+			rc = apply_rela_bits(loc, val, 0, 8, 0, write);
 		else if (r_type == R_390_12)
-			rc = apply_rela_bits(loc, val, 0, 12, 0);
+			rc = apply_rela_bits(loc, val, 0, 12, 0, write);
 		else if (r_type == R_390_16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_20)
-			rc = apply_rela_bits(loc, val, 1, 20, 0);
+			rc = apply_rela_bits(loc, val, 1, 20, 0, write);
 		else if (r_type == R_390_32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		break;
 	case R_390_PC16:	/* PC relative 16 bit.  */
 	case R_390_PC16DBL:	/* PC relative 16 bit shifted by 1.  */
@@ -260,15 +264,15 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	case R_390_PC64:	/* PC relative 64 bit.	*/
 		val += rela->r_addend - loc;
 		if (r_type == R_390_PC16)
-			rc = apply_rela_bits(loc, val, 1, 16, 0);
+			rc = apply_rela_bits(loc, val, 1, 16, 0, write);
 		else if (r_type == R_390_PC16DBL)
-			rc = apply_rela_bits(loc, val, 1, 16, 1);
+			rc = apply_rela_bits(loc, val, 1, 16, 1, write);
 		else if (r_type == R_390_PC32DBL)
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		else if (r_type == R_390_PC32)
-			rc = apply_rela_bits(loc, val, 1, 32, 0);
+			rc = apply_rela_bits(loc, val, 1, 32, 0, write);
 		else if (r_type == R_390_PC64)
-			rc = apply_rela_bits(loc, val, 1, 64, 0);
+			rc = apply_rela_bits(loc, val, 1, 64, 0, write);
 		break;
 	case R_390_GOT12:	/* 12 bit GOT offset.  */
 	case R_390_GOT16:	/* 16 bit GOT offset.  */
@@ -293,23 +297,23 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 		val = info->got_offset + rela->r_addend;
 		if (r_type == R_390_GOT12 ||
 		    r_type == R_390_GOTPLT12)
-			rc = apply_rela_bits(loc, val, 0, 12, 0);
+			rc = apply_rela_bits(loc, val, 0, 12, 0, write);
 		else if (r_type == R_390_GOT16 ||
 			 r_type == R_390_GOTPLT16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_GOT20 ||
 			 r_type == R_390_GOTPLT20)
-			rc = apply_rela_bits(loc, val, 1, 20, 0);
+			rc = apply_rela_bits(loc, val, 1, 20, 0, write);
 		else if (r_type == R_390_GOT32 ||
 			 r_type == R_390_GOTPLT32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_GOT64 ||
 			 r_type == R_390_GOTPLT64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		else if (r_type == R_390_GOTENT ||
 			 r_type == R_390_GOTPLTENT) {
 			val += (Elf_Addr) me->core_layout.base - loc;
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		}
 		break;
 	case R_390_PLT16DBL:	/* 16 bit PC rel. PLT shifted by 1.  */
@@ -357,17 +361,17 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 			val += rela->r_addend - loc;
 		}
 		if (r_type == R_390_PLT16DBL)
-			rc = apply_rela_bits(loc, val, 1, 16, 1);
+			rc = apply_rela_bits(loc, val, 1, 16, 1, write);
 		else if (r_type == R_390_PLTOFF16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_PLT32DBL)
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		else if (r_type == R_390_PLT32 ||
 			 r_type == R_390_PLTOFF32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_PLT64 ||
 			 r_type == R_390_PLTOFF64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		break;
 	case R_390_GOTOFF16:	/* 16 bit offset to GOT.  */
 	case R_390_GOTOFF32:	/* 32 bit offset to GOT.  */
@@ -375,20 +379,20 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 		val = val + rela->r_addend -
 			((Elf_Addr) me->core_layout.base + me->arch.got_offset);
 		if (r_type == R_390_GOTOFF16)
-			rc = apply_rela_bits(loc, val, 0, 16, 0);
+			rc = apply_rela_bits(loc, val, 0, 16, 0, write);
 		else if (r_type == R_390_GOTOFF32)
-			rc = apply_rela_bits(loc, val, 0, 32, 0);
+			rc = apply_rela_bits(loc, val, 0, 32, 0, write);
 		else if (r_type == R_390_GOTOFF64)
-			rc = apply_rela_bits(loc, val, 0, 64, 0);
+			rc = apply_rela_bits(loc, val, 0, 64, 0, write);
 		break;
 	case R_390_GOTPC:	/* 32 bit PC relative offset to GOT. */
 	case R_390_GOTPCDBL:	/* 32 bit PC rel. off. to GOT shifted by 1. */
 		val = (Elf_Addr) me->core_layout.base + me->arch.got_offset +
 			rela->r_addend - loc;
 		if (r_type == R_390_GOTPC)
-			rc = apply_rela_bits(loc, val, 1, 32, 0);
+			rc = apply_rela_bits(loc, val, 1, 32, 0, write);
 		else if (r_type == R_390_GOTPCDBL)
-			rc = apply_rela_bits(loc, val, 1, 32, 1);
+			rc = apply_rela_bits(loc, val, 1, 32, 1, write);
 		break;
 	case R_390_COPY:
 	case R_390_GLOB_DAT:	/* Create GOT entry.  */
@@ -412,9 +416,10 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 	return 0;
 }
 
-int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
+static int __apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 		       unsigned int symindex, unsigned int relsec,
-		       struct module *me)
+		       struct module *me,
+		       void (*write)(void *dest, const void *src, size_t len))
 {
 	Elf_Addr base;
 	Elf_Sym *symtab;
@@ -437,6 +442,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	return 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;
+
+	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				    early ? memcpy : s390_kernel_write);
+}
+
 int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
-- 
2.21.1

Re: [PATCH 4/7] s390/module: Use s390_kernel_write() for relocations

[Miroslav Benes]

On Tue, 14 Apr 2020, Josh Poimboeuf wrote:

> From: Peter Zijlstra <peterz@infradead.org>
> 
> 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.
> 
> [ jpoimboe: Split up patches. Use mod state to determine whether
> 	    memcpy() can be used. ]
> 
> Cc: linux-s390@vger.kernel.org
> Cc: heiko.carstens@de.ibm.com
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/s390/kernel/module.c | 106 ++++++++++++++++++++++----------------
>  1 file changed, 61 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> index ba8f19bb438b..e85e378f876e 100644
> --- a/arch/s390/kernel/module.c
> +++ b/arch/s390/kernel/module.c
> @@ -174,7 +174,8 @@ int module_frob_arch_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
>  }
>  
>  static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
> -			   int sign, int bits, int shift)
> +			   int sign, int bits, int shift,
> +			   void (*write)(void *dest, const void *src, size_t len))
>  {
>  	unsigned long umax;
>  	long min, max;
> @@ -194,26 +195,29 @@ static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
>  			return -ENOEXEC;
>  	}
>  
> -	if (bits == 8)
> -		*(unsigned char *) loc = val;
> -	else if (bits == 12)
> -		*(unsigned short *) loc = (val & 0xfff) |
> +	if (bits == 8) {
> +		write(loc, &val, 1);
> +	} else if (bits == 12) {
> +		unsigned short tmp = (val & 0xfff) |
>  			(*(unsigned short *) loc & 0xf000);
> -	else if (bits == 16)
> -		*(unsigned short *) loc = val;
> -	else if (bits == 20)
> -		*(unsigned int *) loc = (val & 0xfff) << 16 |
> -			(val & 0xff000) >> 4 |
> -			(*(unsigned int *) loc & 0xf00000ff);
> -	else if (bits == 32)
> -		*(unsigned int *) loc = val;
> -	else if (bits == 64)
> -		*(unsigned long *) loc = val;
> +		write(loc, &tmp, 2);
> +	} else if (bits == 16) {
> +		write(loc, &val, 2);
> +	} else if (bits == 20) {
> +		unsigned int tmp = (val & 0xfff) << 16 |
> +			(val & 0xff000) >> 4 | (*(unsigned int *) loc & 0xf00000ff);
> +		write(loc, &tmp, 4);
> +	} else if (bits == 32) {
> +		write(loc, &val, 4);
> +	} else if (bits == 64) {
> +		write(loc, &val, 8);
> +	}
>  	return 0;
>  }

The compiler complains about the above changes

arch/s390/kernel/module.c:199:9: warning: passing argument 1 of 'write' makes pointer from integer without a cast [-Wint-conversion]
   write(loc, &val, 1);
         ^~~
arch/s390/kernel/module.c:199:9: note: expected 'void *' but argument is of type 'Elf64_Addr' {aka 'long long unsigned int'}

[...]  

> -int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> +static int __apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  		       unsigned int symindex, unsigned int relsec,
> -		       struct module *me)
> +		       struct module *me,
> +		       void (*write)(void *dest, const void *src, size_t len))
>  {
>  	Elf_Addr base;
>  	Elf_Sym *symtab;

You also need to update apply_rela() call site in this function. It is 
missing write argument.

> @@ -437,6 +442,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
>  	return 0;
>  }
>  
> +int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
> +		       unsigned int symindex, unsigned int relsec,
> +		       struct module *me)
> +{
> +	int ret;

ret is unused;

> +	bool early = me->state == MODULE_STATE_UNFORMED;
> +
> +	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> +				    early ? memcpy : s390_kernel_write);

The compiler warns about

arch/s390/kernel/module.c: In function 'apply_relocate_add':
arch/s390/kernel/module.c:453:24: warning: pointer type mismatch in conditional expression
         early ? memcpy : s390_kernel_write);

Miroslav

Re: [PATCH 4/7] s390/module: Use s390_kernel_write() for relocations

[Josh Poimboeuf]

On Thu, Apr 16, 2020 at 10:56:02AM +0200, Miroslav Benes wrote:
> > +	bool early = me->state == MODULE_STATE_UNFORMED;
> > +
> > +	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> > +				    early ? memcpy : s390_kernel_write);
> 
> The compiler warns about
> 
> arch/s390/kernel/module.c: In function 'apply_relocate_add':
> arch/s390/kernel/module.c:453:24: warning: pointer type mismatch in conditional expression
>          early ? memcpy : s390_kernel_write);

Thanks, I'll get all that cleaned up.

I could have sworn I got a SUCCESS message from the kbuild bot.  Does it
ignore warnings nowadays?

-- 
Josh

Re: [PATCH 4/7] s390/module: Use s390_kernel_write() for relocations

[Josh Poimboeuf]

On Thu, Apr 16, 2020 at 07:06:51AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 16, 2020 at 10:56:02AM +0200, Miroslav Benes wrote:
> > > +	bool early = me->state == MODULE_STATE_UNFORMED;
> > > +
> > > +	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> > > +				    early ? memcpy : s390_kernel_write);
> > 
> > The compiler warns about
> > 
> > arch/s390/kernel/module.c: In function 'apply_relocate_add':
> > arch/s390/kernel/module.c:453:24: warning: pointer type mismatch in conditional expression
> >          early ? memcpy : s390_kernel_write);
> 
> Thanks, I'll get all that cleaned up.
> 
> I could have sworn I got a SUCCESS message from the kbuild bot.  Does it
> ignore warnings nowadays?

Here's a fix on top of the original patch.

I changed s390_kernel_write() to return "void *" to match memcpy()
(probably a separate patch).

I also grabbed the text_mutex for the !early case in
apply_relocate_add() -- will do something similar for x86.

Will try to test this on a 390 box.


diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index a470f1fa9f2a..324438889fe1 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -276,6 +276,6 @@ static inline unsigned long __must_check clear_user(void __user *to, unsigned lo
 }
 
 int copy_to_user_real(void __user *dest, void *src, unsigned long count);
-void s390_kernel_write(void *dst, const void *src, size_t size);
+void *s390_kernel_write(void *dst, const void *src, size_t size);
 
 #endif /* __S390_UACCESS_H */
diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index e85e378f876e..2b30ed0ce14f 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -19,6 +19,7 @@
 #include <linux/kasan.h>
 #include <linux/moduleloader.h>
 #include <linux/bug.h>
+#include <linux/memory.h>
 #include <asm/alternative.h>
 #include <asm/nospec-branch.h>
 #include <asm/facility.h>
@@ -175,10 +176,11 @@ int module_frob_arch_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
 
 static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
 			   int sign, int bits, int shift,
-			   void (*write)(void *dest, const void *src, size_t len))
+			   void *(*write)(void *dest, const void *src, size_t len))
 {
 	unsigned long umax;
 	long min, max;
+	void *dest = (void *)loc;
 
 	if (val & ((1UL << shift) - 1))
 		return -ENOEXEC;
@@ -196,28 +198,28 @@ static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
 	}
 
 	if (bits == 8) {
-		write(loc, &val, 1);
+		write(dest, &val, 1);
 	} else if (bits == 12) {
 		unsigned short tmp = (val & 0xfff) |
 			(*(unsigned short *) loc & 0xf000);
-		write(loc, &tmp, 2);
+		write(dest, &tmp, 2);
 	} else if (bits == 16) {
-		write(loc, &val, 2);
+		write(dest, &val, 2);
 	} else if (bits == 20) {
 		unsigned int tmp = (val & 0xfff) << 16 |
 			(val & 0xff000) >> 4 | (*(unsigned int *) loc & 0xf00000ff);
-		write(loc, &tmp, 4);
+		write(dest, &tmp, 4);
 	} else if (bits == 32) {
-		write(loc, &val, 4);
+		write(dest, &val, 4);
 	} else if (bits == 64) {
-		write(loc, &val, 8);
+		write(dest, &val, 8);
 	}
 	return 0;
 }
 
 static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 		      const char *strtab, struct module *me,
-		      void (*write)(void *dest, const void *src, size_t len))
+		      void *(*write)(void *dest, const void *src, size_t len))
 {
 	struct mod_arch_syminfo *info;
 	Elf_Addr loc, val;
@@ -419,7 +421,7 @@ static int apply_rela(Elf_Rela *rela, Elf_Addr base, Elf_Sym *symtab,
 static int __apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 		       unsigned int symindex, unsigned int relsec,
 		       struct module *me,
-		       void (*write)(void *dest, const void *src, size_t len))
+		       void *(*write)(void *dest, const void *src, size_t len))
 {
 	Elf_Addr base;
 	Elf_Sym *symtab;
@@ -435,7 +437,7 @@ static int __apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	n = sechdrs[relsec].sh_size / sizeof(Elf_Rela);
 
 	for (i = 0; i < n; i++, rela++) {
-		rc = apply_rela(rela, base, symtab, strtab, me);
+		rc = apply_rela(rela, base, symtab, strtab, me, write);
 		if (rc)
 			return rc;
 	}
@@ -449,8 +451,16 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	int ret;
 	bool early = me->state == MODULE_STATE_UNFORMED;
 
-	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
-				    early ? memcpy : s390_kernel_write);
+	if (!early)
+		mutex_lock(&text_mutex);
+
+	ret = __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
+				   early ? memcpy : s390_kernel_write);
+
+	if (!early)
+		mutex_unlock(&text_mutex);
+
+	return ret;
 }
 
 int module_finalize(const Elf_Ehdr *hdr,
diff --git a/arch/s390/mm/maccess.c b/arch/s390/mm/maccess.c
index de7ca4b6718f..22a0be655f27 100644
--- a/arch/s390/mm/maccess.c
+++ b/arch/s390/mm/maccess.c
@@ -55,19 +55,22 @@ static notrace long s390_kernel_write_odd(void *dst, const void *src, size_t siz
  */
 static DEFINE_SPINLOCK(s390_kernel_write_lock);
 
-void notrace s390_kernel_write(void *dst, const void *src, size_t size)
+notrace void *s390_kernel_write(void *dst, const void *src, size_t size)
 {
+	void *tmp = dst;
 	unsigned long flags;
 	long copied;
 
 	spin_lock_irqsave(&s390_kernel_write_lock, flags);
 	while (size) {
-		copied = s390_kernel_write_odd(dst, src, size);
-		dst += copied;
+		copied = s390_kernel_write_odd(tmp, src, size);
+		tmp += copied;
 		src += copied;
 		size -= copied;
 	}
 	spin_unlock_irqrestore(&s390_kernel_write_lock, flags);
+
+	return dst;
 }
 
 static int __no_sanitize_address __memcpy_real(void *dest, void *src, size_t count)

Re: [PATCH 4/7] s390/module: Use s390_kernel_write() for relocations

[Josh Poimboeuf]

On Thu, Apr 16, 2020 at 08:16:35AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 16, 2020 at 07:06:51AM -0500, Josh Poimboeuf wrote:
> > On Thu, Apr 16, 2020 at 10:56:02AM +0200, Miroslav Benes wrote:
> > > > +	bool early = me->state == MODULE_STATE_UNFORMED;
> > > > +
> > > > +	return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
> > > > +				    early ? memcpy : s390_kernel_write);
> > > 
> > > The compiler warns about
> > > 
> > > arch/s390/kernel/module.c: In function 'apply_relocate_add':
> > > arch/s390/kernel/module.c:453:24: warning: pointer type mismatch in conditional expression
> > >          early ? memcpy : s390_kernel_write);
> > 
> > Thanks, I'll get all that cleaned up.
> > 
> > I could have sworn I got a SUCCESS message from the kbuild bot.  Does it
> > ignore warnings nowadays?
> 
> Here's a fix on top of the original patch.
> 
> I changed s390_kernel_write() to return "void *" to match memcpy()
> (probably a separate patch).
> 
> I also grabbed the text_mutex for the !early case in
> apply_relocate_add() -- will do something similar for x86.
> 
> Will try to test this on a 390 box.

...and that borked the box pretty nicely.  Oops, big endian!  Need
something like this on top.

Sorry about not testing the patch in the first place, it looked trivial
and somehow I was thinking Peter writes exclusively bug-free code.

diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index ee0904a23e24..513e640430ae 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -198,21 +198,25 @@ static int apply_rela_bits(Elf_Addr loc, Elf_Addr val,
 	}
 
 	if (bits == 8) {
-		write(dest, &val, 1);
+		unsigned char tmp = val;
+		write(dest, &tmp, 1);
 	} else if (bits == 12) {
 		unsigned short tmp = (val & 0xfff) |
 			(*(unsigned short *) loc & 0xf000);
 		write(dest, &tmp, 2);
 	} else if (bits == 16) {
-		write(dest, &val, 2);
+		unsigned short tmp = val;
+		write(dest, &tmp, 2);
 	} else if (bits == 20) {
 		unsigned int tmp = (val & 0xfff) << 16 |
 			(val & 0xff000) >> 4 | (*(unsigned int *) loc & 0xf00000ff);
 		write(dest, &tmp, 4);
 	} else if (bits == 32) {
-		write(dest, &val, 4);
+		unsigned int tmp = val;
+		write(dest, &tmp, 4);
 	} else if (bits == 64) {
-		write(dest, &val, 8);
+		unsigned long tmp = val;
+		write(dest, &tmp, 8);
 	}
 	return 0;
 }

-- 
Josh