| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20091001113608.GB2962@elte.hu>
Date: Thu, 1 Oct 2009 13:36:08 +0200
From: Ingo Molnar <mingo@...e.hu>
To: Jason Baron <jbaron@...hat.com>
Cc: linux-kernel@...r.kernel.org, mathieu.desnoyers@...ymtl.ca,
tglx@...utronix.de, rostedt@...dmis.org, ak@...e.de,
roland@...hat.com, rth@...hat.com, mhiramat@...hat.com
Subject: Re: [PATCH 2/4] jump label - base patch
* Jason Baron <jbaron@...hat.com> wrote:
> Base jump label patch which creates macros for jump patching. This
> feature is dependent on gcc version >= 4.5
>
> Signed-off-by: Jason Baron <jbaron@...hat.com>
> ---
> arch/x86/include/asm/jump_label.h | 27 +++++++++++
> arch/x86/kernel/Makefile | 2 +-
> arch/x86/kernel/jump_label.c | 92 +++++++++++++++++++++++++++++++++++++
> include/asm-generic/vmlinux.lds.h | 11 ++++
> include/linux/jump_label.h | 53 +++++++++++++++++++++
> 5 files changed, 184 insertions(+), 1 deletions(-)
> create mode 100644 arch/x86/include/asm/jump_label.h
> create mode 100644 arch/x86/kernel/jump_label.c
> create mode 100644 include/linux/jump_label.h
>
> diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
> new file mode 100644
> index 0000000..21a7b79
> --- /dev/null
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -0,0 +1,27 @@
> +#ifndef _ASM_X86_JUMP_LABEL_H
> +#define _ASM_X86_JUMP_LABEL_H
> +
> +#include <asm/nops.h>
> +
> +#if (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
> +#define __HAVE_ARCH_JUMP_LABEL
> +#endif
that should be:
> +#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5))
Also, please use:
#if ...
# define
#endif
whitespace constructs to keep them more readable.
> +
> +#ifdef __HAVE_ARCH_JUMP_LABEL
> +
> +#define JUMP_LABEL(tag, label, cond) \
(these spaces should be tabs)
> + do { \
> + static const char __jlstrtab_##tag[] \
> + __used __attribute__((section("__jump_strings"))) = #tag; \
> + asm goto ("1:" \
> + "jmp %l[" #label "] \n\t" \
> + ".pushsection __jump_table, \"a\" \n\t" \
> + _ASM_PTR "1b, %l[" #label "], %c0, 0 \n\t" \
> + ".popsection \n\t" \
> + : : "i" (__jlstrtab_##tag) : : label); \
> + } while (0)
> +
> +#endif
> +
> +#endif
> +
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 91d4189..b6b3e42 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -32,7 +32,7 @@ GCOV_PROFILE_paravirt.o := n
> obj-y := process_$(BITS).o signal.o entry_$(BITS).o
> obj-y += traps.o irq.o irq_$(BITS).o dumpstack_$(BITS).o
> obj-y += time.o ioport.o ldt.o dumpstack.o
> -obj-y += setup.o x86_init.o i8259.o irqinit.o
> +obj-y += setup.o x86_init.o i8259.o irqinit.o jump_label.o
> obj-$(CONFIG_X86_VISWS) += visws_quirks.o
> obj-$(CONFIG_X86_32) += probe_roms_32.o
> obj-$(CONFIG_X86_32) += sys_i386_32.o i386_ksyms_32.o
> diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> new file mode 100644
> index 0000000..207947d
> --- /dev/null
> +++ b/arch/x86/kernel/jump_label.c
> @@ -0,0 +1,92 @@
> +#include <linux/jump_label.h>
> +#include <linux/memory.h>
> +#include <linux/kprobes.h>
> +#include <linux/uaccess.h>
> +
> +#ifdef __HAVE_ARCH_JUMP_LABEL
> +
> +extern struct jump_entry __start___jump_table[];
> +extern struct jump_entry __stop___jump_table[];
externs should go into .h's.
> +
> +DEFINE_MUTEX(jump_label_mutex);
Shouldnt this code rely on text_mutex, which is our generic 'patch
kernel text' exclusion primitive?
> +
> +union jump_code_union {
> + char code[RELATIVEJUMP_SIZE];
> + struct {
> + char jump;
> + int offset;
> + } __attribute__((packed));
> +};
> +
> +void jump_label_transform(struct jump_entry *entry, enum jump_label_type type)
> +{
> + union jump_code_union code;
> + long jump_length;
> + unsigned char opcode;
> + size_t opcode_length;
> +
> + if (entry->type == UNKOWN_JUMP) {
> + if (probe_kernel_read(&opcode, (void *)entry->code, 1))
> + BUG();
a BUG() is not nice. It results fewer decodable bug reports than a
WARN_ON_ONCE().
> +
> + if (opcode == 0xe9)
> + entry->type = LONG_JUMP;
> + else if (opcode == 0xeb)
> + entry->type = SHORT_JUMP;
> + else
> + BUG();
ditto.
> + }
> +
> + if (entry->type == LONG_JUMP)
> + opcode_length = 5;
> + else
> + opcode_length = 2;
> +
> + if (type == JUMP_LABEL_ENABLE) {
> + if (entry->type == SHORT_JUMP) {
> + code.code[0] = 0xeb;
> + code.code[1] = 0;
> + } else {
> + code.code[0] = 0xe9;
> + code.offset = 0;
> + }
> + } else {
> + jump_length = entry->target - (entry->code + opcode_length);
> + if (entry->type == SHORT_JUMP) {
> + if ((jump_length > 127 || jump_length < -128)) {
> + BUG();
> + }
WARN_ON_ONCE() again.
> + code.code[0] = 0xeb;
> + code.code[1] = (char)jump_length;
> + } else {
> + if ((jump_length > INT_MAX || jump_length < INT_MIN)) {
> + BUG();
> + }
ditto.
> + code.code[0] = 0xe9;
> + code.offset = jump_length;
> + }
> + }
> +
> + mutex_lock(&text_mutex);
> + text_poke_fixup((void *)entry->code, &code,
> + opcode_length, (void *)entry->code + opcode_length);
> + mutex_unlock(&text_mutex);
Note that here we take the text_mutex anyway. Can drop jump_label_mutex
and take the text_mutex wider?
> +}
> +
> +int jump_label_update(const char *name, enum jump_label_type type)
> +{
> + struct jump_entry *iter;
> +
> + mutex_lock(&jump_label_mutex);
> + for (iter = __start___jump_table; iter < __stop___jump_table; iter++) {
how big can this table become, realistically, long term?
> + if (!strcmp(name, iter->name)) {
> + if (!(system_state == SYSTEM_RUNNING &&
> + (init_kernel_text(iter->code))))
> + jump_label_transform(iter, type);
> + }
> + }
> + mutex_unlock(&jump_label_mutex);
> + return 0;
> +}
We always seem to return 0 - should be void?
> +
> +#endif
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index e7eae37..d2f38e2 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -213,6 +213,7 @@
> *(__vermagic) /* Kernel version magic */ \
> *(__markers_strings) /* Markers: strings */ \
> *(__tracepoints_strings)/* Tracepoints: strings */ \
> + *(__jump_strings)/* Jump: strings */ \
> } \
inserted line does not follow existing style carefully enough.
> \
> .rodata1 : AT(ADDR(.rodata1) - LOAD_OFFSET) { \
> @@ -220,6 +221,8 @@
> } \
> \
> BUG_TABLE \
> + JUMP_TABLE \
> + \
> \
> /* PCI quirks */ \
> .pci_fixup : AT(ADDR(.pci_fixup) - LOAD_OFFSET) { \
> @@ -564,6 +567,14 @@
> #define BUG_TABLE
> #endif
>
> +#define JUMP_TABLE \
> + . = ALIGN(8); \
> + __jump_table : AT(ADDR(__jump_table) - LOAD_OFFSET) { \
> + VMLINUX_SYMBOL(__start___jump_table) = .; \
> + *(__jump_table) \
> + VMLINUX_SYMBOL(__stop___jump_table) = .; \
> + }
ditto.
> +
> #ifdef CONFIG_PM_TRACE
> #define TRACEDATA \
> . = ALIGN(4); \
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> new file mode 100644
> index 0000000..eb0a4ef
> --- /dev/null
> +++ b/include/linux/jump_label.h
> @@ -0,0 +1,53 @@
> +#ifndef _LINUX_JUMP_LABEL_H
> +#define _LINUX_JUMP_LABEL_H
> +
> +#include <asm/jump_label.h>
> +
> +struct jump_entry {
> + unsigned long code;
> + unsigned long target;
> + char *name;
> + unsigned long type;
> +};
please use new structure definition alignments like in
include/linux/perf_event.h.
> +
> +enum jump_label_type {
> + JUMP_LABEL_ENABLE,
> + JUMP_LABEL_DISABLE
> +};
so 0 is enable and 1 is disable?
> +
> +enum jump_label_instruction {
> + UNKOWN_JUMP,
> + SHORT_JUMP,
> + LONG_JUMP
> +};
> +
> +#ifdef __HAVE_ARCH_JUMP_LABEL
> +
> +extern int jump_label_update(const char *name, enum jump_label_type type);
> +
> +#define enable_jump_label(name) \
> + jump_label_update(name, JUMP_LABEL_ENABLE);
> +
> +#define disable_jump_label(name) \
> + jump_label_update(name, JUMP_LABEL_DISABLE);
> +
> +#else
> +
> +#define JUMP_LABEL(tag, label, cond) \
> + if (likely(!cond)) \
> + goto label;
> +
> +static inline int enable_jump_label(const char *name, enum jump_label_type type)
> +{
> + return 0;
> +}
> +
> +static inline int disable_jump_label(const char *name,
> + enum jump_label_type type)
just keep the prototype in a single line. (and ignore checkpatch for
that one)
> +{
> + return 0;
> +}
> +
> +#endif
> +
> +#endif
> --
> 1.6.2.5
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists