Re: [PATCH v3 08/21] arm64: add support for module PLTs

From: Mark Rutland
Date: Fri Jan 22 2016 - 11:55:51 EST

Next message: Vladimir Zapolskiy: "Re: [PATCHv8 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support"
Previous message: Sudip Mukherjee: "Re: [PATCH] drm/vc4: Remove broken attempt at GPU reset using genpd."
Next in thread: Ard Biesheuvel: "Re: [PATCH v3 08/21] arm64: add support for module PLTs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Ard,

This looks good.

My comments below are mostly nits, and much of the rest probably betrays
my lack of familiarity with ELF.

On Mon, Jan 11, 2016 at 02:19:01PM +0100, Ard Biesheuvel wrote:
> This adds support for emitting PLTs at module load time for relative
> branches that are out of range. This is a prerequisite for KASLR, which
> may place the kernel and the modules anywhere in the vmalloc area,
> making it more likely that branch target offsets exceed the maximum
> range of +/- 128 MB.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
> arch/arm64/Kconfig | 9 ++
> arch/arm64/Makefile | 6 +-
> arch/arm64/include/asm/module.h | 11 ++
> arch/arm64/kernel/Makefile | 1 +
> arch/arm64/kernel/module-plts.c | 137 ++++++++++++++++++++
> arch/arm64/kernel/module.c | 12 ++
> arch/arm64/kernel/module.lds | 4 +
> 7 files changed, 179 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index ffa3c549a4ba..778df20bf623 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -363,6 +363,7 @@ config ARM64_ERRATUM_843419
> bool "Cortex-A53: 843419: A load or store might access an incorrect address"
> depends on MODULES
> default y
> + select ARM64_MODULE_CMODEL_LARGE
> help
> This option builds kernel modules using the large memory model in
> order to avoid the use of the ADRP instruction, which can cause
> @@ -702,6 +703,14 @@ config ARM64_LSE_ATOMICS
>
> endmenu
>
> +config ARM64_MODULE_CMODEL_LARGE
> + bool
> +
> +config ARM64_MODULE_PLTS
> + bool
> + select ARM64_MODULE_CMODEL_LARGE
> + select HAVE_MOD_ARCH_SPECIFIC
> +
> endmenu
>
> menu "Boot options"
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index cd822d8454c0..db462980c6be 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -41,10 +41,14 @@ endif
>
> CHECKFLAGS += -D__aarch64__
>
> -ifeq ($(CONFIG_ARM64_ERRATUM_843419), y)
> +ifeq ($(CONFIG_ARM64_MODULE_CMODEL_LARGE), y)
> KBUILD_CFLAGS_MODULE += -mcmodel=large
> endif
>
> +ifeq ($(CONFIG_ARM64_MODULE_PLTS),y)
> +KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/arm64/kernel/module.lds
> +endif
> +
> # Default value
> head-y := arch/arm64/kernel/head.o
>
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index e80e232b730e..7b8cd3dc9d8e 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -20,4 +20,15 @@
>
> #define MODULE_ARCH_VERMAGIC "aarch64"
>
> +#ifdef CONFIG_ARM64_MODULE_PLTS
> +struct mod_arch_specific {
> + struct elf64_shdr *core_plt;
> + struct elf64_shdr *init_plt;
> + int core_plt_count;
> + int init_plt_count;
> +};
> +#endif
> +
> +u64 get_module_plt(struct module *mod, void *loc, u64 val);
> +
> #endif /* __ASM_MODULE_H */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 474691f8b13a..f42b0fff607f 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -30,6 +30,7 @@ arm64-obj-$(CONFIG_COMPAT) += sys32.o kuser32.o signal32.o \
> ../../arm/kernel/opcodes.o
> arm64-obj-$(CONFIG_FUNCTION_TRACER) += ftrace.o entry-ftrace.o
> arm64-obj-$(CONFIG_MODULES) += arm64ksyms.o module.o
> +arm64-obj-$(CONFIG_ARM64_MODULE_PLTS) += module-plts.o
> arm64-obj-$(CONFIG_PERF_EVENTS) += perf_regs.o perf_callchain.o
> arm64-obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o
> arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o
> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
> new file mode 100644
> index 000000000000..4a8ef9ea01ee
> --- /dev/null
> +++ b/arch/arm64/kernel/module-plts.c
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright (C) 2014-2015 Linaro Ltd. <ard.biesheuvel@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/elf.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +struct plt_entry {
> + __le32 mov0; /* movn x16, #0x.... */
> + __le32 mov1; /* movk x16, #0x...., lsl #16 */
> + __le32 mov2; /* movk x16, #0x...., lsl #32 */
> + __le32 br; /* br x16 */
> +} __aligned(8);

We only need natural alignment for the instructions, so what's the
alignment for? I can't see that anything else cares.

It might be worth a comment regarding why why use x16 (i.e. because the
AAPCS says that as IP0 it is valid for veneers/PLTs to clobber).

> +static bool in_init(const struct module *mod, void *addr)
> +{
> + return (u64)addr - (u64)mod->module_init < mod->init_size;
> +}
> +
> +u64 get_module_plt(struct module *mod, void *loc, u64 val)
> +{
> + struct plt_entry entry = {
> + cpu_to_le32(0x92800010 | (((~val ) & 0xffff)) << 5),
> + cpu_to_le32(0xf2a00010 | ((( val >> 16) & 0xffff)) << 5),
> + cpu_to_le32(0xf2c00010 | ((( val >> 32) & 0xffff)) << 5),
> + cpu_to_le32(0xd61f0200)
> + }, *plt;

It would be nice if we could un-magic this, though I see that reusing
the existing insn or reloc_insn code is painful here.

> + int i, *count;
> +
> + if (in_init(mod, loc)) {
> + plt = (struct plt_entry *)mod->arch.init_plt->sh_addr;
> + count = &mod->arch.init_plt_count;
> + } else {
> + plt = (struct plt_entry *)mod->arch.core_plt->sh_addr;
> + count = &mod->arch.core_plt_count;
> + }
> +
> + /* Look for an existing entry pointing to 'val' */
> + for (i = 0; i < *count; i++)
> + if (plt[i].mov0 == entry.mov0 &&
> + plt[i].mov1 == entry.mov1 &&
> + plt[i].mov2 == entry.mov2)
> + return (u64)&plt[i];

I think that at the cost of redundantly comparing the br x16, you could
simplify this by comparing the whole struct, e.g.

for (i = 0; i < *count; i++)
if (plt[i] == entry)
return (u64)&plt[i];

Which would also work if we change the veneer for some reason.

> +
> + i = (*count)++;

given i == *count at the end of the loop, you could just increment
*count here.

> + plt[i] = entry;
> + return (u64)&plt[i];
> +}
> +
> +static int duplicate_rel(Elf64_Addr base, const Elf64_Rela *rela, int num)

Perhaps: static bool is_duplicate_rel

> +{
> + int i;
> +
> + for (i = 0; i < num; i++) {
> + if (rela[i].r_info == rela[num].r_info &&
> + rela[i].r_addend == rela[num].r_addend)
> + return 1;
> + }
> + return 0;
> +}
> +
> +/* Count how many PLT entries we may need */
> +static unsigned int count_plts(Elf64_Addr base, const Elf64_Rela *rela, int num)
> +{
> + unsigned int ret = 0;
> + int i;
> +
> + /*
> + * Sure, this is order(n^2), but it's usually short, and not
> + * time critical
> + */
> + for (i = 0; i < num; i++)
> + switch (ELF64_R_TYPE(rela[i].r_info)) {
> + case R_AARCH64_JUMP26:
> + case R_AARCH64_CALL26:
> + if (!duplicate_rel(base, rela, i))
> + ret++;
> + break;
> + }

While braces aren't strictly required on the for loop, i think it would
look better with them given the contained logic is non-trivial.

> + return ret;
> +}
> +
> +int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> + char *secstrings, struct module *mod)
> +{
> + unsigned long core_plts = 0, init_plts = 0;
> + Elf64_Shdr *s, *sechdrs_end = sechdrs + ehdr->e_shnum;
> +
> + /*
> + * To store the PLTs, we expand the .text section for core module code
> + * and the .init.text section for initialization code.
> + */

That comment is a bit misleading, given we don't touch .text and
.init.text, but rather .core.plt and .init.plt, relying on
layout_sections to group those with .text and .init.text.

> + for (s = sechdrs; s < sechdrs_end; ++s)
> + if (strcmp(".core.plt", secstrings + s->sh_name) == 0)
> + mod->arch.core_plt = s;
> + else if (strcmp(".init.plt", secstrings + s->sh_name) == 0)
> + mod->arch.init_plt = s;

This would be nicer with braces.

> +
> + if (!mod->arch.core_plt || !mod->arch.init_plt) {
> + pr_err("%s: sections missing\n", mod->name);
> + return -ENOEXEC;
> + }
> +
> + for (s = sechdrs + 1; s < sechdrs_end; ++s) {

Could we have a comment as to why we skip the first Shdr? I recall it's
in some way special, but I can't recall why/how.

> + const Elf64_Rela *rels = (void *)ehdr + s->sh_offset;
> + int numrels = s->sh_size / sizeof(Elf64_Rela);
> + Elf64_Shdr *dstsec = sechdrs + s->sh_info;
> +
> + if (s->sh_type != SHT_RELA)
> + continue;

We only have RELA, and no REL?

Thanks,
Mark.

> +
> + if (strstr(secstrings + s->sh_name, ".init"))
> + init_plts += count_plts(dstsec->sh_addr, rels, numrels);
> + else
> + core_plts += count_plts(dstsec->sh_addr, rels, numrels);
> + }
> +
> + mod->arch.core_plt->sh_type = SHT_NOBITS;
> + mod->arch.core_plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> + mod->arch.core_plt->sh_addralign = L1_CACHE_BYTES;
> + mod->arch.core_plt->sh_size = core_plts * sizeof(struct plt_entry);
> + mod->arch.core_plt_count = 0;
> +
> + mod->arch.init_plt->sh_type = SHT_NOBITS;
> + mod->arch.init_plt->sh_flags = SHF_EXECINSTR | SHF_ALLOC;
> + mod->arch.init_plt->sh_addralign = L1_CACHE_BYTES;
> + mod->arch.init_plt->sh_size = init_plts * sizeof(struct plt_entry);
> + mod->arch.init_plt_count = 0;
> + pr_debug("%s: core.plt=%lld, init.plt=%lld\n", __func__,
> + mod->arch.core_plt->sh_size, mod->arch.init_plt->sh_size);
> + return 0;
> +}
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 93e970231ca9..3a298b0e21bb 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -38,6 +38,11 @@ void *module_alloc(unsigned long size)
> GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> NUMA_NO_NODE, __builtin_return_address(0));
>
> + if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
> + p = __vmalloc_node_range(size, MODULE_ALIGN, VMALLOC_START,
> + VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> + NUMA_NO_NODE, __builtin_return_address(0));
> +
> if (p && (kasan_module_alloc(p, size) < 0)) {
> vfree(p);
> return NULL;
> @@ -361,6 +366,13 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> case R_AARCH64_CALL26:
> ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 26,
> AARCH64_INSN_IMM_26);
> +
> + if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
> + ovf == -ERANGE) {
> + val = get_module_plt(me, loc, val);
> + ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2,
> + 26, AARCH64_INSN_IMM_26);
> + }
> break;
>
> default:
> diff --git a/arch/arm64/kernel/module.lds b/arch/arm64/kernel/module.lds
> new file mode 100644
> index 000000000000..3682fa107918
> --- /dev/null
> +++ b/arch/arm64/kernel/module.lds
> @@ -0,0 +1,4 @@
> +SECTIONS {
> + .core.plt : { BYTE(0) }
> + .init.plt : { BYTE(0) }
> +}
> --
> 2.5.0
>

Next message: Vladimir Zapolskiy: "Re: [PATCHv8 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support"
Previous message: Sudip Mukherjee: "Re: [PATCH] drm/vc4: Remove broken attempt at GPU reset using genpd."
Next in thread: Ard Biesheuvel: "Re: [PATCH v3 08/21] arm64: add support for module PLTs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]