Re: [PATCH v2 03/13] powerpc/module_32: Fix livepatching for RO modules

[Joe Lawrence <joe.lawrence@redhat.com>]

On Mon, Dec 20, 2021 at 04:38:09PM +0000, Christophe Leroy wrote:
> Livepatching a loaded module involves applying relocations through
> apply_relocate_add(), which attempts to write to read-only memory when
> CONFIG_STRICT_MODULE_RWX=y.
> 
> R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
> the types generated by the kpatch-build userspace tool or klp-convert
> kernel tree observed applying a relocation to a post-init module.
> 
> Use patch_instruction() to patch those relocations.
> 
> Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
> RO modules") did similar change in module_64.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Russell Currey <ruscur@russell.cc>
> ---
>  arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
> index a491ad481d85..a0432ef46967 100644
> --- a/arch/powerpc/kernel/module_32.c
> +++ b/arch/powerpc/kernel/module_32.c
> @@ -18,6 +18,7 @@
>  #include <linux/bug.h>
>  #include <linux/sort.h>
>  #include <asm/setup.h>
> +#include <asm/code-patching.h>
>  
>  /* Count how many different relocations (different symbol, different
>     addend) */
> @@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,
>  		entry++;
>  	}
>  
> -	entry->jump[0] = PPC_RAW_LIS(_R12, PPC_HA(val));
> -	entry->jump[1] = PPC_RAW_ADDI(_R12, _R12, PPC_LO(val));
> -	entry->jump[2] = PPC_RAW_MTCTR(_R12);
> -	entry->jump[3] = PPC_RAW_BCTR();
> +	if (patch_instruction(&entry->jump[0], ppc_inst(PPC_RAW_LIS(_R12, PPC_HA(val)))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[1], ppc_inst(PPC_RAW_ADDI(_R12, _R12, PPC_LO(val)))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[2], ppc_inst(PPC_RAW_MTCTR(_R12))))
> +		return 0;
> +	if (patch_instruction(&entry->jump[3], ppc_inst(PPC_RAW_BCTR())))
> +		return 0;
>  
>  	pr_debug("Initialized plt for 0x%x at %p\n", val, entry);
>  	return (uint32_t)entry;
>  }
>  
> +static int patch_location_16(uint32_t *loc, u16 value)
> +{
> +	loc = PTR_ALIGN_DOWN(loc, sizeof(u32));
> +	return patch_instruction(loc, ppc_inst((*loc & 0xffff0000) | value));
> +}
> +
>  int apply_relocate_add(Elf32_Shdr *sechdrs,
>  		       const char *strtab,
>  		       unsigned int symindex,
> @@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
>  
>  		case R_PPC_ADDR16_LO:
>  			/* Low half of the symbol */
> -			*(uint16_t *)location = value;
> +			if (patch_location_16(location, PPC_LO(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_ADDR16_HI:
>  			/* Higher half of the symbol */
> -			*(uint16_t *)location = (value >> 16);
> +			if (patch_location_16(location, PPC_HI(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_ADDR16_HA:
> -			/* Sign-adjusted lower 16 bits: PPC ELF ABI says:
> -			   (((x >> 16) + ((x & 0x8000) ? 1 : 0))) & 0xFFFF.
> -			   This is the same, only sane.
> -			 */
> -			*(uint16_t *)location = (value + 0x8000) >> 16;
> +			if (patch_location_16(location, PPC_HA(value)))
> +				return -EFAULT;
>  			break;
>  
>  		case R_PPC_REL24:
>  			if ((int)(value - (uint32_t)location) < -0x02000000
> -			    || (int)(value - (uint32_t)location) >= 0x02000000)
> +			    || (int)(value - (uint32_t)location) >= 0x02000000) {
>  				value = do_plt_call(location, value,
>  						    sechdrs, module);
> +				if (!value)
> +					return -EFAULT;
> +			}
>  
>  			/* Only replace bits 2 through 26 */
>  			pr_debug("REL24 value = %08X. location = %08X\n",
>  			       value, (uint32_t)location);
>  			pr_debug("Location before: %08X.\n",
>  			       *(uint32_t *)location);
> -			*(uint32_t *)location
> -				= (*(uint32_t *)location & ~0x03fffffc)
> +			value = (*(uint32_t *)location & ~0x03fffffc)
>  				| ((value - (uint32_t)location)
>  				   & 0x03fffffc);
> +
> +			if (patch_instruction(location, ppc_inst(value)))
> +				return -EFAULT;
> +
>  			pr_debug("Location after: %08X.\n",
>  			       *(uint32_t *)location);
>  			pr_debug("ie. jump to %08X+%08X = %08X\n",
> -- 
> 2.33.1
> 

IIRC, offlist we hacked up klp-convert to create the klp-relocations for
a 32-bit target and then you hit the selftest late relocation crash, so
I assume that part is happy after this fix. :)  Thanks again for the
testing.

For the livepatching implications,

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe