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@xxxxxxxxxx>
> Cc: Russell Currey <ruscur@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
-- Joe
