The previous version of this patch set was named "Compile-time stack
frame pointer validation". I changed the subject from "frame pointer
validation" to "asm code validation" because the focus of the patch set
has changed to be less frame pointer-focused and more asm-focused. I
also renamed the tool to asmvalidate (it was previously called
stackvalidate) and basically rewrote most of the code.
The goal of asm validation is to enforce sane rules on asm code: all
callable asm functions must be self-contained and properly annotated.
Some of the benefits are:
- Frame pointers are more reliable.
- DWARF CFI metadata can be autogenerated (coming soon).
- The asm code becomes less like spaghetti, more like C, and easier to
comprehend.
The asmvalidate tool runs on every compiled .S file, and enforces the
following rules:
1. Each callable function must be annotated with the ELF STT_FUNC type.
This is typically done using the existing ENTRY/ENDPROC macros. If
asmvalidate finds a return instruction outside of a function, it
flags an error, since that usually indicates callable code which
should be annotated accordingly.
2. Each callable function must never leave its own bounds (i.e. with a
jump to outside the function) except when returning.
3. Each callable non-leaf function must have frame pointer logic (if
required by CONFIG_FRAME_POINTER or the architecture's back chain
rules). This should by done by the FP_SAVE/FP_RESTORE macros.
It currently only supports x86_64, but the code is generic and designed
for it to be easy to plug in support for other architectures.
There are still a lot of outstanding warnings (which I'll paste as a
reply to this email). Once those are all cleaned up, we can change the
warnings to build errors and change the default to
CONFIG_ASM_VALIDATION=y so the asm code stays clean.
The first patch adds some frame pointer macros. The second patch adds
asmvalidate support. The rest of the patches have fixes for (some of)
the reported warnings.
These patches are based on tip/master.
[1] http://lkml.kernel.org/r/[email protected]
v5:
- stackvalidate -> asmvalidate
- frame pointers only required for non-leaf functions
- check for the use of the FP_SAVE/RESTORE macros instead of manually
analyzing code to detect frame pointer usage
- additional checks to ensure each function doesn't leave its boundaries
- make the macros simpler and more flexible
- support for analyzing ALTERNATIVE macros
- simplified the arch interfaces in scripts/asmvalidate/arch.h
- fixed some asmvalidate warnings
- rebased onto latest tip asm cleanups
- many more small changes
v4:
- Changed the default to CONFIG_STACK_VALIDATION=n, until all the asm
code can get cleaned up.
- Fixed a stackvalidate error path exit code issue found by Michal
Marek.
v3:
- Added a patch to make the push/pop CFI macros arch-independent, as
suggested by H. Peter Anvin
v2:
- Fixed memory leaks reported by Petr Mladek
Josh Poimboeuf (10):
x86/asm: Add FP_SAVE/RESTORE frame pointer macros
x86: Compile-time asm code validation
x86/asm/entry: Fix asmvalidate warnings for entry_64_compat.S
x86/asm/crypto: Fix asmvalidate warnings for aesni-intel_asm.S
x86/asm/crypto: Fix asmvalidate warnings for ghash-clmulni-intel_asm.S
x86/asm/efi: Fix asmvalidate warnings for efi_stub_64.S
x86/asm/acpi: Fix asmvalidate warnings for wakeup_64.S
x86/asm/head: Fix asmvalidate warnings for head_64.S
x86/asm/lib: Fix asmvalidate warnings for lib functions
x86/asm/lib: Fix asmvalidate warnings for rwsem.S
MAINTAINERS | 6 +
arch/Kconfig | 3 +
arch/x86/Kconfig | 1 +
arch/x86/Makefile | 6 +-
arch/x86/crypto/aesni-intel_asm.S | 19 ++
arch/x86/crypto/ghash-clmulni-intel_asm.S | 5 +
arch/x86/entry/entry_64_compat.S | 35 +--
arch/x86/include/asm/func.h | 62 ++++++
arch/x86/kernel/acpi/wakeup_64.S | 13 +-
arch/x86/kernel/head_64.S | 7 +-
arch/x86/lib/clear_page_64.S | 9 +-
arch/x86/lib/copy_page_64.S | 5 +-
arch/x86/lib/memcpy_64.S | 10 +-
arch/x86/lib/memset_64.S | 10 +-
arch/x86/lib/rwsem.S | 11 +-
arch/x86/platform/efi/efi_stub_64.S | 3 +
lib/Kconfig.debug | 21 ++
scripts/Makefile | 1 +
scripts/Makefile.build | 23 +-
scripts/asmvalidate/Makefile | 17 ++
scripts/asmvalidate/arch-x86.c | 283 ++++++++++++++++++++++++
scripts/asmvalidate/arch.h | 40 ++++
scripts/asmvalidate/asmvalidate.c | 324 +++++++++++++++++++++++++++
scripts/asmvalidate/elf.c | 354 ++++++++++++++++++++++++++++++
scripts/asmvalidate/elf.h | 74 +++++++
scripts/asmvalidate/list.h | 217 ++++++++++++++++++
26 files changed, 1509 insertions(+), 50 deletions(-)
create mode 100644 arch/x86/include/asm/func.h
create mode 100644 scripts/asmvalidate/Makefile
create mode 100644 scripts/asmvalidate/arch-x86.c
create mode 100644 scripts/asmvalidate/arch.h
create mode 100644 scripts/asmvalidate/asmvalidate.c
create mode 100644 scripts/asmvalidate/elf.c
create mode 100644 scripts/asmvalidate/elf.h
create mode 100644 scripts/asmvalidate/list.h
--
2.1.0
Add the FP_SAVE and FP_RESTORE asm macros, which can be used to save and
restore the frame pointer.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/include/asm/func.h | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 arch/x86/include/asm/func.h
diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
new file mode 100644
index 0000000..4d62782
--- /dev/null
+++ b/arch/x86/include/asm/func.h
@@ -0,0 +1,24 @@
+#ifndef _ASM_X86_FUNC_H
+#define _ASM_X86_FUNC_H
+
+#include <linux/linkage.h>
+#include <asm/asm.h>
+
+/*
+ * These are frame pointer save and restore macros. They should be used by
+ * every callable non-leaf asm function.
+ */
+.macro FP_SAVE name
+ .if CONFIG_FRAME_POINTER
+ push %_ASM_BP
+ _ASM_MOV %_ASM_SP, %_ASM_BP
+ .endif
+.endm
+
+.macro FP_RESTORE name
+ .if CONFIG_FRAME_POINTER
+ pop %_ASM_BP
+ .endif
+.endm
+
+#endif /* _ASM_X86_FUNC_H */
--
2.1.0
Add a new CONFIG_ASM_VALIDATION option which adds an asmvalidate host
tool which runs on every compiled .S file. Its goal is to enforce sane
rules on all asm code, so that stack debug metadata (frame/back chain
pointers and/or DWARF CFI metadata) can be made reliable.
It enforces the following rules:
1. Each callable function must be annotated with the ELF STT_FUNC type.
This is typically done using the ENTRY/ENDPROC macros. If
asmvalidate finds a return instruction outside of a function, it
flags an error, since that usually indicates callable code which
should be annotated accordingly.
2. Each callable function must never leave its own bounds (i.e. with a
jump to outside the function) except when returning.
3. Each callable non-leaf function must have frame pointer logic (if
required by CONFIG_FRAME_POINTER or the architecture's back chain
rules). This can be done with the new FP_SAVE/FP_RESTORE macros.
It currently only supports x86_64. It *almost* supports x86_32, but the
stackvalidate code doesn't yet know how to deal with 32-bit REL
relocations for the return whitelists. I tried to make the code generic
so that support for other architectures can be plugged in pretty easily.
As a first step, CONFIG_ASM_VALIDATION is disabled by default, and all
reported non-compliances result in warnings. Once we get them all
cleaned up, we can change the default to CONFIG_ASM_VALIDATION=y and
change the warnings to build errors so the asm code can stay clean.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
MAINTAINERS | 6 +
arch/Kconfig | 3 +
arch/x86/Kconfig | 1 +
arch/x86/Makefile | 6 +-
arch/x86/include/asm/func.h | 32 ++++
lib/Kconfig.debug | 21 +++
scripts/Makefile | 1 +
scripts/Makefile.build | 23 ++-
scripts/asmvalidate/Makefile | 17 ++
scripts/asmvalidate/arch-x86.c | 283 ++++++++++++++++++++++++++++++
scripts/asmvalidate/arch.h | 40 +++++
scripts/asmvalidate/asmvalidate.c | 324 ++++++++++++++++++++++++++++++++++
scripts/asmvalidate/elf.c | 354 ++++++++++++++++++++++++++++++++++++++
scripts/asmvalidate/elf.h | 74 ++++++++
scripts/asmvalidate/list.h | 217 +++++++++++++++++++++++
15 files changed, 1399 insertions(+), 3 deletions(-)
create mode 100644 scripts/asmvalidate/Makefile
create mode 100644 scripts/asmvalidate/arch-x86.c
create mode 100644 scripts/asmvalidate/arch.h
create mode 100644 scripts/asmvalidate/asmvalidate.c
create mode 100644 scripts/asmvalidate/elf.c
create mode 100644 scripts/asmvalidate/elf.h
create mode 100644 scripts/asmvalidate/list.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 469cd4d..831bf8b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1664,6 +1664,12 @@ S: Maintained
F: Documentation/hwmon/asc7621
F: drivers/hwmon/asc7621.c
+ASM VALIDATION
+M: Josh Poimboeuf <[email protected]>
+S: Supported
+F: scripts/asmvalidate/
+F: arch/x86/include/asm/func.h
+
ASUS NOTEBOOKS AND EEEPC ACPI/WMI EXTRAS DRIVERS
M: Corentin Chary <[email protected]>
L: [email protected]
diff --git a/arch/Kconfig b/arch/Kconfig
index a65eafb..8d85326 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -499,6 +499,9 @@ config ARCH_HAS_ELF_RANDOMIZE
- arch_mmap_rnd()
- arch_randomize_brk()
+config HAVE_ASM_VALIDATION
+ bool
+
#
# ABI hall of shame
#
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 228aa35..a85e149 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -144,6 +144,7 @@ config X86
select VIRT_TO_BUS
select X86_DEV_DMA_OPS if X86_64
select X86_FEATURE_NAMES if PROC_FS
+ select HAVE_ASM_VALIDATION if X86_64
config INSTRUCTION_DECODER
def_bool y
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 118e6de..e2dd40e 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -174,9 +174,13 @@ KBUILD_CFLAGS += $(call cc-option,-mno-avx,)
KBUILD_CFLAGS += $(mflags-y)
KBUILD_AFLAGS += $(mflags-y)
-archscripts: scripts_basic
+archscripts: scripts_basic $(objtree)/arch/x86/lib/inat-tables.c
$(Q)$(MAKE) $(build)=arch/x86/tools relocs
+# this file is needed early by scripts/asmvalidate
+$(objtree)/arch/x86/lib/inat-tables.c:
+ $(Q)$(MAKE) $(build)=arch/x86/lib $@
+
###
# Syscall table generation
diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
index 4d62782..52b3225 100644
--- a/arch/x86/include/asm/func.h
+++ b/arch/x86/include/asm/func.h
@@ -9,6 +9,14 @@
* every callable non-leaf asm function.
*/
.macro FP_SAVE name
+ .if CONFIG_ASM_VALIDATION
+ 163:
+ .pushsection __asmvalidate_fp_save, "ae"
+ _ASM_ALIGN
+ .long 163b - .
+ .popsection
+ .endif
+
.if CONFIG_FRAME_POINTER
push %_ASM_BP
_ASM_MOV %_ASM_SP, %_ASM_BP
@@ -19,6 +27,30 @@
.if CONFIG_FRAME_POINTER
pop %_ASM_BP
.endif
+
+ .if CONFIG_ASM_VALIDATION
+ 163:
+ .pushsection __asmvalidate_fp_restore, "ae"
+ _ASM_ALIGN
+ .long 163b - .
+ .popsection
+ .endif
+.endm
+
+/*
+ * This macro tells the asm validation script to ignore the instruction
+ * immediately after the macro. It should only be used in special cases where
+ * you're 100% sure that the asm validation constraints don't need to be
+ * adhered to. Use with caution!
+ */
+.macro ASMVALIDATE_IGNORE
+ .if CONFIG_ASM_VALIDATION
+ 163:
+ .pushsection __asmvalidate_ignore, "ae"
+ _ASM_ALIGN
+ .long 163b - .
+ .popsection
+ .endif
.endm
#endif /* _ASM_X86_FUNC_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index b908048..119dfd1 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -332,6 +332,27 @@ config FRAME_POINTER
larger and slower, but it gives very useful debugging information
in case of kernel bugs. (precise oopses/stacktraces/warnings)
+
+config ASM_VALIDATION
+ bool "Enable compile-time asm code validation"
+ depends on HAVE_ASM_VALIDATION
+ default n
+ help
+ Add compile-time checks to enforce sane rules on all asm code, so
+ that stack debug metadata can be more reliable. Enforces the
+ following rules:
+
+ 1. Each callable function must be annotated with the ELF STT_FUNC
+ type.
+
+ 2. Each callable function must never leave its own bounds except when
+ returning.
+
+ 3. Each callable non-leaf function must have frame pointer logic (if
+ required by CONFIG_FRAME_POINTER or the architecture's back chain
+ rules.
+
+
config DEBUG_FORCE_WEAK_PER_CPU
bool "Force weak per-cpu definitions"
depends on DEBUG_KERNEL
diff --git a/scripts/Makefile b/scripts/Makefile
index 2016a64..c4c8350 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -37,6 +37,7 @@ subdir-y += mod
subdir-$(CONFIG_SECURITY_SELINUX) += selinux
subdir-$(CONFIG_DTC) += dtc
subdir-$(CONFIG_GDB_SCRIPTS) += gdb
+subdir-$(CONFIG_ASM_VALIDATION) += asmvalidate
# Let clean descend into subdirs
subdir- += basic kconfig package
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 01df30a..8bf1085 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -253,6 +253,25 @@ define rule_cc_o_c
mv -f $(dot-target).tmp $(dot-target).cmd
endef
+ifdef CONFIG_ASM_VALIDATION
+asmvalidate = $(objtree)/scripts/asmvalidate/asmvalidate
+cmd_asmvalidate = \
+ case $(@) in \
+ arch/x86/purgatory/*) ;; \
+ arch/x86/realmode/rm/*) ;; \
+ *) $(asmvalidate) "$(@)"; \
+ esac;
+endif
+
+define rule_as_o_S
+ $(call echo-cmd,as_o_S) $(cmd_as_o_S); \
+ $(cmd_asmvalidate) \
+ scripts/basic/fixdep $(depfile) $@ '$(call make-cmd,as_o_S)' > \
+ $(dot-target).tmp; \
+ rm -f $(depfile); \
+ mv -f $(dot-target).tmp $(dot-target).cmd
+endef
+
# Built-in and composite module parts
$(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE
$(call cmd,force_checksrc)
@@ -290,8 +309,8 @@ $(obj)/%.s: $(src)/%.S FORCE
quiet_cmd_as_o_S = AS $(quiet_modtag) $@
cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
-$(obj)/%.o: $(src)/%.S FORCE
- $(call if_changed_dep,as_o_S)
+$(obj)/%.o: $(src)/%.S $(asmvalidate) FORCE
+ $(call if_changed_rule,as_o_S)
targets += $(real-objs-y) $(real-objs-m) $(lib-y)
targets += $(extra-y) $(MAKECMDGOALS) $(always)
diff --git a/scripts/asmvalidate/Makefile b/scripts/asmvalidate/Makefile
new file mode 100644
index 0000000..a202ad6
--- /dev/null
+++ b/scripts/asmvalidate/Makefile
@@ -0,0 +1,17 @@
+hostprogs-y := asmvalidate
+always := $(hostprogs-y)
+
+asmvalidate-objs := asmvalidate.o elf.o
+
+HOSTCFLAGS += -Werror
+HOSTLOADLIBES_asmvalidate := -lelf
+
+ifdef CONFIG_X86
+
+asmvalidate-objs += arch-x86.o
+
+HOSTCFLAGS_arch-x86.o := -I$(objtree)/arch/x86/lib/ -I$(srctree)/arch/x86/include/ -I$(srctree)/arch/x86/lib/
+
+$(obj)/arch-x86.o: $(srctree)/arch/x86/lib/insn.c $(srctree)/arch/x86/lib/inat.c $(srctree)/arch/x86/include/asm/inat_types.h $(srctree)/arch/x86/include/asm/inat.h $(srctree)/arch/x86/include/asm/insn.h $(objtree)/arch/x86/lib/inat-tables.c
+
+endif
diff --git a/scripts/asmvalidate/arch-x86.c b/scripts/asmvalidate/arch-x86.c
new file mode 100644
index 0000000..87e7073
--- /dev/null
+++ b/scripts/asmvalidate/arch-x86.c
@@ -0,0 +1,283 @@
+/*
+ * Copyright (C) 2015 Josh Poimboeuf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+
+#define unlikely(cond) (cond)
+#include <asm/insn.h>
+#include <inat.c>
+#include <insn.c>
+
+#include "elf.h"
+#include "arch.h"
+
+static int is_x86_64(struct elf *elf)
+{
+ switch (elf->ehdr.e_machine) {
+ case EM_X86_64:
+ return 1;
+ case EM_386:
+ return 0;
+ default:
+ WARN("unexpected ELF machine type %d", elf->ehdr.e_machine);
+ return -1;
+ }
+}
+
+int arch_insn_decode(struct elf *elf, struct arch_insn *arch_insn,
+ unsigned long addr, unsigned int maxlen)
+{
+ struct insn insn;
+ int x86_64;
+ unsigned char op1, op2, ext;
+
+ x86_64 = is_x86_64(elf);
+ if (x86_64 == -1)
+ return 1;
+
+ insn_init(&insn, (void *)addr, maxlen, x86_64);
+ insn_get_length(&insn);
+ insn_get_opcode(&insn);
+ insn_get_modrm(&insn);
+ insn_get_immediate(&insn);
+
+ if (!insn.opcode.got)
+ return 1;
+
+ memset(arch_insn, 0, sizeof(*arch_insn));
+
+ arch_insn->len = insn.length;
+
+ if (insn.vex_prefix.nbytes)
+ return 0;
+
+ op1 = insn.opcode.bytes[0];
+ op2 = insn.opcode.bytes[1];
+
+ switch (op1) {
+ case 0x70 ... 0x7f:
+ arch_insn->jump = true;
+ break;
+ case 0x0f:
+ if (op2 >= 0x80 && op2 <= 0x8f)
+ arch_insn->jump = true;
+ break;
+ case 0xe3:
+ arch_insn->jump = true;
+ break;
+ case 0xe9:
+ case 0xeb:
+ arch_insn->jump = true;
+ arch_insn->unconditional = true;
+ break;
+
+ case 0xc2:
+ case 0xc3:
+ arch_insn->ret = true;
+ break;
+ case 0xe8:
+ arch_insn->call = true;
+ break;
+ case 0xff:
+ ext = X86_MODRM_REG(insn.modrm.bytes[0]);
+ if (ext == 2 || ext == 3)
+ arch_insn->call = true;
+ else if (ext == 4 || ext == 5) {
+ arch_insn->jump = true;
+ arch_insn->unconditional = true;
+ }
+ break;
+ }
+
+ if (arch_insn->jump && insn.immediate.value)
+ arch_insn->dest = addr + insn.length + insn.immediate.value;
+
+ return 0;
+}
+
+static struct rela *find_rela_at_offset(struct section *sec,
+ unsigned long offset)
+{
+ struct rela *rela;
+
+ list_for_each_entry(rela, &sec->relas, list)
+ if (rela->offset == offset)
+ return rela;
+
+ return NULL;
+}
+
+static struct symbol *find_containing_func(struct section *sec,
+ unsigned long offset)
+{
+ struct symbol *sym;
+ unsigned long addr = sec->start + offset;
+
+ list_for_each_entry(sym, &sec->symbols, list) {
+ if (sym->type != STT_FUNC)
+ continue;
+ if (addr >= sym->start && addr < sym->end)
+ return sym;
+ }
+
+ return NULL;
+}
+
+#define ALT_ENTRY_SIZE 13
+#define ALT_INSTR_OFFSET 0
+#define ALT_REPL_OFFSET 4
+#define ALT_REPL_LEN_OFFSET 11
+
+static int validate_alternative_insn(struct elf *elf, struct symbol *old_func,
+ struct section *replacesec,
+ unsigned long offset, int maxlen,
+ unsigned int *len)
+{
+ struct rela *dest_rela;
+ struct arch_insn insn;
+ int ret, warnings = 0;
+
+ ret = arch_insn_decode(elf, &insn, replacesec->start + offset, maxlen);
+ if (ret) {
+ WARN("can't decode instruction at %s+0x%lx", replacesec->name,
+ offset);
+ return -1;
+ }
+
+ *len = insn.len;
+
+ if (insn.call) {
+ WARN("call instruction in %s", replacesec->name);
+ return 1;
+ } else if (insn.jump) {
+ if (insn.dest) {
+ WARN("unexpected non-relocated jump dest at %s+0x%lx",
+ replacesec->name, offset);
+ return -1;
+ }
+
+ dest_rela = find_rela_at_offset(replacesec->rela, offset + 1);
+ if (!dest_rela) {
+ WARN("can't find rela for jump instruction at %s+0x%lx",
+ replacesec->name, offset);
+ return -1;
+ }
+
+ if (old_func &&
+ !(dest_rela->sym->sec == old_func->sec &&
+ dest_rela->addend+dest_rela->sym->start+4 >= old_func->start &&
+ dest_rela->addend+dest_rela->sym->start+4 < old_func->end)) {
+ WARN("alternative jump to outside the scope of original function %s",
+ old_func->name);
+ warnings++;
+ }
+ }
+
+ return warnings;
+}
+
+static int validate_alternative_entry(struct elf *elf, struct section *altsec,
+ struct section *replacesec, int entry)
+{
+ struct rela *old_rela, *new_rela;
+ struct symbol *old_func;
+ unsigned long alt_offset, insn_offset;
+ unsigned int insn_len;
+ unsigned char new_insns_len;
+ int ret, warnings = 0;
+
+ alt_offset = entry * ALT_ENTRY_SIZE;
+
+ old_rela = find_rela_at_offset(altsec->rela,
+ alt_offset + ALT_INSTR_OFFSET);
+ if (!old_rela) {
+ WARN("can't find altinstructions rela at offset 0x%lx",
+ alt_offset + ALT_INSTR_OFFSET);
+ return -1;
+ }
+
+ if (old_rela->sym->type != STT_SECTION) {
+ WARN("don't know how to deal with non-section symbol %s",
+ old_rela->sym->name);
+ return -1;
+ }
+
+ old_func = find_containing_func(old_rela->sym->sec, old_rela->addend);
+
+ new_rela = find_rela_at_offset(altsec->rela,
+ alt_offset + ALT_REPL_OFFSET);
+ if (!new_rela) {
+ WARN("can't find altinstructions rela at offset 0x%lx",
+ alt_offset + ALT_REPL_OFFSET);
+ return -1;
+ }
+
+ if (new_rela->sym != replacesec->sym) {
+ WARN("new_rela sym %s isn't .altinstr_replacement",
+ new_rela->sym->name);
+ return -1;
+ }
+
+ new_insns_len = *(unsigned char *)(altsec->start + alt_offset + ALT_REPL_LEN_OFFSET);
+
+ for (insn_offset = new_rela->addend; new_insns_len > 0;
+ insn_offset += insn_len, new_insns_len -= insn_len) {
+ ret = validate_alternative_insn(elf, old_func, replacesec,
+ insn_offset, new_insns_len,
+ &insn_len);
+ if (ret < 0)
+ return ret;
+
+ warnings += ret;
+ }
+
+ return warnings;
+}
+
+int arch_validate_alternatives(struct elf *elf)
+{
+ struct section *altsec, *replacesec;
+ int entry, ret, warnings = 0;
+ unsigned int nr_entries;
+
+ altsec = elf_find_section_by_name(elf, ".altinstructions");
+ if (!altsec)
+ return 0;
+
+ if ((altsec->end - altsec->start) % ALT_ENTRY_SIZE != 0) {
+ WARN(".altinstructions size not a multiple of %d",
+ ALT_ENTRY_SIZE);
+ return -1;
+ }
+
+ nr_entries = (altsec->end - altsec->start) / ALT_ENTRY_SIZE;
+
+ replacesec = elf_find_section_by_name(elf, ".altinstr_replacement");
+ if (!replacesec)
+ return 0;
+
+ for (entry = 0; entry < nr_entries; entry++) {
+ ret = validate_alternative_entry(elf, altsec, replacesec,
+ entry);
+ if (ret < 0)
+ return ret;
+
+ warnings += ret;
+ }
+
+ return warnings;
+}
diff --git a/scripts/asmvalidate/arch.h b/scripts/asmvalidate/arch.h
new file mode 100644
index 0000000..da85a18
--- /dev/null
+++ b/scripts/asmvalidate/arch.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2015 Josh Poimboeuf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ARCH_H_
+#define _ARCH_H_
+
+#include <stdbool.h>
+#include "elf.h"
+
+struct arch_insn {
+ bool call, ret, jump;
+ unsigned int len;
+
+ /* jump-specific variables */
+ bool unconditional;
+ unsigned long dest;
+};
+
+/* decode an instruction and populate the arch_insn accordingly */
+int arch_insn_decode(struct elf *elf, struct arch_insn *arch_insn,
+ unsigned long addr, unsigned int maxlen);
+
+/* validate any .altinstructions sections */
+int arch_validate_alternatives(struct elf *elf);
+
+#endif /* _ARCH_H_ */
diff --git a/scripts/asmvalidate/asmvalidate.c b/scripts/asmvalidate/asmvalidate.c
new file mode 100644
index 0000000..2c586c0
--- /dev/null
+++ b/scripts/asmvalidate/asmvalidate.c
@@ -0,0 +1,324 @@
+/*
+ * Copyright (C) 2015 Josh Poimboeuf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * asmvalidate:
+ *
+ * This tool runs on every compiled .S file. Its goal is to enforce sane rules
+ * on all asm code, so that stack debug metadata (frame/back chain pointers
+ * and/or DWARF CFI metadata) can be made reliable.
+ *
+ * It enforces the following rules:
+ *
+ * 1. Each callable function must be annotated with the ELF STT_FUNC type.
+ * This is typically done using the ENTRY/ENDPROC macros. If asmvalidate
+ * finds a return instruction outside of a function, it flags an error,
+ * since that usually indicates callable code which should be annotated
+ * accordingly.
+ *
+ * 2. Each callable function must never leave its own bounds (i.e. with a jump
+ * to outside the function) except when returning.
+ *
+ * 3. Each callable non-leaf function must have frame pointer logic (if
+ * required by CONFIG_FRAME_POINTER or the architecture's back chain rules).
+ * This should by done by the FP_SAVE/FP_RESTORE macros.
+ *
+ */
+
+#include <argp.h>
+#include <stdbool.h>
+#include <string.h>
+
+#include "elf.h"
+#include "arch.h"
+
+int warnings;
+
+struct args {
+ char *args[1];
+};
+static const char args_doc[] = "file.o";
+static struct argp_option options[] = {
+ {0},
+};
+static error_t parse_opt(int key, char *arg, struct argp_state *state)
+{
+ /* Get the input argument from argp_parse, which we
+ know is a pointer to our args structure. */
+ struct args *args = state->input;
+
+ switch (key) {
+ case ARGP_KEY_ARG:
+ if (state->arg_num >= 1)
+ /* Too many arguments. */
+ argp_usage(state);
+ args->args[state->arg_num] = arg;
+ break;
+ case ARGP_KEY_END:
+ if (state->arg_num < 1)
+ /* Not enough arguments. */
+ argp_usage(state);
+ break;
+ default:
+ return ARGP_ERR_UNKNOWN;
+ }
+ return 0;
+}
+static struct argp argp = { options, parse_opt, args_doc, 0 };
+
+static bool used_macro(struct elf *elf, struct section *sec,
+ unsigned long offset, const char *macro)
+{
+ struct section *macro_sec;
+ struct rela *rela;
+ char rela_sec[51];
+
+ strcpy(rela_sec, ".rela__asmvalidate_");
+ strncat(rela_sec, macro, 50 - strlen(rela_sec));
+
+ macro_sec = elf_find_section_by_name(elf, rela_sec);
+ if (!macro_sec)
+ return false;
+
+ list_for_each_entry(rela, ¯o_sec->relas, list)
+ if (rela->sym->type == STT_SECTION &&
+ rela->sym == sec->sym &&
+ rela->addend == offset)
+ return true;
+
+ return false;
+}
+
+/*
+ * Check if the ASMVALIDATE_IGNORE macro was used at the given section offset.
+ */
+static bool ignore_macro(struct elf *elf, struct section *sec,
+ unsigned long offset)
+{
+ return used_macro(elf, sec, offset, "ignore");
+}
+
+/*
+ * Check if the FP_SAVE macro was used at the given section offset.
+ */
+static bool fp_save_macro(struct elf *elf, struct section *sec,
+ unsigned long offset)
+{
+ return used_macro(elf, sec, offset, "fp_save");
+}
+
+/*
+ * Check if the FP_RESTORE macro was used at the given section offset.
+ */
+static bool fp_restore_macro(struct elf *elf, struct section *sec,
+ unsigned long offset)
+{
+ return used_macro(elf, sec, offset, "fp_restore");
+}
+
+/*
+ * For the given collection of instructions which are outside an STT_FUNC
+ * function, ensure there are no (un-whitelisted) return instructions.
+ */
+static int validate_nonfunction(struct elf *elf, struct section *sec,
+ unsigned long offset, unsigned int len)
+{
+ unsigned long insn_offset;
+ struct arch_insn insn;
+ int ret, warnings = 0;
+
+ for (insn_offset = offset; len > 0;
+ insn_offset += insn.len, len -= insn.len) {
+ ret = arch_insn_decode(elf, &insn, sec->start + insn_offset,
+ len);
+ if (ret)
+ return -1;
+
+ if (insn.ret && !ignore_macro(elf, sec, insn_offset)) {
+ WARN("%s+0x%lx: return instruction outside of a function",
+ sec->name, insn_offset);
+ warnings++;
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * validate_function():
+ *
+ * 1. Ensure the function never leaves its own bounds.
+ *
+ * 2. If it's a non-leaf function, ensure that it uses the frame pointer macros
+ * (FP_SAVE and FP_RESTORE).
+ *
+ * Return value:
+ * -1: bad instruction
+ * 1: missing frame pointer logic
+ * 0: validation succeeded
+ */
+static int validate_function(struct elf *elf, struct symbol *func)
+{
+ struct section *sec = func->sec;
+ unsigned long addr;
+ struct arch_insn insn;
+ bool leaf = true, fp;
+ int ret, warnings = 0;
+
+ fp = fp_save_macro(elf, sec, func->start - sec->start);
+
+ for (addr = func->start; addr < func->end; addr += insn.len) {
+ ret = arch_insn_decode(elf, &insn, addr, func->end - addr);
+ if (ret)
+ return -1;
+
+ if (insn.call)
+ leaf = false;
+ else if (insn.ret) {
+ if (fp &&
+ !fp_restore_macro(elf, sec, addr - sec->start)) {
+ WARN("%s()+0x%lx: return not preceded by FP_RESTORE macro",
+ func->name, addr - func->start);
+ warnings++;
+ }
+ }
+ else if (insn.jump &&
+ (insn.dest < func->start ||
+ insn.dest >= func->end) &&
+ !ignore_macro(elf, sec, addr - sec->start)) {
+ WARN("%s()+0x%lx: unsupported jump to outside of function",
+ func->name, addr - func->start);
+ warnings++;
+ }
+ }
+
+ if (!insn.ret &&
+ !(insn.unconditional &&
+ insn.dest >= func->start && insn.dest < func->end)) {
+ WARN("%s(): unsupported fallthrough at end of function",
+ func->name);
+ warnings++;
+ }
+
+ if (!leaf && !fp) {
+ WARN("%s(): missing FP_SAVE/RESTORE macros", func->name);
+ warnings++;
+ }
+
+ return warnings;
+}
+
+/*
+ * For the given section, find all functions and non-function regions and
+ * validate them accordingly.
+ */
+static int validate_section(struct elf *elf, struct section *sec)
+{
+ struct symbol *func, *last_func;
+ struct symbol null_func = {};
+ int ret, warnings = 0;
+
+ if (!(sec->sh.sh_flags & SHF_EXECINSTR))
+ return 0;
+
+ if (list_empty(&sec->symbols)) {
+ WARN("%s: no symbols", sec->name);
+ return -1;
+ }
+
+ /* alternatives are validated later by arch_validate_alternatives() */
+ if (!strcmp(sec->name, ".altinstr_replacement"))
+ return 0;
+
+ last_func = &null_func;
+ last_func->start = last_func->end = sec->start;
+ list_for_each_entry(func, &sec->symbols, list) {
+ if (func->type != STT_FUNC)
+ continue;
+
+ if (func->start != last_func->start &&
+ func->end != last_func->end &&
+ func->start < last_func->end) {
+ WARN("overlapping functions %s and %s",
+ last_func->name, func->name);
+ warnings++;
+ }
+
+ if (func->start > last_func->end) {
+ ret = validate_nonfunction(elf, sec,
+ last_func->end - sec->start,
+ func->start - last_func->end);
+ if (ret < 0)
+ return -1;
+
+ warnings += ret;
+ }
+
+ ret = validate_function(elf, func);
+ if (ret < 0)
+ return -1;
+
+ warnings += ret;
+
+ last_func = func;
+ }
+
+ if (last_func->end < sec->end) {
+ ret = validate_nonfunction(elf, sec,
+ last_func->end - sec->start,
+ sec->end - last_func->end);
+ if (ret < 0)
+ return -1;
+
+ warnings += ret;
+ }
+
+ return warnings;
+}
+
+int main(int argc, char *argv[])
+{
+ struct args args;
+ struct elf *elf;
+ struct section *sec;
+ int ret, warnings = 0;
+
+ argp_parse(&argp, argc, argv, 0, 0, &args);
+
+ elf = elf_open(args.args[0]);
+ if (!elf) {
+ fprintf(stderr, "error reading elf file %s\n", args.args[0]);
+ return 1;
+ }
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ ret = validate_section(elf, sec);
+ if (ret < 0)
+ return 1;
+
+ warnings += ret;
+ }
+
+ ret = arch_validate_alternatives(elf);
+ if (ret < 0)
+ return 1;
+
+ warnings += ret;
+
+ /* ignore warnings for now until we get all the asm code cleaned up */
+ return 0;
+}
diff --git a/scripts/asmvalidate/elf.c b/scripts/asmvalidate/elf.c
new file mode 100644
index 0000000..b2b0986
--- /dev/null
+++ b/scripts/asmvalidate/elf.c
@@ -0,0 +1,354 @@
+/*
+ * elf.c - ELF access library
+ *
+ * Adapted from kpatch (https://github.com/dynup/kpatch):
+ * Copyright (C) 2013-2015 Josh Poimboeuf <[email protected]>
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "elf.h"
+
+struct section *elf_find_section_by_name(struct elf *elf, const char *name)
+{
+ struct section *sec;
+
+ list_for_each_entry(sec, &elf->sections, list)
+ if (!strcmp(sec->name, name))
+ return sec;
+
+ return NULL;
+}
+
+static struct section *elf_find_section_by_index(struct elf *elf,
+ unsigned int index)
+{
+ struct section *sec;
+
+ list_for_each_entry(sec, &elf->sections, list)
+ if (sec->index == index)
+ return sec;
+
+ return NULL;
+}
+
+static struct symbol *elf_find_symbol_by_index(struct elf *elf,
+ unsigned int index)
+{
+ struct section *sec;
+ struct symbol *sym;
+
+ list_for_each_entry(sec, &elf->sections, list)
+ list_for_each_entry(sym, &sec->symbols, list)
+ if (sym->index == index)
+ return sym;
+
+ return NULL;
+}
+
+static int elf_read_sections(struct elf *elf)
+{
+ Elf_Scn *s = NULL;
+ struct section *sec;
+ size_t shstrndx, sections_nr;
+ int i;
+
+ if (elf_getshdrnum(elf->elf, §ions_nr)) {
+ perror("elf_getshdrnum");
+ return -1;
+ }
+
+ if (elf_getshdrstrndx(elf->elf, &shstrndx)) {
+ perror("elf_getshdrstrndx");
+ return -1;
+ }
+
+ for (i = 0; i < sections_nr; i++) {
+ sec = malloc(sizeof(*sec));
+ if (!sec) {
+ perror("malloc");
+ return -1;
+ }
+ memset(sec, 0, sizeof(*sec));
+
+ INIT_LIST_HEAD(&sec->symbols);
+ INIT_LIST_HEAD(&sec->relas);
+
+ list_add_tail(&sec->list, &elf->sections);
+
+ s = elf_getscn(elf->elf, i);
+ if (!s) {
+ perror("elf_getscn");
+ return -1;
+ }
+
+ sec->index = elf_ndxscn(s);
+
+ if (!gelf_getshdr(s, &sec->sh)) {
+ perror("gelf_getshdr");
+ return -1;
+ }
+
+ sec->name = elf_strptr(elf->elf, shstrndx, sec->sh.sh_name);
+ if (!sec->name) {
+ perror("elf_strptr");
+ return -1;
+ }
+
+ sec->data = elf_getdata(s, NULL);
+ if (!sec->data) {
+ perror("elf_getdata");
+ return -1;
+ }
+
+ if (sec->data->d_off != 0 ||
+ sec->data->d_size != sec->sh.sh_size) {
+ WARN("unexpected data attributes for %s", sec->name);
+ return -1;
+ }
+
+ sec->start = (unsigned long)sec->data->d_buf;
+ sec->end = sec->start + sec->data->d_size;
+ }
+
+ /* sanity check, one more call to elf_nextscn() should return NULL */
+ if (elf_nextscn(elf->elf, s)) {
+ WARN("section entry mismatch");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int elf_read_symbols(struct elf *elf)
+{
+ struct section *symtab;
+ struct symbol *sym;
+ struct list_head *entry, *tmp;
+ int symbols_nr, i;
+
+ symtab = elf_find_section_by_name(elf, ".symtab");
+ if (!symtab) {
+ WARN("missing symbol table");
+ return -1;
+ }
+
+ symbols_nr = symtab->sh.sh_size / symtab->sh.sh_entsize;
+
+ for (i = 0; i < symbols_nr; i++) {
+ sym = malloc(sizeof(*sym));
+ if (!sym) {
+ perror("malloc");
+ return -1;
+ }
+ memset(sym, 0, sizeof(*sym));
+
+ sym->index = i;
+
+ if (!gelf_getsym(symtab->data, i, &sym->sym)) {
+ perror("gelf_getsym");
+ goto err;
+ }
+
+ sym->name = elf_strptr(elf->elf, symtab->sh.sh_link,
+ sym->sym.st_name);
+ if (!sym->name) {
+ perror("elf_strptr");
+ goto err;
+ }
+
+ sym->type = GELF_ST_TYPE(sym->sym.st_info);
+ sym->bind = GELF_ST_BIND(sym->sym.st_info);
+
+ if (sym->sym.st_shndx > SHN_UNDEF &&
+ sym->sym.st_shndx < SHN_LORESERVE) {
+ sym->sec = elf_find_section_by_index(elf,
+ sym->sym.st_shndx);
+ if (!sym->sec) {
+ WARN("couldn't find section for symbol %s",
+ sym->name);
+ goto err;
+ }
+ if (sym->type == STT_SECTION) {
+ sym->name = sym->sec->name;
+ sym->sec->sym = sym;
+ }
+ } else
+ sym->sec = elf_find_section_by_index(elf, 0);
+
+ sym->start = sym->sec->start + sym->sym.st_value;
+ sym->end = sym->start + sym->sym.st_size;
+
+ /* sorted insert into a per-section list */
+ entry = &sym->sec->symbols;
+ list_for_each_prev(tmp, &sym->sec->symbols) {
+ struct symbol *s;
+
+ s = list_entry(tmp, struct symbol, list);
+
+ if (sym->start > s->start) {
+ entry = tmp;
+ break;
+ }
+
+ if (sym->start == s->start && sym->end >= s->end) {
+ entry = tmp;
+ break;
+ }
+ }
+ list_add(&sym->list, entry);
+ }
+
+ return 0;
+
+err:
+ free(sym);
+ return -1;
+}
+
+static int elf_read_relas(struct elf *elf)
+{
+ struct section *sec;
+ struct rela *rela;
+ int i;
+ unsigned int symndx;
+
+ list_for_each_entry(sec, &elf->sections, list) {
+ if (sec->sh.sh_type != SHT_RELA)
+ continue;
+
+ sec->base = elf_find_section_by_name(elf, sec->name + 5);
+ if (!sec->base) {
+ WARN("can't find base section for rela section %s",
+ sec->name);
+ return -1;
+ }
+
+ sec->base->rela = sec;
+
+ for (i = 0; i < sec->sh.sh_size / sec->sh.sh_entsize; i++) {
+ rela = malloc(sizeof(*rela));
+ if (!rela) {
+ perror("malloc");
+ return -1;
+ }
+ memset(rela, 0, sizeof(*rela));
+
+ list_add_tail(&rela->list, &sec->relas);
+
+ if (!gelf_getrela(sec->data, i, &rela->rela)) {
+ perror("gelf_getrela");
+ return -1;
+ }
+
+ rela->type = GELF_R_TYPE(rela->rela.r_info);
+ rela->addend = rela->rela.r_addend;
+ rela->offset = rela->rela.r_offset;
+ symndx = GELF_R_SYM(rela->rela.r_info);
+ rela->sym = elf_find_symbol_by_index(elf, symndx);
+ if (!rela->sym) {
+ WARN("can't find rela entry symbol %d for %s",
+ symndx, sec->name);
+ return -1;
+ }
+ }
+ }
+
+ return 0;
+}
+
+struct elf *elf_open(const char *name)
+{
+ struct elf *elf;
+
+ elf_version(EV_CURRENT);
+
+ elf = malloc(sizeof(*elf));
+ if (!elf) {
+ perror("malloc");
+ return NULL;
+ }
+ memset(elf, 0, sizeof(*elf));
+
+ INIT_LIST_HEAD(&elf->sections);
+
+ elf->name = strdup(name);
+ if (!elf->name) {
+ perror("strdup");
+ goto err;
+ }
+
+ elf->fd = open(name, O_RDONLY);
+ if (elf->fd == -1) {
+ perror("open");
+ goto err;
+ }
+
+ elf->elf = elf_begin(elf->fd, ELF_C_READ_MMAP, NULL);
+ if (!elf->elf) {
+ perror("elf_begin");
+ goto err;
+ }
+
+ if (!gelf_getehdr(elf->elf, &elf->ehdr)) {
+ perror("gelf_getehdr");
+ goto err;
+ }
+
+ if (elf_read_sections(elf))
+ goto err;
+
+ if (elf_read_symbols(elf))
+ goto err;
+
+ if (elf_read_relas(elf))
+ goto err;
+
+ return elf;
+
+err:
+ elf_close(elf);
+ return NULL;
+}
+
+void elf_close(struct elf *elf)
+{
+ struct section *sec, *tmpsec;
+ struct symbol *sym, *tmpsym;
+
+ list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
+ list_for_each_entry_safe(sym, tmpsym, &sec->symbols, list) {
+ list_del(&sym->list);
+ free(sym);
+ }
+ list_del(&sec->list);
+ free(sec);
+ }
+ if (elf->name)
+ free(elf->name);
+ if (elf->fd > 0)
+ close(elf->fd);
+ if (elf->elf)
+ elf_end(elf->elf);
+ free(elf);
+}
diff --git a/scripts/asmvalidate/elf.h b/scripts/asmvalidate/elf.h
new file mode 100644
index 0000000..abfc902
--- /dev/null
+++ b/scripts/asmvalidate/elf.h
@@ -0,0 +1,74 @@
+/*
+ * Copyright (C) 2015 Josh Poimboeuf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ELF_H_
+#define _ELF_H_
+
+#include <gelf.h>
+#include "list.h"
+
+#define WARN(format, ...) \
+ fprintf(stderr, \
+ "asmvalidate: %s: " format "\n", \
+ elf->name, ##__VA_ARGS__)
+
+struct section {
+ struct list_head list;
+ GElf_Shdr sh;
+ struct list_head symbols;
+ struct list_head relas;
+ struct section *base, *rela;
+ struct symbol *sym;
+ Elf_Data *data;
+ char *name;
+ int index;
+ unsigned long start, end;
+};
+
+struct symbol {
+ struct list_head list;
+ GElf_Sym sym;
+ struct section *sec;
+ char *name;
+ int index;
+ unsigned char bind, type;
+ unsigned long start, end;
+};
+
+struct rela {
+ struct list_head list;
+ GElf_Rela rela;
+ struct symbol *sym;
+ unsigned int type;
+ int offset;
+ int addend;
+};
+
+struct elf {
+ Elf *elf;
+ GElf_Ehdr ehdr;
+ int fd;
+ char *name;
+ struct list_head sections;
+};
+
+
+struct elf *elf_open(const char *name);
+struct section *elf_find_section_by_name(struct elf *elf, const char *name);
+void elf_close(struct elf *elf);
+
+#endif /* _ELF_H_ */
diff --git a/scripts/asmvalidate/list.h b/scripts/asmvalidate/list.h
new file mode 100644
index 0000000..25716b5
--- /dev/null
+++ b/scripts/asmvalidate/list.h
@@ -0,0 +1,217 @@
+#ifndef _LIST_H
+#define _LIST_H
+
+#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
+
+#define container_of(ptr, type, member) ({ \
+ const typeof(((type *)0)->member) *__mptr = (ptr); \
+ (type *)((char *)__mptr - offsetof(type, member)); })
+
+#define LIST_POISON1 ((void *) 0x00100100)
+#define LIST_POISON2 ((void *) 0x00200200)
+
+struct list_head {
+ struct list_head *next, *prev;
+};
+
+#define LIST_HEAD_INIT(name) { &(name), &(name) }
+
+#define LIST_HEAD(name) \
+ struct list_head name = LIST_HEAD_INIT(name)
+
+static inline void INIT_LIST_HEAD(struct list_head *list)
+{
+ list->next = list;
+ list->prev = list;
+}
+
+static inline void __list_add(struct list_head *new,
+ struct list_head *prev,
+ struct list_head *next)
+{
+ next->prev = new;
+ new->next = next;
+ new->prev = prev;
+ prev->next = new;
+}
+
+static inline void list_add(struct list_head *new, struct list_head *head)
+{
+ __list_add(new, head, head->next);
+}
+
+static inline void list_add_tail(struct list_head *new, struct list_head *head)
+{
+ __list_add(new, head->prev, head);
+}
+
+static inline void __list_del(struct list_head *prev, struct list_head *next)
+{
+ next->prev = prev;
+ prev->next = next;
+}
+
+static inline void __list_del_entry(struct list_head *entry)
+{
+ __list_del(entry->prev, entry->next);
+}
+
+static inline void list_del(struct list_head *entry)
+{
+ __list_del(entry->prev, entry->next);
+ entry->next = LIST_POISON1;
+ entry->prev = LIST_POISON2;
+}
+
+static inline void list_replace(struct list_head *old,
+ struct list_head *new)
+{
+ new->next = old->next;
+ new->next->prev = new;
+ new->prev = old->prev;
+ new->prev->next = new;
+}
+
+static inline void list_replace_init(struct list_head *old,
+ struct list_head *new)
+{
+ list_replace(old, new);
+ INIT_LIST_HEAD(old);
+}
+
+static inline void list_del_init(struct list_head *entry)
+{
+ __list_del_entry(entry);
+ INIT_LIST_HEAD(entry);
+}
+
+static inline void list_move(struct list_head *list, struct list_head *head)
+{
+ __list_del_entry(list);
+ list_add(list, head);
+}
+
+static inline void list_move_tail(struct list_head *list,
+ struct list_head *head)
+{
+ __list_del_entry(list);
+ list_add_tail(list, head);
+}
+
+static inline int list_is_last(const struct list_head *list,
+ const struct list_head *head)
+{
+ return list->next == head;
+}
+
+static inline int list_empty(const struct list_head *head)
+{
+ return head->next == head;
+}
+
+static inline int list_empty_careful(const struct list_head *head)
+{
+ struct list_head *next = head->next;
+
+ return (next == head) && (next == head->prev);
+}
+
+static inline void list_rotate_left(struct list_head *head)
+{
+ struct list_head *first;
+
+ if (!list_empty(head)) {
+ first = head->next;
+ list_move_tail(first, head);
+ }
+}
+
+static inline int list_is_singular(const struct list_head *head)
+{
+ return !list_empty(head) && (head->next == head->prev);
+}
+
+#define list_entry(ptr, type, member) \
+ container_of(ptr, type, member)
+
+#define list_first_entry(ptr, type, member) \
+ list_entry((ptr)->next, type, member)
+
+#define list_last_entry(ptr, type, member) \
+ list_entry((ptr)->prev, type, member)
+
+#define list_first_entry_or_null(ptr, type, member) \
+ (!list_empty(ptr) ? list_first_entry(ptr, type, member) : NULL)
+
+#define list_next_entry(pos, member) \
+ list_entry((pos)->member.next, typeof(*(pos)), member)
+
+#define list_prev_entry(pos, member) \
+ list_entry((pos)->member.prev, typeof(*(pos)), member)
+
+#define list_for_each(pos, head) \
+ for (pos = (head)->next; pos != (head); pos = pos->next)
+
+#define list_for_each_prev(pos, head) \
+ for (pos = (head)->prev; pos != (head); pos = pos->prev)
+
+#define list_for_each_safe(pos, n, head) \
+ for (pos = (head)->next, n = pos->next; pos != (head); \
+ pos = n, n = pos->next)
+
+#define list_for_each_prev_safe(pos, n, head) \
+ for (pos = (head)->prev, n = pos->prev; \
+ pos != (head); \
+ pos = n, n = pos->prev)
+
+#define list_for_each_entry(pos, head, member) \
+ for (pos = list_first_entry(head, typeof(*pos), member); \
+ &pos->member != (head); \
+ pos = list_next_entry(pos, member))
+
+#define list_for_each_entry_reverse(pos, head, member) \
+ for (pos = list_last_entry(head, typeof(*pos), member); \
+ &pos->member != (head); \
+ pos = list_prev_entry(pos, member))
+
+#define list_prepare_entry(pos, head, member) \
+ ((pos) ? : list_entry(head, typeof(*pos), member))
+
+#define list_for_each_entry_continue(pos, head, member) \
+ for (pos = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = list_next_entry(pos, member))
+
+#define list_for_each_entry_continue_reverse(pos, head, member) \
+ for (pos = list_prev_entry(pos, member); \
+ &pos->member != (head); \
+ pos = list_prev_entry(pos, member))
+
+#define list_for_each_entry_from(pos, head, member) \
+ for (; &pos->member != (head); \
+ pos = list_next_entry(pos, member))
+
+#define list_for_each_entry_safe(pos, n, head, member) \
+ for (pos = list_first_entry(head, typeof(*pos), member), \
+ n = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_next_entry(n, member))
+
+#define list_for_each_entry_safe_continue(pos, n, head, member) \
+ for (pos = list_next_entry(pos, member), \
+ n = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_next_entry(n, member))
+
+#define list_for_each_entry_safe_from(pos, n, head, member) \
+ for (n = list_next_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_next_entry(n, member))
+
+#define list_for_each_entry_safe_reverse(pos, n, head, member) \
+ for (pos = list_last_entry(head, typeof(*pos), member), \
+ n = list_prev_entry(pos, member); \
+ &pos->member != (head); \
+ pos = n, n = list_prev_entry(n, member))
+
+#endif /* _LIST_H */
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/entry/entry_64_compat.o: native_usergs_sysret32(): unsupported fallthrough at end of function
asmvalidate: arch/x86/entry/entry_64_compat.o: entry_SYSENTER_compat()+0xcf: unsupported jump to outside of function
asmvalidate: arch/x86/entry/entry_64_compat.o: entry_SYSENTER_compat()+0x113: unsupported jump to outside of function
asmvalidate: arch/x86/entry/entry_64_compat.o: entry_SYSENTER_compat()+0x16d: unsupported jump to outside of function
asmvalidate: arch/x86/entry/entry_64_compat.o: entry_SYSENTER_compat(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/entry/entry_64_compat.o: .entry.text+0x56e: return instruction outside of a function
1. native_usergs_sysret32 is redirected to from a jump rather than a
call, so it shouldn't be annotated as a function. Change ENDPROC ->
END accordingly.
2. Ditto for entry_SYSENTER_compat.
3. The stub functions can be called, so annotate them as functions with
ENTRY/ENDPROC.
4. The stub functions aren't leaf functions, so save/restore the frame
pointer with FP_SAVE/RESTORE.
5. The stub functions all jump outside of their respective functions'
boundaries to the ia32_ptregs_common label. Change them to be
self-contained so they stay within their boundaries.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/entry/entry_64_compat.S | 35 +++++++++++++++++++----------------
arch/x86/include/asm/func.h | 6 ++++++
2 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index bb187a6..07f5ae8 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -13,6 +13,7 @@
#include <asm/irqflags.h>
#include <asm/asm.h>
#include <asm/smap.h>
+#include <asm/func.h>
#include <linux/linkage.h>
#include <linux/err.h>
@@ -32,7 +33,7 @@
ENTRY(native_usergs_sysret32)
swapgs
sysretl
-ENDPROC(native_usergs_sysret32)
+END(native_usergs_sysret32)
#endif
/*
@@ -270,7 +271,7 @@ sysenter_tracesys:
RESTORE_EXTRA_REGS
jmp sysenter_do_call
-ENDPROC(entry_SYSENTER_compat)
+END(entry_SYSENTER_compat)
/*
* 32-bit SYSCALL instruction entry.
@@ -523,10 +524,15 @@ ia32_tracesys:
END(entry_INT80_compat)
.macro PTREGSCALL label, func
- ALIGN
-GLOBAL(\label)
- leaq \func(%rip), %rax
- jmp ia32_ptregs_common
+ENTRY(\label)
+ FP_SAVE
+ leaq \func(%rip),%rax
+ SAVE_EXTRA_REGS(8+FP_SIZE)
+ call *%rax
+ RESTORE_EXTRA_REGS(8+FP_SIZE)
+ FP_RESTORE
+ ret
+ENDPROC(\label)
.endm
PTREGSCALL stub32_rt_sigreturn, sys32_rt_sigreturn
@@ -534,9 +540,9 @@ GLOBAL(\label)
PTREGSCALL stub32_fork, sys_fork
PTREGSCALL stub32_vfork, sys_vfork
- ALIGN
-GLOBAL(stub32_clone)
- leaq sys_clone(%rip), %rax
+ENTRY(stub32_clone)
+ FP_SAVE
+ leaq sys_clone(%rip),%rax
/*
* The 32-bit clone ABI is: clone(..., int tls_val, int *child_tidptr).
* The 64-bit clone ABI is: clone(..., int *child_tidptr, int tls_val).
@@ -545,12 +551,9 @@ GLOBAL(stub32_clone)
* so we need to swap arguments here before calling it:
*/
xchg %r8, %rcx
- jmp ia32_ptregs_common
-
- ALIGN
-ia32_ptregs_common:
- SAVE_EXTRA_REGS 8
+ SAVE_EXTRA_REGS(8+FP_SIZE)
call *%rax
- RESTORE_EXTRA_REGS 8
+ RESTORE_EXTRA_REGS(8+FP_SIZE)
+ FP_RESTORE
ret
-END(ia32_ptregs_common)
+ENDPROC(stub32_clone)
diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
index 52b3225..1d923bd 100644
--- a/arch/x86/include/asm/func.h
+++ b/arch/x86/include/asm/func.h
@@ -37,6 +37,12 @@
.endif
.endm
+#ifdef CONFIG_FRAME_POINTER
+#define FP_SIZE __ASM_SEL(4, 8)
+#else
+#define FP_SIZE 0
+#endif
+
/*
* This macro tells the asm validation script to ignore the instruction
* immediately after the macro. It should only be used in special cases where
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_set_key(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_enc(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_dec(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_ecb_enc(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_ecb_dec(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_cbc_enc(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_cbc_dec(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_ctr_enc(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/aesni-intel_asm.o: aesni_xts_crypt8(): missing FP_SAVE/RESTORE macros
These are all non-leaf callable functions, so save/restore the frame
pointer with FP_SAVE/RESTORE.
Signed-off-by: Josh Poimboeuf <[email protected]>
Cc: [email protected]
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
---
arch/x86/crypto/aesni-intel_asm.S | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S
index 6bd2c6c..83465f9a 100644
--- a/arch/x86/crypto/aesni-intel_asm.S
+++ b/arch/x86/crypto/aesni-intel_asm.S
@@ -31,6 +31,7 @@
#include <linux/linkage.h>
#include <asm/inst.h>
+#include <asm/func.h>
/*
* The following macros are used to move an (un)aligned 16 byte value to/from
@@ -1800,6 +1801,7 @@ ENDPROC(_key_expansion_256b)
* unsigned int key_len)
*/
ENTRY(aesni_set_key)
+ FP_SAVE
#ifndef __x86_64__
pushl KEYP
movl 8(%esp), KEYP # ctx
@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
#ifndef __x86_64__
popl KEYP
#endif
+ FP_RESTORE
ret
ENDPROC(aesni_set_key)
@@ -1912,6 +1915,7 @@ ENDPROC(aesni_set_key)
* void aesni_enc(struct crypto_aes_ctx *ctx, u8 *dst, const u8 *src)
*/
ENTRY(aesni_enc)
+ FP_SAVE
#ifndef __x86_64__
pushl KEYP
pushl KLEN
@@ -1927,6 +1931,7 @@ ENTRY(aesni_enc)
popl KLEN
popl KEYP
#endif
+ FP_RESTORE
ret
ENDPROC(aesni_enc)
@@ -2101,6 +2106,7 @@ ENDPROC(_aesni_enc4)
* void aesni_dec (struct crypto_aes_ctx *ctx, u8 *dst, const u8 *src)
*/
ENTRY(aesni_dec)
+ FP_SAVE
#ifndef __x86_64__
pushl KEYP
pushl KLEN
@@ -2117,6 +2123,7 @@ ENTRY(aesni_dec)
popl KLEN
popl KEYP
#endif
+ FP_RESTORE
ret
ENDPROC(aesni_dec)
@@ -2292,6 +2299,7 @@ ENDPROC(_aesni_dec4)
* size_t len)
*/
ENTRY(aesni_ecb_enc)
+ FP_SAVE
#ifndef __x86_64__
pushl LEN
pushl KEYP
@@ -2342,6 +2350,7 @@ ENTRY(aesni_ecb_enc)
popl KEYP
popl LEN
#endif
+ FP_RESTORE
ret
ENDPROC(aesni_ecb_enc)
@@ -2350,6 +2359,7 @@ ENDPROC(aesni_ecb_enc)
* size_t len);
*/
ENTRY(aesni_ecb_dec)
+ FP_SAVE
#ifndef __x86_64__
pushl LEN
pushl KEYP
@@ -2401,6 +2411,7 @@ ENTRY(aesni_ecb_dec)
popl KEYP
popl LEN
#endif
+ FP_RESTORE
ret
ENDPROC(aesni_ecb_dec)
@@ -2409,6 +2420,7 @@ ENDPROC(aesni_ecb_dec)
* size_t len, u8 *iv)
*/
ENTRY(aesni_cbc_enc)
+ FP_SAVE
#ifndef __x86_64__
pushl IVP
pushl LEN
@@ -2443,6 +2455,7 @@ ENTRY(aesni_cbc_enc)
popl LEN
popl IVP
#endif
+ FP_RESTORE
ret
ENDPROC(aesni_cbc_enc)
@@ -2451,6 +2464,7 @@ ENDPROC(aesni_cbc_enc)
* size_t len, u8 *iv)
*/
ENTRY(aesni_cbc_dec)
+ FP_SAVE
#ifndef __x86_64__
pushl IVP
pushl LEN
@@ -2534,6 +2548,7 @@ ENTRY(aesni_cbc_dec)
popl LEN
popl IVP
#endif
+ FP_RESTORE
ret
ENDPROC(aesni_cbc_dec)
@@ -2598,6 +2613,7 @@ ENDPROC(_aesni_inc)
* size_t len, u8 *iv)
*/
ENTRY(aesni_ctr_enc)
+ FP_SAVE
cmp $16, LEN
jb .Lctr_enc_just_ret
mov 480(KEYP), KLEN
@@ -2651,6 +2667,7 @@ ENTRY(aesni_ctr_enc)
.Lctr_enc_ret:
movups IV, (IVP)
.Lctr_enc_just_ret:
+ FP_RESTORE
ret
ENDPROC(aesni_ctr_enc)
@@ -2677,6 +2694,7 @@ ENDPROC(aesni_ctr_enc)
* bool enc, u8 *iv)
*/
ENTRY(aesni_xts_crypt8)
+ FP_SAVE
cmpb $0, %cl
movl $0, %ecx
movl $240, %r10d
@@ -2777,6 +2795,7 @@ ENTRY(aesni_xts_crypt8)
pxor INC, STATE4
movdqu STATE4, 0x70(OUTP)
+ FP_RESTORE
ret
ENDPROC(aesni_xts_crypt8)
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/crypto/ghash-clmulni-intel_asm.o: clmul_ghash_mul(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/ghash-clmulni-intel_asm.o: clmul_ghash_update(): missing FP_SAVE/RESTORE macros
These are non-leaf callable functions, so save/restore the frame pointer
with FP_SAVE/RESTORE.
Signed-off-by: Josh Poimboeuf <[email protected]>
Cc: [email protected]
Cc: Herbert Xu <[email protected]>
Cc: "David S. Miller" <[email protected]>
---
arch/x86/crypto/ghash-clmulni-intel_asm.S | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/x86/crypto/ghash-clmulni-intel_asm.S b/arch/x86/crypto/ghash-clmulni-intel_asm.S
index 5d1e007..e5b76b1 100644
--- a/arch/x86/crypto/ghash-clmulni-intel_asm.S
+++ b/arch/x86/crypto/ghash-clmulni-intel_asm.S
@@ -18,6 +18,7 @@
#include <linux/linkage.h>
#include <asm/inst.h>
+#include <asm/func.h>
.data
@@ -94,6 +95,7 @@ ENDPROC(__clmul_gf128mul_ble)
/* void clmul_ghash_mul(char *dst, const u128 *shash) */
ENTRY(clmul_ghash_mul)
+ FP_SAVE
movups (%rdi), DATA
movups (%rsi), SHASH
movaps .Lbswap_mask, BSWAP
@@ -101,6 +103,7 @@ ENTRY(clmul_ghash_mul)
call __clmul_gf128mul_ble
PSHUFB_XMM BSWAP DATA
movups DATA, (%rdi)
+ FP_RESTORE
ret
ENDPROC(clmul_ghash_mul)
@@ -109,6 +112,7 @@ ENDPROC(clmul_ghash_mul)
* const u128 *shash);
*/
ENTRY(clmul_ghash_update)
+ FP_SAVE
cmp $16, %rdx
jb .Lupdate_just_ret # check length
movaps .Lbswap_mask, BSWAP
@@ -128,5 +132,6 @@ ENTRY(clmul_ghash_update)
PSHUFB_XMM BSWAP DATA
movups DATA, (%rdi)
.Lupdate_just_ret:
+ FP_RESTORE
ret
ENDPROC(clmul_ghash_update)
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/platform/efi/efi_stub_64.o: efi_call(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/boot/compressed/efi_stub_64.o: efi_call(): missing FP_SAVE/RESTORE macros
efi_call() is a non-leaf callable function, so save/restore the frame
pointer with FP_SAVE/RESTORE.
Signed-off-by: Josh Poimboeuf <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: [email protected]
---
arch/x86/platform/efi/efi_stub_64.S | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
index 86d0f9e..0ca6bfb 100644
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -11,6 +11,7 @@
#include <asm/msr.h>
#include <asm/processor-flags.h>
#include <asm/page_types.h>
+#include <asm/func.h>
#define SAVE_XMM \
mov %rsp, %rax; \
@@ -74,6 +75,7 @@
.endm
ENTRY(efi_call)
+ FP_SAVE
SAVE_XMM
mov (%rsp), %rax
mov 8(%rax), %rax
@@ -88,6 +90,7 @@ ENTRY(efi_call)
RESTORE_PGT
addq $48, %rsp
RESTORE_XMM
+ FP_RESTORE
ret
ENDPROC(efi_call)
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x15: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x55: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x9a: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x116: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): missing FP_SAVE/RESTORE macros
1. wakeup_long64() isn't a function that can be called. It's actually
redirected to via a return instruction in the entry code. It
shouldn't be annotated as a callable function. Change ENDPROC ->
PROC accordingly.
2. do_suspend_lowlevel() is a non-leaf callable function, so
save/restore the frame pointer with FP_SAVE/RESTORE.
3. Remove the unnecessary jump to .Lresume_point, as it just results in
jumping to the next instruction (which is a nop because of the
align). Otherwise asmvalidate gets confused by the jump.
4. Change the "jmp restore_processor_state" to a call instruction,
because jumping outside the function's boundaries isn't allowed. Now
restore_processor_state() will return back to do_suspend_lowlevel()
instead of do_suspend_lowlevel()'s caller.
5. Remove superfluous rsp changes.
Signed-off-by: Josh Poimboeuf <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: [email protected]
---
arch/x86/kernel/acpi/wakeup_64.S | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
index 8c35df4..7e442be 100644
--- a/arch/x86/kernel/acpi/wakeup_64.S
+++ b/arch/x86/kernel/acpi/wakeup_64.S
@@ -5,6 +5,7 @@
#include <asm/page_types.h>
#include <asm/msr.h>
#include <asm/asm-offsets.h>
+#include <asm/func.h>
# Copyright 2003 Pavel Machek <[email protected]>, distribute under GPLv2
@@ -33,13 +34,13 @@ ENTRY(wakeup_long64)
movq saved_rip, %rax
jmp *%rax
-ENDPROC(wakeup_long64)
+END(wakeup_long64)
bogus_64_magic:
jmp bogus_64_magic
ENTRY(do_suspend_lowlevel)
- subq $8, %rsp
+ FP_SAVE
xorl %eax, %eax
call save_processor_state
@@ -70,12 +71,11 @@ ENTRY(do_suspend_lowlevel)
movq %rdi, saved_rdi
movq %rsi, saved_rsi
- addq $8, %rsp
movl $3, %edi
xorl %eax, %eax
call x86_acpi_enter_sleep_state
+
/* in case something went wrong, restore the machine status and go on */
- jmp .Lresume_point
.align 4
.Lresume_point:
@@ -108,8 +108,9 @@ ENTRY(do_suspend_lowlevel)
movq pt_regs_r15(%rax), %r15
xorl %eax, %eax
- addq $8, %rsp
- jmp restore_processor_state
+ call restore_processor_state
+ FP_RESTORE
+ ret
ENDPROC(do_suspend_lowlevel)
.data
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/kernel/head_64.o: start_cpu0(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x4: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xd: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x16: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x1f: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x28: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x31: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x3a: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x43: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x4a: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x55: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x5c: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x65: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x6e: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x77: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x80: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x8b: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x94: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x9b: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xa6: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xaf: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xb8: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xc1: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xca: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xd3: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xdc: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xe5: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xee: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0xf7: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x100: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x109: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x112: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array()+0x11b: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_array(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_common()+0xbb: unsupported jump to outside of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_common(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: early_idt_handler_common(): missing FP_SAVE/RESTORE macros
1. start_cpu0() isn't a normal callable function because it doesn't
return to its caller. Change ENDPROC -> END accordingly.
2. early_idt_handler_array() and early_idt_handler_common() aren't
callable functions. Change ENDPROC -> END accordingly.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/kernel/head_64.S | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index e5c27f7..8ba22cf 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -20,6 +20,7 @@
#include <asm/processor-flags.h>
#include <asm/percpu.h>
#include <asm/nops.h>
+#include <asm/func.h>
#ifdef CONFIG_PARAVIRT
#include <asm/asm-offsets.h>
@@ -301,7 +302,7 @@ ENTRY(start_cpu0)
pushq $__KERNEL_CS # set correct cs
pushq %rax # target address in negative space
lretq
-ENDPROC(start_cpu0)
+END(start_cpu0)
#endif
/* SMP bootup changes these two */
@@ -336,7 +337,7 @@ ENTRY(early_idt_handler_array)
i = i + 1
.fill early_idt_handler_array + i*EARLY_IDT_HANDLER_SIZE - ., 1, 0xcc
.endr
-ENDPROC(early_idt_handler_array)
+END(early_idt_handler_array)
early_idt_handler_common:
/*
@@ -414,7 +415,7 @@ early_idt_handler_common:
.Lis_nmi:
addq $16,%rsp # drop vector number and error code
INTERRUPT_RETURN
-ENDPROC(early_idt_handler_common)
+END(early_idt_handler_common)
__INITDATA
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/lib/clear_page_64.o: clear_page()+0x0: unsupported jump to outside of function
asmvalidate: arch/x86/lib/clear_page_64.o: alternative jump to outside the scope of original function clear_page
asmvalidate: arch/x86/lib/copy_page_64.o: copy_page()+0x0: unsupported jump to outside of function
asmvalidate: arch/x86/lib/memcpy_64.o: memcpy()+0x0: unsupported jump to outside of function
asmvalidate: arch/x86/lib/memcpy_64.o: __memcpy()+0x0: unsupported jump to outside of function
asmvalidate: arch/x86/lib/memcpy_64.o: alternative jump to outside the scope of original function memcpy
asmvalidate: arch/x86/lib/memset_64.o: memset()+0x0: unsupported jump to outside of function
asmvalidate: arch/x86/lib/memset_64.o: __memset()+0x0: unsupported jump to outside of function
asmvalidate: arch/x86/lib/memset_64.o: alternative jump to outside the scope of original function memset
Change the annotations for clear_page(), copy_page(), memcpy(), and
memset() so that they don't jump outside of their function boundaries.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/lib/clear_page_64.S | 9 +++------
arch/x86/lib/copy_page_64.S | 5 ++---
arch/x86/lib/memcpy_64.S | 10 ++++------
arch/x86/lib/memset_64.S | 10 ++++------
4 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
index a2fe51b..c342566 100644
--- a/arch/x86/lib/clear_page_64.S
+++ b/arch/x86/lib/clear_page_64.S
@@ -22,10 +22,8 @@ ENTRY(clear_page)
xorl %eax,%eax
rep stosq
ret
-ENDPROC(clear_page)
-
-ENTRY(clear_page_orig)
+clear_page_orig:
xorl %eax,%eax
movl $4096/64,%ecx
.p2align 4
@@ -44,11 +42,10 @@ ENTRY(clear_page_orig)
jnz .Lloop
nop
ret
-ENDPROC(clear_page_orig)
-ENTRY(clear_page_c_e)
+clear_page_c_e:
movl $4096,%ecx
xorl %eax,%eax
rep stosb
ret
-ENDPROC(clear_page_c_e)
+ENDPROC(clear_page)
diff --git a/arch/x86/lib/copy_page_64.S b/arch/x86/lib/copy_page_64.S
index 009f982..81d5cba 100644
--- a/arch/x86/lib/copy_page_64.S
+++ b/arch/x86/lib/copy_page_64.S
@@ -16,9 +16,8 @@ ENTRY(copy_page)
movl $4096/8, %ecx
rep movsq
ret
-ENDPROC(copy_page)
-ENTRY(copy_page_regs)
+copy_page_regs:
subq $2*8, %rsp
movq %rbx, (%rsp)
movq %r12, 1*8(%rsp)
@@ -83,4 +82,4 @@ ENTRY(copy_page_regs)
movq 1*8(%rsp), %r12
addq $2*8, %rsp
ret
-ENDPROC(copy_page_regs)
+ENDPROC(copy_page)
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 16698bb..64d00ec 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -37,21 +37,18 @@ ENTRY(memcpy)
movl %edx, %ecx
rep movsb
ret
-ENDPROC(memcpy)
-ENDPROC(__memcpy)
/*
* memcpy_erms() - enhanced fast string memcpy. This is faster and
* simpler than memcpy. Use memcpy_erms when possible.
*/
-ENTRY(memcpy_erms)
+memcpy_erms:
movq %rdi, %rax
movq %rdx, %rcx
rep movsb
ret
-ENDPROC(memcpy_erms)
-ENTRY(memcpy_orig)
+memcpy_orig:
movq %rdi, %rax
cmpq $0x20, %rdx
@@ -176,4 +173,5 @@ ENTRY(memcpy_orig)
.Lend:
retq
-ENDPROC(memcpy_orig)
+ENDPROC(memcpy)
+ENDPROC(__memcpy)
diff --git a/arch/x86/lib/memset_64.S b/arch/x86/lib/memset_64.S
index 2661fad..a0d9f3f 100644
--- a/arch/x86/lib/memset_64.S
+++ b/arch/x86/lib/memset_64.S
@@ -41,8 +41,6 @@ ENTRY(__memset)
rep stosb
movq %r9,%rax
ret
-ENDPROC(memset)
-ENDPROC(__memset)
/*
* ISO C memset - set a memory block to a byte value. This function uses
@@ -55,16 +53,15 @@ ENDPROC(__memset)
*
* rax original destination
*/
-ENTRY(memset_erms)
+memset_erms:
movq %rdi,%r9
movb %sil,%al
movq %rdx,%rcx
rep stosb
movq %r9,%rax
ret
-ENDPROC(memset_erms)
-ENTRY(memset_orig)
+memset_orig:
movq %rdi,%r10
/* expand byte value */
@@ -135,4 +132,5 @@ ENTRY(memset_orig)
subq %r8,%rdx
jmp .Lafter_bad_alignment
.Lfinal:
-ENDPROC(memset_orig)
+ENDPROC(memset)
+ENDPROC(__memset)
--
2.1.0
Fix the following asmvalidate warnings:
asmvalidate: arch/x86/lib/rwsem.o: call_rwsem_down_read_failed(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/lib/rwsem.o: call_rwsem_down_write_failed(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/lib/rwsem.o: call_rwsem_wake(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/lib/rwsem.o: call_rwsem_downgrade_wake(): missing FP_SAVE/RESTORE macros
These are callable non-leaf functions, so save/restore the frame pointer
with FP_SAVE/RESTORE.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/lib/rwsem.S | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
index 40027db..709bc9d 100644
--- a/arch/x86/lib/rwsem.S
+++ b/arch/x86/lib/rwsem.S
@@ -15,6 +15,7 @@
#include <linux/linkage.h>
#include <asm/alternative-asm.h>
+#include <asm/func.h>
#define __ASM_HALF_REG(reg) __ASM_SEL(reg, e##reg)
#define __ASM_HALF_SIZE(inst) __ASM_SEL(inst##w, inst##l)
@@ -84,24 +85,29 @@
/* Fix up special calling conventions */
ENTRY(call_rwsem_down_read_failed)
+ FP_SAVE
save_common_regs
__ASM_SIZE(push,) %__ASM_REG(dx)
movq %rax,%rdi
call rwsem_down_read_failed
__ASM_SIZE(pop,) %__ASM_REG(dx)
restore_common_regs
+ FP_RESTORE
ret
ENDPROC(call_rwsem_down_read_failed)
ENTRY(call_rwsem_down_write_failed)
+ FP_SAVE
save_common_regs
movq %rax,%rdi
call rwsem_down_write_failed
restore_common_regs
+ FP_RESTORE
ret
ENDPROC(call_rwsem_down_write_failed)
ENTRY(call_rwsem_wake)
+ FP_SAVE
/* do nothing if still outstanding active readers */
__ASM_HALF_SIZE(dec) %__ASM_HALF_REG(dx)
jnz 1f
@@ -109,15 +115,18 @@ ENTRY(call_rwsem_wake)
movq %rax,%rdi
call rwsem_wake
restore_common_regs
-1: ret
+1: FP_RESTORE
+ ret
ENDPROC(call_rwsem_wake)
ENTRY(call_rwsem_downgrade_wake)
+ FP_SAVE
save_common_regs
__ASM_SIZE(push,) %__ASM_REG(dx)
movq %rax,%rdi
call rwsem_downgrade_wake
__ASM_SIZE(pop,) %__ASM_REG(dx)
restore_common_regs
+ FP_RESTORE
ret
ENDPROC(call_rwsem_downgrade_wake)
--
2.1.0
On Wed, Jun 10, 2015 at 07:06:08AM -0500, Josh Poimboeuf wrote:
> There are still a lot of outstanding warnings (which I'll paste as a
> reply to this email). Once those are all cleaned up, we can change the
> warnings to build errors and change the default to
> CONFIG_ASM_VALIDATION=y so the asm code stays clean.
Here are the 194 outstanding warnings I'm seeing with my Fedora kernel
config. I'll keep chipping away at them.
asmvalidate: arch/x86/crypto/crc32c-pcl-intel-asm_64.o: crc_pcl()+0x84: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/crc32c-pcl-intel-asm_64.o: crc_pcl(): unsupported fallthrough at end of function
asmvalidate: arch/x86/crypto/sha1_avx2_x86_64_asm.o: sha1_transform_avx2()+0x645: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/sha1_avx2_x86_64_asm.o: sha1_transform_avx2()+0x1418: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/sha1_avx2_x86_64_asm.o: sha1_transform_avx2()+0x16e4: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/sha1_avx2_x86_64_asm.o: sha1_transform_avx2()+0x1a22: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: __camellia_enc_blk16(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: __camellia_dec_blk16(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_ecb_enc_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_ecb_dec_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_cbc_dec_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_ctr_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_xts_crypt_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_xts_enc_16way()+0xb: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_xts_enc_16way(): unsupported fallthrough at end of function
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_xts_dec_16way()+0x1e: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/camellia-aesni-avx-asm_64.o: camellia_xts_dec_16way(): unsupported fallthrough at end of function
asmvalidate: arch/x86/crypto/cast5-avx-x86_64-asm_64.o: cast5_ecb_enc_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast5-avx-x86_64-asm_64.o: cast5_ecb_dec_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast5-avx-x86_64-asm_64.o: cast5_cbc_dec_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast5-avx-x86_64-asm_64.o: cast5_ctr_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast6-avx-x86_64-asm_64.o: cast6_ecb_enc_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast6-avx-x86_64-asm_64.o: cast6_ecb_dec_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast6-avx-x86_64-asm_64.o: cast6_cbc_dec_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast6-avx-x86_64-asm_64.o: cast6_ctr_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast6-avx-x86_64-asm_64.o: cast6_xts_enc_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/cast6-avx-x86_64-asm_64.o: cast6_xts_dec_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/twofish-avx-x86_64-asm_64.o: twofish_ecb_enc_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/twofish-avx-x86_64-asm_64.o: twofish_ecb_dec_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/twofish-avx-x86_64-asm_64.o: twofish_cbc_dec_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/twofish-avx-x86_64-asm_64.o: twofish_ctr_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/twofish-avx-x86_64-asm_64.o: twofish_xts_enc_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/twofish-avx-x86_64-asm_64.o: twofish_xts_dec_8way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx-x86_64-asm_64.o: serpent_ecb_enc_8way_avx(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx-x86_64-asm_64.o: serpent_ecb_dec_8way_avx(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx-x86_64-asm_64.o: serpent_cbc_dec_8way_avx(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx-x86_64-asm_64.o: serpent_ctr_8way_avx(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx-x86_64-asm_64.o: serpent_xts_enc_8way_avx(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx-x86_64-asm_64.o: serpent_xts_dec_8way_avx(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: __camellia_enc_blk32(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: __camellia_dec_blk32(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_ecb_enc_32way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_ecb_dec_32way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_cbc_dec_32way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_ctr_32way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_xts_crypt_32way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_xts_enc_32way()+0xb: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_xts_enc_32way(): unsupported fallthrough at end of function
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_xts_dec_32way()+0x1e: unsupported jump to outside of function
asmvalidate: arch/x86/crypto/camellia-aesni-avx2-asm_64.o: camellia_xts_dec_32way(): unsupported fallthrough at end of function
asmvalidate: arch/x86/crypto/serpent-avx2-asm_64.o: serpent_ecb_enc_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx2-asm_64.o: serpent_ecb_dec_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx2-asm_64.o: serpent_cbc_dec_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx2-asm_64.o: serpent_ctr_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx2-asm_64.o: serpent_xts_enc_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/crypto/serpent-avx2-asm_64.o: serpent_xts_dec_16way(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/entry/entry_64.o: native_usergs_sysret64(): unsupported fallthrough at end of function
asmvalidate: arch/x86/entry/entry_64.o: .entry.text+0x399: return instruction outside of a function
asmvalidate: arch/x86/entry/entry_64.o: .entry.text+0x1ba9: return instruction outside of a function
asmvalidate: arch/x86/entry/entry_64.o: .entry.text+0x1bd5: return instruction outside of a function
asmvalidate: arch/x86/entry/entry_64.o: .entry.text+0x20e4: return instruction outside of a function
asmvalidate: arch/x86/entry/entry_64.o: .entry.text+0x21be: return instruction outside of a function
asmvalidate: arch/x86/entry/vdso/vdso32/int80.o: __kernel_sigreturn(): unsupported fallthrough at end of function
asmvalidate: arch/x86/entry/vdso/vdso32/int80.o: __kernel_rt_sigreturn(): unsupported fallthrough at end of function
asmvalidate: arch/x86/entry/vdso/vdso32/syscall.o: __kernel_sigreturn(): unsupported fallthrough at end of function
asmvalidate: arch/x86/entry/vdso/vdso32/syscall.o: __kernel_rt_sigreturn(): unsupported fallthrough at end of function
asmvalidate: arch/x86/entry/vdso/vdso32/sysenter.o: __kernel_sigreturn(): unsupported fallthrough at end of function
asmvalidate: arch/x86/entry/vdso/vdso32/sysenter.o: __kernel_rt_sigreturn(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/mcount_64.o: .entry.text+0x0: return instruction outside of a function
asmvalidate: arch/x86/kernel/mcount_64.o: .entry.text+0xbb: return instruction outside of a function
asmvalidate: arch/x86/kernel/mcount_64.o: .entry.text+0x2b7: return instruction outside of a function
asmvalidate: arch/x86/kernel/relocate_kernel_64.o: .text+0x6b: return instruction outside of a function
asmvalidate: arch/x86/kernel/relocate_kernel_64.o: .text+0xc7: return instruction outside of a function
asmvalidate: arch/x86/kernel/relocate_kernel_64.o: .text+0x110: return instruction outside of a function
asmvalidate: arch/x86/kernel/relocate_kernel_64.o: .text+0x145: return instruction outside of a function
asmvalidate: arch/x86/kernel/relocate_kernel_64.o: .text+0x1c4: return instruction outside of a function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_set_trap_table(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_mmu_update(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_set_gdt(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_stack_switch(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_set_callbacks(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_fpu_taskswitch(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_sched_op_compat(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_dom0_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_set_debugreg(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_get_debugreg(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_update_descriptor(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_memory_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_multicall(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_update_va_mapping(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_set_timer_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_event_channel_op_compat(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_xen_version(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_console_io(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_physdev_op_compat(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_grant_table_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_vm_assist(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_update_va_mapping_otherdomain(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_iret(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_vcpu_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_set_segment_base(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_mmuext_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_xsm_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_nmi_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_sched_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_callback_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_xenoprof_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_event_channel_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_physdev_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_hvm_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_sysctl(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_domctl(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_kexec_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_tmem_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_xc_reserved_op(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_mca(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_arch_1(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_arch_2(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_arch_3(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_arch_4(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_arch_5(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_arch_6(): unsupported fallthrough at end of function
asmvalidate: arch/x86/kernel/head_64.o: xen_hypercall_arch_7(): unsupported fallthrough at end of function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0x18: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0x34: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0x47: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0x74: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0xa7: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0xd3: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0xfa: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0x127: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0x14d: return instruction outside of a function
asmvalidate: arch/x86/net/bpf_jit.o: .text+0x16d: return instruction outside of a function
asmvalidate: arch/x86/platform/efi/efi_thunk_64.o: efi64_thunk(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/platform/efi/efi_thunk_64.o: efi_enter32(): unsupported fallthrough at end of function
asmvalidate: arch/x86/platform/efi/efi_thunk_64.o: efi_enter32(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/xen/xen-asm.o: xen_irq_enable_direct(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/xen/xen-asm.o: xen_restore_fl_direct(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/xen/xen-asm.o: .text+0x7f: return instruction outside of a function
asmvalidate: arch/x86/xen/xen-asm_64.o: .text+0xa: return instruction outside of a function
asmvalidate: arch/x86/xen/xen-asm_64.o: xen_syscall_target()+0xe: unsupported jump to outside of function
asmvalidate: arch/x86/xen/xen-asm_64.o: xen_syscall_target(): unsupported fallthrough at end of function
asmvalidate: arch/x86/xen/xen-asm_64.o: xen_syscall32_target()+0xe: unsupported jump to outside of function
asmvalidate: arch/x86/xen/xen-asm_64.o: xen_syscall32_target(): unsupported fallthrough at end of function
asmvalidate: arch/x86/xen/xen-asm_64.o: xen_sysenter_target()+0xe: unsupported jump to outside of function
asmvalidate: arch/x86/xen/xen-asm_64.o: xen_sysenter_target(): unsupported fallthrough at end of function
asmvalidate: arch/x86/power/hibernate_asm_64.o: .text+0x69: return instruction outside of a function
asmvalidate: arch/x86/power/hibernate_asm_64.o: .text+0x16d: return instruction outside of a function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_to_user()+0x15: unsupported jump to outside of function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_to_user()+0x1f: unsupported jump to outside of function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_to_user()+0x25: unsupported jump to outside of function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_to_user(): unsupported fallthrough at end of function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_from_user()+0x15: unsupported jump to outside of function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_from_user()+0x1f: unsupported jump to outside of function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_from_user()+0x25: unsupported jump to outside of function
asmvalidate: arch/x86/lib/copy_user_64.o: _copy_from_user(): unsupported fallthrough at end of function
asmvalidate: arch/x86/lib/copy_user_64.o: alternative jump to outside the scope of original function _copy_to_user
asmvalidate: arch/x86/lib/copy_user_64.o: alternative jump to outside the scope of original function _copy_to_user
asmvalidate: arch/x86/lib/copy_user_64.o: alternative jump to outside the scope of original function _copy_from_user
asmvalidate: arch/x86/lib/copy_user_64.o: alternative jump to outside the scope of original function _copy_from_user
asmvalidate: arch/x86/lib/csum-copy_64.o: csum_partial_copy_generic()+0x6: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: __get_user_1()+0x14: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: __get_user_2()+0x4: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: __get_user_2()+0x1e: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: __get_user_4()+0x4: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: __get_user_4()+0x1a: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: __get_user_8()+0x4: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: __get_user_8()+0x1a: unsupported jump to outside of function
asmvalidate: arch/x86/lib/getuser.o: .text+0xc5: return instruction outside of a function
asmvalidate: arch/x86/lib/putuser.o: __put_user_1()+0x14: unsupported jump to outside of function
asmvalidate: arch/x86/lib/putuser.o: __put_user_2()+0x1b: unsupported jump to outside of function
asmvalidate: arch/x86/lib/putuser.o: __put_user_4()+0x1b: unsupported jump to outside of function
asmvalidate: arch/x86/lib/putuser.o: __put_user_8()+0x1b: unsupported jump to outside of function
asmvalidate: arch/x86/lib/putuser.o: .text+0xc1: return instruction outside of a function
asmvalidate: arch/x86/boot/copy.o: copy_from_fs(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/boot/copy.o: copy_to_fs(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/boot/compressed/head_64.o: .text+0x16e: return instruction outside of a function
asmvalidate: arch/x86/boot/compressed/head_64.o: .text+0x172: return instruction outside of a function
asmvalidate: arch/x86/boot/compressed/head_64.o: startup_32()+0x38: unsupported jump to outside of function
asmvalidate: arch/x86/boot/compressed/head_64.o: startup_32(): unsupported fallthrough at end of function
asmvalidate: arch/x86/boot/compressed/head_64.o: startup_32(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/boot/compressed/head_64.o: efi32_stub_entry()+0x37: unsupported jump to outside of function
asmvalidate: arch/x86/boot/compressed/head_64.o: efi32_stub_entry(): unsupported fallthrough at end of function
asmvalidate: arch/x86/boot/compressed/head_64.o: efi32_stub_entry(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/boot/compressed/head_64.o: efi64_stub_entry()+0x1f: unsupported jump to outside of function
asmvalidate: arch/x86/boot/compressed/head_64.o: efi64_stub_entry(): unsupported fallthrough at end of function
asmvalidate: arch/x86/boot/compressed/efi_thunk_64.o: efi_enter32(): unsupported fallthrough at end of function
asmvalidate: arch/x86/boot/compressed/efi_thunk_64.o: efi_enter32(): missing FP_SAVE/RESTORE macros
asmvalidate: arch/x86/boot/header.o: die()+0x1: unsupported jump to outside of function
asmvalidate: arch/x86/boot/header.o: die(): unsupported fallthrough at end of function
asmvalidate: arch/x86/boot/pmjump.o: protected_mode_jump()+0x11: unsupported jump to outside of function
asmvalidate: arch/x86/boot/pmjump.o: protected_mode_jump(): unsupported fallthrough at end of function
asmvalidate: arch/x86/boot/pmjump.o: in_pm32()+0x1c: unsupported jump to outside of function
asmvalidate: arch/x86/boot/pmjump.o: in_pm32(): unsupported fallthrough at end of function
--
Josh
> 2. Each callable function must never leave its own bounds (i.e. with a
> jump to outside the function) except when returning.
That prevents a lot of optimizations with out of line code.
In fact even gcc with the right options can generate code that violates
this. Standard Linux constructions, such as exception handling,
also violate this.
If your tool needs that your tool is broken.
BTW any other frame pointer requirement should be also optional,
as it slows down a number of CPUs, such as Atoms.
-Andi
Hi!
> Fix the following asmvalidate warnings:
>
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x15: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x55: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64(): unsupported fallthrough at end of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x9a: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x116: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): unsupported fallthrough at end of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): missing FP_SAVE/RESTORE macros
>
> 1. wakeup_long64() isn't a function that can be called. It's actually
> redirected to via a return instruction in the entry code. It
> shouldn't be annotated as a callable function. Change ENDPROC ->
> PROC accordingly.
But I see -> END.
> 2. do_suspend_lowlevel() is a non-leaf callable function, so
> save/restore the frame pointer with FP_SAVE/RESTORE.
It does not work with the frame pointer itself. Is FP_SAVE/RESTORE
still neccessary? Will you need FP_RESTORE to wakeup_long64, then?
> 3. Remove the unnecessary jump to .Lresume_point, as it just results in
> jumping to the next instruction (which is a nop because of the
> align). Otherwise asmvalidate gets confused by the jump.
It also results in flushing the pipeline. Ok, I guess this one is unneccessary.
> 4. Change the "jmp restore_processor_state" to a call instruction,
> because jumping outside the function's boundaries isn't allowed. Now
> restore_processor_state() will return back to do_suspend_lowlevel()
> instead of do_suspend_lowlevel()'s caller.
>
> 5. Remove superfluous rsp changes.
Did you test the changes?
Do you plan to make similar changes to wakeup_32.S?
> diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
> index 8c35df4..7e442be 100644
> --- a/arch/x86/kernel/acpi/wakeup_64.S
> +++ b/arch/x86/kernel/acpi/wakeup_64.S
> @@ -5,6 +5,7 @@
> #include <asm/page_types.h>
> #include <asm/msr.h>
> #include <asm/asm-offsets.h>
> +#include <asm/func.h>
>
> # Copyright 2003 Pavel Machek <[email protected]>, distribute under GPLv2
>
> @@ -33,13 +34,13 @@ ENTRY(wakeup_long64)
>
> movq saved_rip, %rax
> jmp *%rax
> -ENDPROC(wakeup_long64)
> +END(wakeup_long64)
>
This should result in no binary code changes, so that's ok with me...
> ENTRY(do_suspend_lowlevel)
> - subq $8, %rsp
> + FP_SAVE
> xorl %eax, %eax
> call save_processor_state
>
Are you sure? Stuff like
movq $saved_context, %rax
movq %rsp, pt_regs_sp(%rax)
follows. And you did not modify wakeup_long64, which now receives
different value in saved_rsp.
> @@ -108,8 +108,9 @@ ENTRY(do_suspend_lowlevel)
> movq pt_regs_r15(%rax), %r15
>
> xorl %eax, %eax
> - addq $8, %rsp
> - jmp restore_processor_state
> + call restore_processor_state
> + FP_RESTORE
> + ret
> ENDPROC(do_suspend_lowlevel)
Umm. I rather liked the direct jump.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed 2015-06-10 07:06:15, Josh Poimboeuf wrote:
> Fix the following asmvalidate warnings:
>
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x15: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x55: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64(): unsupported fallthrough at end of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x9a: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x116: unsupported jump to outside of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): unsupported fallthrough at end of function
> asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): missing FP_SAVE/RESTORE macros
>
Actually first things first. Purpose of warnings is to pinpoint
errors. Do you believe there are some errors in wakeup_64.S?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed 2015-06-10 07:06:08, Josh Poimboeuf wrote:
> The previous version of this patch set was named "Compile-time stack
> frame pointer validation". I changed the subject from "frame pointer
> validation" to "asm code validation" because the focus of the patch set
> has changed to be less frame pointer-focused and more asm-focused. I
> also renamed the tool to asmvalidate (it was previously called
> stackvalidate) and basically rewrote most of the code.
>
> The goal of asm validation is to enforce sane rules on asm code: all
> callable asm functions must be self-contained and properly annotated.
>
> Some of the benefits are:
>
> - Frame pointers are more reliable.
>
> - DWARF CFI metadata can be autogenerated (coming soon).
>
> - The asm code becomes less like spaghetti, more like C, and easier to
> comprehend.
>
>
> The asmvalidate tool runs on every compiled .S file, and enforces the
> following rules:
>
> 1. Each callable function must be annotated with the ELF STT_FUNC type.
> This is typically done using the existing ENTRY/ENDPROC macros. If
> asmvalidate finds a return instruction outside of a function, it
> flags an error, since that usually indicates callable code which
> should be annotated accordingly.
>
> 2. Each callable function must never leave its own bounds (i.e. with a
> jump to outside the function) except when returning.
>
> 3. Each callable non-leaf function must have frame pointer logic (if
> required by CONFIG_FRAME_POINTER or the architecture's back chain
> rules). This should by done by the FP_SAVE/FP_RESTORE macros.
>
>
> It currently only supports x86_64, but the code is generic and designed
> for it to be easy to plug in support for other architectures.
>
> There are still a lot of outstanding warnings (which I'll paste as a
> reply to this email). Once those are all cleaned up, we can change the
> warnings to build errors and change the default to
> CONFIG_ASM_VALIDATION=y so the asm code stays clean.
You have interesting definition of "clean".
The reason we sometimes have to use assembly is that it is impossible
to write corresponding code in C, or that performance would be bad.
So... fixing these may have some sense, but I doubt enforcing "you
can't write real assembly" is a good idea.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Jun 10, 2015 at 03:08:14PM +0200, Andi Kleen wrote:
>
> > 2. Each callable function must never leave its own bounds (i.e. with a
> > jump to outside the function) except when returning.
>
> That prevents a lot of optimizations with out of line code.
In most cases there are ways to keep the optimizations. For example:
- grow the function bounds to keep the jump internal
- duplicate the destination code inside the function
- convert the jump to a call
Also note that these rules only affect _callable_ functions, so the
entry code and other non-function asm code can still be a pile of
spaghetti (though I think Andy is working on improving that).
> In fact even gcc with the right options can generate code that violates
> this. Standard Linux constructions, such as exception handling,
> also violate this.
>
> If your tool needs that your tool is broken.
This tool only validates asm code, so I don't see how whatever gcc does
is relevant.
> BTW any other frame pointer requirement should be also optional,
> as it slows down a number of CPUs, such as Atoms.
Yes. This patch set is a first step towards being able to disable frame
pointers. Once all callable functions are reasonably structured, we can
start generating and validating DWARF CFI data. Then we can make a
reliable DWARF unwinder and get rid of frame pointers.
--
Josh
On Wed, Jun 10, 2015 at 03:19:14PM +0200, Pavel Machek wrote:
> Hi!
>
> > Fix the following asmvalidate warnings:
> >
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x15: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x55: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64(): unsupported fallthrough at end of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x9a: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x116: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): unsupported fallthrough at end of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): missing FP_SAVE/RESTORE macros
> >
> > 1. wakeup_long64() isn't a function that can be called. It's actually
> > redirected to via a return instruction in the entry code. It
> > shouldn't be annotated as a callable function. Change ENDPROC ->
> > PROC accordingly.
>
> But I see -> END.
Oops! It should say -> END.
> > 2. do_suspend_lowlevel() is a non-leaf callable function, so
> > save/restore the frame pointer with FP_SAVE/RESTORE.
>
> It does not work with the frame pointer itself. Is FP_SAVE/RESTORE
> still neccessary? Will you need FP_RESTORE to wakeup_long64, then?
wakeup_long64 jumps to .Lresume_point, which does the FP_RESTORE.
> > 3. Remove the unnecessary jump to .Lresume_point, as it just results in
> > jumping to the next instruction (which is a nop because of the
> > align). Otherwise asmvalidate gets confused by the jump.
>
> It also results in flushing the pipeline. Ok, I guess this one is unneccessary.
>
> > 4. Change the "jmp restore_processor_state" to a call instruction,
> > because jumping outside the function's boundaries isn't allowed. Now
> > restore_processor_state() will return back to do_suspend_lowlevel()
> > instead of do_suspend_lowlevel()'s caller.
> >
> > 5. Remove superfluous rsp changes.
>
> Did you test the changes?
Yes, I verified that it didn't break suspend/resume on my system.
> Do you plan to make similar changes to wakeup_32.S?
Currently, asmvalidate is x86_64 only, so I'm only fixing the 64-bit
stuff right now.
> > diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S
> > index 8c35df4..7e442be 100644
> > --- a/arch/x86/kernel/acpi/wakeup_64.S
> > +++ b/arch/x86/kernel/acpi/wakeup_64.S
> > @@ -5,6 +5,7 @@
> > #include <asm/page_types.h>
> > #include <asm/msr.h>
> > #include <asm/asm-offsets.h>
> > +#include <asm/func.h>
> >
> > # Copyright 2003 Pavel Machek <[email protected]>, distribute under GPLv2
> >
> > @@ -33,13 +34,13 @@ ENTRY(wakeup_long64)
> >
> > movq saved_rip, %rax
> > jmp *%rax
> > -ENDPROC(wakeup_long64)
> > +END(wakeup_long64)
> >
>
> This should result in no binary code changes, so that's ok with me...
>
> > ENTRY(do_suspend_lowlevel)
> > - subq $8, %rsp
> > + FP_SAVE
> > xorl %eax, %eax
> > call save_processor_state
> >
>
> Are you sure? Stuff like
> movq $saved_context, %rax
> movq %rsp, pt_regs_sp(%rax)
>
> follows. And you did not modify wakeup_long64, which now receives
> different value in saved_rsp.
Hm, I'm looking hard, but I still don't see a problem with that code.
It's saving rsp to the saved_context struct. As I mentioned above, it's
ok for the wakeup_long64 path to restore the same rsp value, since it
jumps to .Lresume_point which has FP_RESTORE.
> > @@ -108,8 +108,9 @@ ENTRY(do_suspend_lowlevel)
> > movq pt_regs_r15(%rax), %r15
> >
> > xorl %eax, %eax
> > - addq $8, %rsp
> > - jmp restore_processor_state
> > + call restore_processor_state
> > + FP_RESTORE
> > + ret
> > ENDPROC(do_suspend_lowlevel)
>
> Umm. I rather liked the direct jump.
Why?
--
Josh
> In most cases there are ways to keep the optimizations. For example:
>
> - grow the function bounds to keep the jump internal
So you mean moving it after the ret? That still means icache bloat.
> - duplicate the destination code inside the function
> - convert the jump to a call
That all won't work for a lot of cases.
> Also note that these rules only affect _callable_ functions, so the
> entry code and other non-function asm code can still be a pile of
> spaghetti (though I think Andy is working on improving that).
Thank you for your kind words.
> > In fact even gcc with the right options can generate code that violates
> > this. Standard Linux constructions, such as exception handling,
> > also violate this.
> >
> > If your tool needs that your tool is broken.
>
> This tool only validates asm code, so I don't see how whatever gcc does
> is relevant.
Whoever needs it would need it everywhere, right? If it's not needed
for gcc then it shouldn't be needed for assembler code either.
BTW the way gcc handles it is to use the dwarf 4 extensions that
can describe non contiguous sections.`
-Andi
--
[email protected] -- Speaking for myself only.
On Wed, Jun 10, 2015 at 03:21:35PM +0200, Pavel Machek wrote:
> On Wed 2015-06-10 07:06:15, Josh Poimboeuf wrote:
> > Fix the following asmvalidate warnings:
> >
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x15: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x55: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64(): unsupported fallthrough at end of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x9a: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x116: unsupported jump to outside of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): unsupported fallthrough at end of function
> > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): missing FP_SAVE/RESTORE macros
> >
>
> Actually first things first. Purpose of warnings is to pinpoint
> errors. Do you believe there are some errors in wakeup_64.S?
The "errors" are that it doesn't conform with the guidelines outlined in
the cover letter. Specifically, wakeup_long64() is improperly
annotated, and do_suspend_lowlevel() doesn't honor CONFIG_FRAME_POINTER.
--
Josh
On Wed, Jun 10, 2015 at 03:42:41PM +0200, Pavel Machek wrote:
> On Wed 2015-06-10 07:06:08, Josh Poimboeuf wrote:
> > The previous version of this patch set was named "Compile-time stack
> > frame pointer validation". I changed the subject from "frame pointer
> > validation" to "asm code validation" because the focus of the patch set
> > has changed to be less frame pointer-focused and more asm-focused. I
> > also renamed the tool to asmvalidate (it was previously called
> > stackvalidate) and basically rewrote most of the code.
> >
> > The goal of asm validation is to enforce sane rules on asm code: all
> > callable asm functions must be self-contained and properly annotated.
> >
> > Some of the benefits are:
> >
> > - Frame pointers are more reliable.
> >
> > - DWARF CFI metadata can be autogenerated (coming soon).
> >
> > - The asm code becomes less like spaghetti, more like C, and easier to
> > comprehend.
> >
> >
> > The asmvalidate tool runs on every compiled .S file, and enforces the
> > following rules:
> >
> > 1. Each callable function must be annotated with the ELF STT_FUNC type.
> > This is typically done using the existing ENTRY/ENDPROC macros. If
> > asmvalidate finds a return instruction outside of a function, it
> > flags an error, since that usually indicates callable code which
> > should be annotated accordingly.
> >
> > 2. Each callable function must never leave its own bounds (i.e. with a
> > jump to outside the function) except when returning.
> >
> > 3. Each callable non-leaf function must have frame pointer logic (if
> > required by CONFIG_FRAME_POINTER or the architecture's back chain
> > rules). This should by done by the FP_SAVE/FP_RESTORE macros.
> >
> >
> > It currently only supports x86_64, but the code is generic and designed
> > for it to be easy to plug in support for other architectures.
> >
> > There are still a lot of outstanding warnings (which I'll paste as a
> > reply to this email). Once those are all cleaned up, we can change the
> > warnings to build errors and change the default to
> > CONFIG_ASM_VALIDATION=y so the asm code stays clean.
>
> You have interesting definition of "clean".
"clean":
- reliably honors CONFIG_FRAME_POINTER
- reliably creates/generates DWARF CFI metadata
- doesn't break stack walking
- code is more readable
> The reason we sometimes have to use assembly is that it is impossible
> to write corresponding code in C, or that performance would be bad.
Agreed, but I don't see how this patch set prevents those things.
> So... fixing these may have some sense, but I doubt enforcing "you
> can't write real assembly" is a good idea.
You can certainly still write real assembly. This just creates a few
constraints. I really don't think they are very limiting.
--
Josh
On Wed, Jun 10, 2015 at 04:11:04PM +0200, Andi Kleen wrote:
> > In most cases there are ways to keep the optimizations. For example:
> >
> > - grow the function bounds to keep the jump internal
>
> So you mean moving it after the ret? That still means icache bloat.
No, in most cases it just means changing the ELF annotations. See patch
9 for an example.
> > - duplicate the destination code inside the function
> > - convert the jump to a call
>
> That all won't work for a lot of cases.
Hm, could you give an example?
> > Also note that these rules only affect _callable_ functions, so the
> > entry code and other non-function asm code can still be a pile of
> > spaghetti (though I think Andy is working on improving that).
>
> Thank you for your kind words.
Don't like spaghetti? :-)
> > > In fact even gcc with the right options can generate code that violates
> > > this. Standard Linux constructions, such as exception handling,
> > > also violate this.
> > >
> > > If your tool needs that your tool is broken.
> >
> > This tool only validates asm code, so I don't see how whatever gcc does
> > is relevant.
>
> Whoever needs it would need it everywhere, right? If it's not needed
> for gcc then it shouldn't be needed for assembler code either.
Well, I don't see how that's really a logical conclusion. But we're
probably being too vague here... Do you have any examples where you
really need to jump outside of a callable function?
If we ignore C++, then 99% of the time, C functions are self-contained.
The only exception I can think of is for switch statements, which
sometimes have an external jump table.
--
Josh
> > > - duplicate the destination code inside the function
> > > - convert the jump to a call
> >
> > That all won't work for a lot of cases.
>
> Hm, could you give an example?
Just a standard *_user exception handler.
>
> Well, I don't see how that's really a logical conclusion.
What's special about assembler code?
> But we're
> probably being too vague here... Do you have any examples where you
> really need to jump outside of a callable function?
It's not needed, but it's an optimization to optimize icache usage.
It is optional (-freorder-blocks-and-partition)
In this case gcc splits the function into two (hot and cold)
It's actually a nice optimization and it would be sad from stopping
the kernel from using it.
-Andi
--
[email protected] -- Speaking for myself only.
On Wed, Jun 10, 2015 at 05:04:12PM +0200, Andi Kleen wrote:
> > > > - duplicate the destination code inside the function
> > > > - convert the jump to a call
> > >
> > > That all won't work for a lot of cases.
> >
> > Hm, could you give an example?
>
> Just a standard *_user exception handler.
I'm afraid I don't follow. Exception handlers don't work via jump
instructions, but rather via CPU exceptions.
Or are you talking about something else?
> > Well, I don't see how that's really a logical conclusion.
>
> What's special about assembler code?
Ok, I'll bite:
- it's human-generated
- it's much simpler
- it doesn't have stack metadata by default
- it has far fewer constraints
But I don't see this line of discussion going anywhere without any real
examples of why you need external jumps in asm functions...
> > But we're
> > probably being too vague here... Do you have any examples where you
> > really need to jump outside of a callable function?
>
> It's not needed, but it's an optimization to optimize icache usage.
> It is optional (-freorder-blocks-and-partition)
>
> In this case gcc splits the function into two (hot and cold)
>
> It's actually a nice optimization and it would be sad from stopping
> the kernel from using it.
Sorry if I wasn't clear, I was trying to ask for examples in kernel asm
code.
Are you suggesting that we implement this gcc optimization in kernel asm
code?
--
Josh
On Wed, Jun 10, 2015 at 10:31:55AM -0500, Josh Poimboeuf wrote:
> On Wed, Jun 10, 2015 at 05:04:12PM +0200, Andi Kleen wrote:
> > It's not needed, but it's an optimization to optimize icache usage.
> > It is optional (-freorder-blocks-and-partition)
> >
> > In this case gcc splits the function into two (hot and cold)
> >
> > It's actually a nice optimization and it would be sad from stopping
> > the kernel from using it.
>
> Sorry if I wasn't clear, I was trying to ask for examples in kernel asm
> code.
>
> Are you suggesting that we implement this gcc optimization in kernel asm
> code?
If you're wanting something like -freorder-blocks-and-partition for asm
code, maybe we could implement something analagous to the
likely()/unlikely() macros, to allow the hot and cold portions to be
placed into different sections. (I could then teach asmvalidate how to
validate such code.)
Would that be useful?
--
Josh
On Jun 10, 2015 5:07 AM, "Josh Poimboeuf" <[email protected]> wrote:
>
> Add a new CONFIG_ASM_VALIDATION option which adds an asmvalidate host
> tool which runs on every compiled .S file. Its goal is to enforce sane
> rules on all asm code, so that stack debug metadata (frame/back chain
> pointers and/or DWARF CFI metadata) can be made reliable.
>
> It enforces the following rules:
>
> 1. Each callable function must be annotated with the ELF STT_FUNC type.
> This is typically done using the ENTRY/ENDPROC macros. If
> asmvalidate finds a return instruction outside of a function, it
> flags an error, since that usually indicates callable code which
> should be annotated accordingly.
>
> 2. Each callable function must never leave its own bounds (i.e. with a
> jump to outside the function) except when returning.
Won't that break with sibling/tail calls? GCC can generate those, and
the ia32_ptregs_common label is an example of such a thing.
I'd rather have the script understand tail calls and possibly require
that ia32_ptregs_common have a dummy frame pointer save in front
before the label if needed.
--Andy
On Wed, Jun 10, 2015 at 10:21:36AM -0700, Andy Lutomirski wrote:
> On Jun 10, 2015 5:07 AM, "Josh Poimboeuf" <[email protected]> wrote:
> >
> > Add a new CONFIG_ASM_VALIDATION option which adds an asmvalidate host
> > tool which runs on every compiled .S file. Its goal is to enforce sane
> > rules on all asm code, so that stack debug metadata (frame/back chain
> > pointers and/or DWARF CFI metadata) can be made reliable.
> >
> > It enforces the following rules:
> >
> > 1. Each callable function must be annotated with the ELF STT_FUNC type.
> > This is typically done using the ENTRY/ENDPROC macros. If
> > asmvalidate finds a return instruction outside of a function, it
> > flags an error, since that usually indicates callable code which
> > should be annotated accordingly.
> >
> > 2. Each callable function must never leave its own bounds (i.e. with a
> > jump to outside the function) except when returning.
>
> Won't that break with sibling/tail calls?
Yes, asmvalidate will flag a warning for tail calls.
> GCC can generate those, and the ia32_ptregs_common label is an example
> of such a thing.
>
> I'd rather have the script understand tail calls and possibly require
> that ia32_ptregs_common have a dummy frame pointer save in front
> before the label if needed.
Why do you prefer tail calls there? See patch 3 for how I handled that
for ia32_ptregs_common (I duplicated the code with macros).
I think adding support for tail calls in the tooling would be tricky.
So I'm just trying to figure out if there's a good reason to keep them.
--
Josh
On Wed, Jun 10, 2015 at 10:53 AM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Jun 10, 2015 at 10:21:36AM -0700, Andy Lutomirski wrote:
>> On Jun 10, 2015 5:07 AM, "Josh Poimboeuf" <[email protected]> wrote:
>> >
>> > Add a new CONFIG_ASM_VALIDATION option which adds an asmvalidate host
>> > tool which runs on every compiled .S file. Its goal is to enforce sane
>> > rules on all asm code, so that stack debug metadata (frame/back chain
>> > pointers and/or DWARF CFI metadata) can be made reliable.
>> >
>> > It enforces the following rules:
>> >
>> > 1. Each callable function must be annotated with the ELF STT_FUNC type.
>> > This is typically done using the ENTRY/ENDPROC macros. If
>> > asmvalidate finds a return instruction outside of a function, it
>> > flags an error, since that usually indicates callable code which
>> > should be annotated accordingly.
>> >
>> > 2. Each callable function must never leave its own bounds (i.e. with a
>> > jump to outside the function) except when returning.
>>
>> Won't that break with sibling/tail calls?
>
> Yes, asmvalidate will flag a warning for tail calls.
>
>> GCC can generate those, and the ia32_ptregs_common label is an example
>> of such a thing.
>>
>> I'd rather have the script understand tail calls and possibly require
>> that ia32_ptregs_common have a dummy frame pointer save in front
>> before the label if needed.
>
> Why do you prefer tail calls there? See patch 3 for how I handled that
> for ia32_ptregs_common (I duplicated the code with macros).
>
> I think adding support for tail calls in the tooling would be tricky.
> So I'm just trying to figure out if there's a good reason to keep them.
To save code size by deduplicating common tails. The code currently
does that, and it would be nice to avoid bloating the code to keep the
validator happy.
I imagine that an automatic CFI annotation adder would walk through
functions one instruction at a time and keep track of the frame state.
If so, then it could verify that common jump targets had identical
state and continue walking through them and annotating. I think this
would get this case right, and it might be necessary anyway to handle
jumps within functions.
--Andy
On Wed, Jun 10, 2015 at 10:21:36AM -0700, Andy Lutomirski wrote:
> On Jun 10, 2015 5:07 AM, "Josh Poimboeuf" <[email protected]> wrote:
> >
> > Add a new CONFIG_ASM_VALIDATION option which adds an asmvalidate host
> > tool which runs on every compiled .S file. Its goal is to enforce sane
> > rules on all asm code, so that stack debug metadata (frame/back chain
> > pointers and/or DWARF CFI metadata) can be made reliable.
> >
> > It enforces the following rules:
> >
> > 1. Each callable function must be annotated with the ELF STT_FUNC type.
> > This is typically done using the ENTRY/ENDPROC macros. If
> > asmvalidate finds a return instruction outside of a function, it
> > flags an error, since that usually indicates callable code which
> > should be annotated accordingly.
> >
> > 2. Each callable function must never leave its own bounds (i.e. with a
> > jump to outside the function) except when returning.
>
> Won't that break with sibling/tail calls? GCC can generate those, and
> the ia32_ptregs_common label is an example of such a thing.
It only validates hand-written assembly, so how it could?
--
Vojtech Pavlik
Director SUSE Labs
On Wed 2015-06-10 07:06:09, Josh Poimboeuf wrote:
> Add the FP_SAVE and FP_RESTORE asm macros, which can be used to save and
> restore the frame pointer.
Add a changelog, which can be used to tell what changed.
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> arch/x86/include/asm/func.h | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
> create mode 100644 arch/x86/include/asm/func.h
>
> diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
> new file mode 100644
> index 0000000..4d62782
> --- /dev/null
> +++ b/arch/x86/include/asm/func.h
> @@ -0,0 +1,24 @@
> +#ifndef _ASM_X86_FUNC_H
> +#define _ASM_X86_FUNC_H
> +
> +#include <linux/linkage.h>
> +#include <asm/asm.h>
> +
> +/*
> + * These are frame pointer save and restore macros. They should be used by
> + * every callable non-leaf asm function.
> + */
> +.macro FP_SAVE name
> + .if CONFIG_FRAME_POINTER
> + push %_ASM_BP
> + _ASM_MOV %_ASM_SP, %_ASM_BP
> + .endif
> +.endm
Hmm. This will not compile when included into .c file. Should it have
other extension than .h? (Or can the macros be done in different way?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, Jun 10, 2015 at 11:16 AM, Vojtech Pavlik <[email protected]> wrote:
> On Wed, Jun 10, 2015 at 10:21:36AM -0700, Andy Lutomirski wrote:
>> On Jun 10, 2015 5:07 AM, "Josh Poimboeuf" <[email protected]> wrote:
>> >
>> > Add a new CONFIG_ASM_VALIDATION option which adds an asmvalidate host
>> > tool which runs on every compiled .S file. Its goal is to enforce sane
>> > rules on all asm code, so that stack debug metadata (frame/back chain
>> > pointers and/or DWARF CFI metadata) can be made reliable.
>> >
>> > It enforces the following rules:
>> >
>> > 1. Each callable function must be annotated with the ELF STT_FUNC type.
>> > This is typically done using the ENTRY/ENDPROC macros. If
>> > asmvalidate finds a return instruction outside of a function, it
>> > flags an error, since that usually indicates callable code which
>> > should be annotated accordingly.
>> >
>> > 2. Each callable function must never leave its own bounds (i.e. with a
>> > jump to outside the function) except when returning.
>>
>> Won't that break with sibling/tail calls? GCC can generate those, and
>> the ia32_ptregs_common label is an example of such a thing.
>
> It only validates hand-written assembly, so how it could?
>
It's a hand-written tail call.
--Andy
On Wed, Jun 10, 2015 at 5:06 AM, Josh Poimboeuf <[email protected]> wrote:
> The previous version of this patch set was named "Compile-time stack
> frame pointer validation". I changed the subject from "frame pointer
> validation" to "asm code validation" because the focus of the patch set
> has changed to be less frame pointer-focused and more asm-focused. I
> also renamed the tool to asmvalidate (it was previously called
> stackvalidate) and basically rewrote most of the code.
>
Slightly off-topic, but this reminds me: when writing inline asm that
needs to push to the stack (for whatever reason), it's incredibly
messy to get the annotations right -- they're different depending on
whether the previous frame base (is that what "CFA" is?) is currently
sp + constant, in which case we need an annotation adjusting the
constant or whether it's independent of sp (bp + constant), in which
case we shouldn't adjust the offset. (If it's some other function of
sp, we're screwed.)
Regardless of whether these types of annotations end up being done by
hand or by script, should we consider asking the binutils people to
give us some nice .cfi_adjust_for_push and .cfi_adjust_for_pop or
similar directives?
See here for Jan Beulich's solution, which is incomprehensible to me:
http://thread.gmane.org/gmane.linux.kernel/1820765
--Andy
On Wed, Jun 10, 2015 at 08:17:32PM +0200, Pavel Machek wrote:
> On Wed 2015-06-10 07:06:09, Josh Poimboeuf wrote:
> > Add the FP_SAVE and FP_RESTORE asm macros, which can be used to save and
> > restore the frame pointer.
>
> Add a changelog, which can be used to tell what changed.
Fair enough :-)
> > Signed-off-by: Josh Poimboeuf <[email protected]>
> > ---
> > arch/x86/include/asm/func.h | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> > create mode 100644 arch/x86/include/asm/func.h
> >
> > diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
> > new file mode 100644
> > index 0000000..4d62782
> > --- /dev/null
> > +++ b/arch/x86/include/asm/func.h
> > @@ -0,0 +1,24 @@
> > +#ifndef _ASM_X86_FUNC_H
> > +#define _ASM_X86_FUNC_H
> > +
> > +#include <linux/linkage.h>
> > +#include <asm/asm.h>
> > +
> > +/*
> > + * These are frame pointer save and restore macros. They should be used by
> > + * every callable non-leaf asm function.
> > + */
> > +.macro FP_SAVE name
> > + .if CONFIG_FRAME_POINTER
> > + push %_ASM_BP
> > + _ASM_MOV %_ASM_SP, %_ASM_BP
> > + .endif
> > +.endm
>
> Hmm. This will not compile when included into .c file. Should it have
> other extension than .h? (Or can the macros be done in different way?
These are gnu assembler macros. This file is only included by .S files.
--
Josh
Josh Poimboeuf <[email protected]> writes:
> On Wed, Jun 10, 2015 at 05:04:12PM +0200, Andi Kleen wrote:
>> > > > - duplicate the destination code inside the function
>> > > > - convert the jump to a call
>> > >
>> > > That all won't work for a lot of cases.
>> >
>> > Hm, could you give an example?
>>
>> Just a standard *_user exception handler.
>
> I'm afraid I don't follow. Exception handlers don't work via jump
> instructions, but rather via CPU exceptions.
>
> Or are you talking about something else?
Let's take an example:
102:
.section .fixup,"ax"
103: addl %ecx,%edx /* ecx is zerorest also */
jmp copy_user_handle_tail
.previous
_ASM_EXTABLE(100b,103b)
_ASM_EXTABLE(101b,103b)
The exception handling code is part of the function, but it's out of line.
> Are you suggesting that we implement this gcc optimization in kernel asm
> code?
It was how Linux traditionally implemented locking code for example.
Have the hot path handle the uncontended fast path, and the slow path
call.
I don't know if there is much left of it (a lot of it was removed because
it was hard to describe in dwarf3, needs dwarf4). But it seems bad
to completely disallow it.
But yes eventually gcc generated code should use it again, because it's
great for icache usage if you measure it correctly at run time
(not the broken "size" approach that is unfortunately far too common)
-Andi
--
[email protected] -- Speaking for myself only
Josh Poimboeuf <[email protected]> writes:
>
> If you're wanting something like -freorder-blocks-and-partition for asm
> code, maybe we could implement something analagous to the
> likely()/unlikely() macros, to allow the hot and cold portions to be
> placed into different sections. (I could then teach asmvalidate how to
> validate such code.)
>
> Would that be useful?
Eventually you have to handle dwarf4, because that's the only way to
handle the gcc generated code.
So your tool just needs to understand the dwarf. It's ok to warn
about assembler code that is not correctly annotated, and fix it
then.
-Andi
--
[email protected] -- Speaking for myself only
On Wed, Jun 10, 2015 at 11:15:19AM -0700, Andy Lutomirski wrote:
> On Wed, Jun 10, 2015 at 10:53 AM, Josh Poimboeuf <[email protected]> wrote:
> > On Wed, Jun 10, 2015 at 10:21:36AM -0700, Andy Lutomirski wrote:
> >> On Jun 10, 2015 5:07 AM, "Josh Poimboeuf" <[email protected]> wrote:
> >> > 2. Each callable function must never leave its own bounds (i.e. with a
> >> > jump to outside the function) except when returning.
> >>
> >> Won't that break with sibling/tail calls?
> >
> > Yes, asmvalidate will flag a warning for tail calls.
> >
> >> GCC can generate those, and the ia32_ptregs_common label is an example
> >> of such a thing.
> >>
> >> I'd rather have the script understand tail calls and possibly require
> >> that ia32_ptregs_common have a dummy frame pointer save in front
> >> before the label if needed.
> >
> > Why do you prefer tail calls there? See patch 3 for how I handled that
> > for ia32_ptregs_common (I duplicated the code with macros).
> >
> > I think adding support for tail calls in the tooling would be tricky.
> > So I'm just trying to figure out if there's a good reason to keep them.
>
> To save code size by deduplicating common tails. The code currently
> does that, and it would be nice to avoid bloating the code to keep the
> validator happy.
Well, I wonder whether it's really worth sacrificing code readability
and consistency, and maybe some improved i-cache locality, to save a few
hundred bytes of code size.
> I imagine that an automatic CFI annotation adder would walk through
> functions one instruction at a time and keep track of the frame state.
> If so, then it could verify that common jump targets had identical
> state and continue walking through them and annotating. I think this
> would get this case right, and it might be necessary anyway to handle
> jumps within functions.
This would definitely add complexity to both asmvalidate and the CFI
generator. In fact it sounds like it would push the CFI generator out
of its current awk script territory and more into complex C code
territory.
--
Josh
On Wed, Jun 10, 2015 at 11:40:06AM -0700, Andi Kleen wrote:
> Josh Poimboeuf <[email protected]> writes:
>
> > On Wed, Jun 10, 2015 at 05:04:12PM +0200, Andi Kleen wrote:
> >> > > > - duplicate the destination code inside the function
> >> > > > - convert the jump to a call
> >> > >
> >> > > That all won't work for a lot of cases.
> >> >
> >> > Hm, could you give an example?
> >>
> >> Just a standard *_user exception handler.
> >
> > I'm afraid I don't follow. Exception handlers don't work via jump
> > instructions, but rather via CPU exceptions.
> >
> > Or are you talking about something else?
>
> Let's take an example:
>
> 102:
> .section .fixup,"ax"
> 103: addl %ecx,%edx /* ecx is zerorest also */
> jmp copy_user_handle_tail
> .previous
>
> _ASM_EXTABLE(100b,103b)
> _ASM_EXTABLE(101b,103b)
>
> The exception handling code is part of the function, but it's out of line.
The jump instruction is in the .fixup section, not in the callable
function itself. So it doesn't violate the asmvalidate rules.
> > Are you suggesting that we implement this gcc optimization in kernel asm
> > code?
>
> It was how Linux traditionally implemented locking code for example.
> Have the hot path handle the uncontended fast path, and the slow path
> call.
>
> I don't know if there is much left of it (a lot of it was removed because
> it was hard to describe in dwarf3, needs dwarf4). But it seems bad
> to completely disallow it.
>
> But yes eventually gcc generated code should use it again, because it's
> great for icache usage if you measure it correctly at run time
> (not the broken "size" approach that is unfortunately far too common)
This patch set has no relationship to gcc generated code whatsoever. So
it doesn't disallow anything there.
For kernel asm code, AFAIK, such a mechanism for hot/cold path
separation in separate sections doesn't exist today. So it's not
"disallowed" there either. It's just apparently not currently done.
If somebody were to create such a mechanism, I think we could
standardize it in such a way that it could be compatible with
asmvalidate.
--
Josh
On Wed, Jun 10, 2015 at 12:36 PM, Josh Poimboeuf <[email protected]> wrote:
> On Wed, Jun 10, 2015 at 11:40:06AM -0700, Andi Kleen wrote:
>> Josh Poimboeuf <[email protected]> writes:
>>
>> > On Wed, Jun 10, 2015 at 05:04:12PM +0200, Andi Kleen wrote:
>> >> > > > - duplicate the destination code inside the function
>> >> > > > - convert the jump to a call
>> >> > >
>> >> > > That all won't work for a lot of cases.
>> >> >
>> >> > Hm, could you give an example?
>> >>
>> >> Just a standard *_user exception handler.
>> >
>> > I'm afraid I don't follow. Exception handlers don't work via jump
>> > instructions, but rather via CPU exceptions.
>> >
>> > Or are you talking about something else?
>>
>> Let's take an example:
>>
>> 102:
>> .section .fixup,"ax"
>> 103: addl %ecx,%edx /* ecx is zerorest also */
>> jmp copy_user_handle_tail
>> .previous
>>
>> _ASM_EXTABLE(100b,103b)
>> _ASM_EXTABLE(101b,103b)
>>
>> The exception handling code is part of the function, but it's out of line.
>
> The jump instruction is in the .fixup section, not in the callable
> function itself. So it doesn't violate the asmvalidate rules.
It still won't unwind correctly unless .pushsection somehow magically
propagates CFI state. (Does it?)
>
>> > Are you suggesting that we implement this gcc optimization in kernel asm
>> > code?
>>
>> It was how Linux traditionally implemented locking code for example.
>> Have the hot path handle the uncontended fast path, and the slow path
>> call.
>>
>> I don't know if there is much left of it (a lot of it was removed because
>> it was hard to describe in dwarf3, needs dwarf4). But it seems bad
>> to completely disallow it.
>>
>> But yes eventually gcc generated code should use it again, because it's
>> great for icache usage if you measure it correctly at run time
>> (not the broken "size" approach that is unfortunately far too common)
>
> This patch set has no relationship to gcc generated code whatsoever. So
> it doesn't disallow anything there.
>
> For kernel asm code, AFAIK, such a mechanism for hot/cold path
> separation in separate sections doesn't exist today. So it's not
> "disallowed" there either. It's just apparently not currently done.
>
> If somebody were to create such a mechanism, I think we could
> standardize it in such a way that it could be compatible with
> asmvalidate.
Hopefully true. The entry code is full of tail calls, though.
--Andy
>
> --
> Josh
--
Andy Lutomirski
AMA Capital Management, LLC
On Wed, Jun 10, 2015 at 11:41:43AM -0700, Andi Kleen wrote:
> Josh Poimboeuf <[email protected]> writes:
> >
> > If you're wanting something like -freorder-blocks-and-partition for asm
> > code, maybe we could implement something analagous to the
> > likely()/unlikely() macros, to allow the hot and cold portions to be
> > placed into different sections. (I could then teach asmvalidate how to
> > validate such code.)
> >
> > Would that be useful?
>
> Eventually you have to handle dwarf4, because that's the only way to
> handle the gcc generated code.
>
> So your tool just needs to understand the dwarf. It's ok to warn
> about assembler code that is not correctly annotated, and fix it
> then.
Ok, I think we're talking about two different things. Maybe you're
mixing up this patch set with some of the other things we've discussed
like the DWARF CFI generation tool or the runtime DWARF unwinder.
This patch set is only concerned with enforcing sane rules on asm code.
It has nothing to do with understanding DWARF. In fact, since Ingo
reverted the CFI annotations, there is currently no DWARF in asm code at
all. This patch set is a prerequisite for the other DWARF stuff we
talked about previously.
--
Josh
On Wed, Jun 10, 2015 at 12:38:32PM -0700, Andy Lutomirski wrote:
> On Wed, Jun 10, 2015 at 12:36 PM, Josh Poimboeuf <[email protected]> wrote:
> > On Wed, Jun 10, 2015 at 11:40:06AM -0700, Andi Kleen wrote:
> >> Josh Poimboeuf <[email protected]> writes:
> >>
> >> > On Wed, Jun 10, 2015 at 05:04:12PM +0200, Andi Kleen wrote:
> >> >> > > > - duplicate the destination code inside the function
> >> >> > > > - convert the jump to a call
> >> >> > >
> >> >> > > That all won't work for a lot of cases.
> >> >> >
> >> >> > Hm, could you give an example?
> >> >>
> >> >> Just a standard *_user exception handler.
> >> >
> >> > I'm afraid I don't follow. Exception handlers don't work via jump
> >> > instructions, but rather via CPU exceptions.
> >> >
> >> > Or are you talking about something else?
> >>
> >> Let's take an example:
> >>
> >> 102:
> >> .section .fixup,"ax"
> >> 103: addl %ecx,%edx /* ecx is zerorest also */
> >> jmp copy_user_handle_tail
> >> .previous
> >>
> >> _ASM_EXTABLE(100b,103b)
> >> _ASM_EXTABLE(101b,103b)
> >>
> >> The exception handling code is part of the function, but it's out of line.
> >
> > The jump instruction is in the .fixup section, not in the callable
> > function itself. So it doesn't violate the asmvalidate rules.
>
> It still won't unwind correctly unless .pushsection somehow magically
> propagates CFI state. (Does it?)
I don't think it does. We'll probably need some intelligence in the
CFI generation tooling to deal properly with the extable stuff.
> >> > Are you suggesting that we implement this gcc optimization in kernel asm
> >> > code?
> >>
> >> It was how Linux traditionally implemented locking code for example.
> >> Have the hot path handle the uncontended fast path, and the slow path
> >> call.
> >>
> >> I don't know if there is much left of it (a lot of it was removed because
> >> it was hard to describe in dwarf3, needs dwarf4). But it seems bad
> >> to completely disallow it.
> >>
> >> But yes eventually gcc generated code should use it again, because it's
> >> great for icache usage if you measure it correctly at run time
> >> (not the broken "size" approach that is unfortunately far too common)
> >
> > This patch set has no relationship to gcc generated code whatsoever. So
> > it doesn't disallow anything there.
> >
> > For kernel asm code, AFAIK, such a mechanism for hot/cold path
> > separation in separate sections doesn't exist today. So it's not
> > "disallowed" there either. It's just apparently not currently done.
> >
> > If somebody were to create such a mechanism, I think we could
> > standardize it in such a way that it could be compatible with
> > asmvalidate.
>
> Hopefully true. The entry code is full of tail calls, though.
Well, I wasn't talking specifically about tail calls here. But either
way, as long as they're not in a callable function (which is the case
for most of the entry code), asmvalidate doesn't care.
--
Josh
On Wed, Jun 10, 2015 at 11:24:05AM -0700, Andy Lutomirski wrote:
> Slightly off-topic, but this reminds me: when writing inline asm that
> needs to push to the stack (for whatever reason), it's incredibly
> messy to get the annotations right -- they're different depending on
> whether the previous frame base (is that what "CFA" is?) is currently
> sp + constant, in which case we need an annotation adjusting the
> constant or whether it's independent of sp (bp + constant), in which
> case we shouldn't adjust the offset. (If it's some other function of
> sp, we're screwed.)
>
> Regardless of whether these types of annotations end up being done by
> hand or by script, should we consider asking the binutils people to
> give us some nice .cfi_adjust_for_push and .cfi_adjust_for_pop or
> similar directives?
Hm, that's a tough one. Might be worth asking...
Another alternative would be to ask gcc to make a change to always setup
the frame pointer for any function which has inline assembly, so that
you know (hopefully) that CFA is based on bp.
Or, maybe there's already a way to force gcc to do that with the asm
directive somehow?
>
> See here for Jan Beulich's solution, which is incomprehensible to me:
>
> http://thread.gmane.org/gmane.linux.kernel/1820765
<brain explodes>
--
Josh
On Wed, Jun 10, 2015 at 01:58:45PM -0500, Josh Poimboeuf wrote:
> On Wed, Jun 10, 2015 at 11:15:19AM -0700, Andy Lutomirski wrote:
> > On Wed, Jun 10, 2015 at 10:53 AM, Josh Poimboeuf <[email protected]> wrote:
> > > On Wed, Jun 10, 2015 at 10:21:36AM -0700, Andy Lutomirski wrote:
> > >> GCC can generate those, and the ia32_ptregs_common label is an example
> > >> of such a thing.
> > >>
> > >> I'd rather have the script understand tail calls and possibly require
> > >> that ia32_ptregs_common have a dummy frame pointer save in front
> > >> before the label if needed.
> > >
> > > Why do you prefer tail calls there? See patch 3 for how I handled that
> > > for ia32_ptregs_common (I duplicated the code with macros).
> > >
> > > I think adding support for tail calls in the tooling would be tricky.
> > > So I'm just trying to figure out if there's a good reason to keep them.
> >
> > To save code size by deduplicating common tails. The code currently
> > does that, and it would be nice to avoid bloating the code to keep the
> > validator happy.
>
> Well, I wonder whether it's really worth sacrificing code readability
> and consistency, and maybe some improved i-cache locality, to save a few
> hundred bytes of code size.
I should also mention that my proposed ia32_ptregs_common patch, which
duplicated the needed code, was more optimized for performance than code
size.
But if you're more worried about code size, we could turn
ia32_ptregs_common into a proper callable function, and then replace
jmp ia32_ptregs_common
with:
call ia32_ptregs_common
ret
So it becomes a regular call instead of a tail call. It only adds a few
instructions and the function is self-contained. Would that be good
enough?
--
Josh
On Wed, 10 Jun 2015, Pavel Machek wrote:
> > diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
> > new file mode 100644
> > index 0000000..4d62782
> > --- /dev/null
> > +++ b/arch/x86/include/asm/func.h
> > @@ -0,0 +1,24 @@
> > +#ifndef _ASM_X86_FUNC_H
> > +#define _ASM_X86_FUNC_H
> > +
> > +#include <linux/linkage.h>
> > +#include <asm/asm.h>
> > +
> > +/*
> > + * These are frame pointer save and restore macros. They should be used by
> > + * every callable non-leaf asm function.
> > + */
> > +.macro FP_SAVE name
> > + .if CONFIG_FRAME_POINTER
> > + push %_ASM_BP
> > + _ASM_MOV %_ASM_SP, %_ASM_BP
> > + .endif
> > +.endm
>
> Hmm. This will not compile when included into .c file. Should it have
> other extension than .h? (Or can the macros be done in different way?
We have quite a few of .h headers in the kernel already which are supposed
to be included only by .S files, so what exactly is the problem you are
seeing here?
--
Jiri Kosina
SUSE Labs
* Josh Poimboeuf <[email protected]> wrote:
> I should also mention that my proposed ia32_ptregs_common patch, which
> duplicated the needed code, was more optimized for performance than code size.
>
> But if you're more worried about code size, we could turn ia32_ptregs_common
> into a proper callable function, and then replace
>
> jmp ia32_ptregs_common
>
> with:
>
> call ia32_ptregs_common
> ret
>
> So it becomes a regular call instead of a tail call. It only adds a few
> instructions and the function is self-contained. Would that be good enough?
No, debug info should not slow down the kernel, especially not code we write in
assembly partly for performance.
Thanks,
Ingo
* Josh Poimboeuf <[email protected]> wrote:
> > I imagine that an automatic CFI annotation adder would walk through functions
> > one instruction at a time and keep track of the frame state. If so, then it
> > could verify that common jump targets had identical state and continue walking
> > through them and annotating. I think this would get this case right, and it
> > might be necessary anyway to handle jumps within functions.
>
> This would definitely add complexity to both asmvalidate and the CFI generator.
> In fact it sounds like it would push the CFI generator out of its current awk
> script territory and more into complex C code territory.
I'd count that as a plus: awk isn't a common skillset while C is, and properly
written it doesn't have to be _that_ complex.
Thanks,
Ingo
* Josh Poimboeuf <[email protected]> wrote:
> On Wed, Jun 10, 2015 at 03:21:35PM +0200, Pavel Machek wrote:
> > On Wed 2015-06-10 07:06:15, Josh Poimboeuf wrote:
> > > Fix the following asmvalidate warnings:
> > >
> > > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x15: unsupported jump to outside of function
> > > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64()+0x55: unsupported jump to outside of function
> > > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: wakeup_long64(): unsupported fallthrough at end of function
> > > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x9a: unsupported jump to outside of function
> > > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel()+0x116: unsupported jump to outside of function
> > > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): unsupported fallthrough at end of function
> > > asmvalidate: arch/x86/kernel/acpi/wakeup_64.o: do_suspend_lowlevel(): missing FP_SAVE/RESTORE macros
> > >
> >
> > Actually first things first. Purpose of warnings is to pinpoint
> > errors. Do you believe there are some errors in wakeup_64.S?
>
> The "errors" are that it doesn't conform with the guidelines outlined in
> the cover letter. Specifically, wakeup_long64() is improperly
> annotated, and do_suspend_lowlevel() doesn't honor CONFIG_FRAME_POINTER.
Please create a file for this in Documentation/x86/, outlining the common cases of
such .S debug info problems and the effects this has on the stack backtrace
output.
Thanks,
Ingo
On Thu 2015-06-11 06:22:49, Jiri Kosina wrote:
> On Wed, 10 Jun 2015, Pavel Machek wrote:
>
> > > diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
> > > new file mode 100644
> > > index 0000000..4d62782
> > > --- /dev/null
> > > +++ b/arch/x86/include/asm/func.h
> > > @@ -0,0 +1,24 @@
> > > +#ifndef _ASM_X86_FUNC_H
> > > +#define _ASM_X86_FUNC_H
> > > +
> > > +#include <linux/linkage.h>
> > > +#include <asm/asm.h>
> > > +
> > > +/*
> > > + * These are frame pointer save and restore macros. They should be used by
> > > + * every callable non-leaf asm function.
> > > + */
> > > +.macro FP_SAVE name
> > > + .if CONFIG_FRAME_POINTER
> > > + push %_ASM_BP
> > > + _ASM_MOV %_ASM_SP, %_ASM_BP
> > > + .endif
> > > +.endm
> >
> > Hmm. This will not compile when included into .c file. Should it have
> > other extension than .h? (Or can the macros be done in different way?
>
> We have quite a few of .h headers in the kernel already which are supposed
> to be included only by .S files, so what exactly is the problem you are
> seeing here?
Such as...? Can this be merged into one of them so that we don't have
a separate file? For example "frame.h"?
(I thought asm includes traditionally had different extension, but I
may be mistaken).
And at the very least, dwarf2.h includes
#ifndef __ASSEMBLY__
#warning "asm/dwarf2.h should be only included in pure assembly files"
#endif
and guards stuff that would not compile in C with
#ifdef __ASSEMBLY__
....
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, 11 Jun 2015, Pavel Machek wrote:
> > We have quite a few of .h headers in the kernel already which are supposed
> > to be included only by .S files, so what exactly is the problem you are
> > seeing here?
>
> Such as...?
arch/x86/include/asm/calling.h
is seems to be the only one (on x86 arch) which doesn't have the
__ASSEMBLY__ guard.
--
Jiri Kosina
SUSE Labs
Hi!
> > > 5. Remove superfluous rsp changes.
> >
> > Did you test the changes?
>
> Yes, I verified that it didn't break suspend/resume on my system.
Ok, so I can not see anything wrong, either. I'd like to understand
why the original code manipulated %rsp, but...
If you did testing with frame pointer on, you can get my
Acked-by: Pavel Machek <[email protected]>
> > Do you plan to make similar changes to wakeup_32.S?
>
> Currently, asmvalidate is x86_64 only, so I'm only fixing the 64-bit
> stuff right now.
Well, you are "improving debuggability", afaict. It worked well before.
> > > @@ -108,8 +108,9 @@ ENTRY(do_suspend_lowlevel)
> > > movq pt_regs_r15(%rax), %r15
> > >
> > > xorl %eax, %eax
> > > - addq $8, %rsp
> > > - jmp restore_processor_state
> > > + call restore_processor_state
> > > + FP_RESTORE
> > > + ret
> > > ENDPROC(do_suspend_lowlevel)
> >
> > Umm. I rather liked the direct jump.
>
> Why?
It is both smaller and faster than the new code. But...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 10 Jun, at 07:06:14AM, Josh Poimboeuf wrote:
> Fix the following asmvalidate warnings:
>
> asmvalidate: arch/x86/platform/efi/efi_stub_64.o: efi_call(): missing FP_SAVE/RESTORE macros
> asmvalidate: arch/x86/boot/compressed/efi_stub_64.o: efi_call(): missing FP_SAVE/RESTORE macros
>
> efi_call() is a non-leaf callable function, so save/restore the frame
> pointer with FP_SAVE/RESTORE.
>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: [email protected]
> ---
> arch/x86/platform/efi/efi_stub_64.S | 3 +++
> 1 file changed, 3 insertions(+)
Yeah, fair enough. Though it's worth noting that because we're calling
firmware functions, which use the Microsoft ABI, %rbp will be saved by
the callee function if used. Anyway,
Acked-by: Matt Fleming <[email protected]>
And since I know Boris in particular has poked around in this area, an
ACK from him would be worth alot.
--
Matt Fleming, Intel Open Source Technology Center
On Thu, Jun 11, 2015 at 08:08:07AM +0200, Ingo Molnar wrote:
>
> * Josh Poimboeuf <[email protected]> wrote:
>
> > I should also mention that my proposed ia32_ptregs_common patch, which
> > duplicated the needed code, was more optimized for performance than code size.
> >
> > But if you're more worried about code size, we could turn ia32_ptregs_common
> > into a proper callable function, and then replace
> >
> > jmp ia32_ptregs_common
> >
> > with:
> >
> > call ia32_ptregs_common
> > ret
> >
> > So it becomes a regular call instead of a tail call. It only adds a few
> > instructions and the function is self-contained. Would that be good enough?
>
> No, debug info should not slow down the kernel, especially not code we write in
> assembly partly for performance.
Ok. I'll work on adding support for external jumps.
--
Josh
On Thu, Jun 11, 2015 at 08:10:50AM +0200, Ingo Molnar wrote:
> * Josh Poimboeuf <[email protected]> wrote:
> > > I imagine that an automatic CFI annotation adder would walk through functions
> > > one instruction at a time and keep track of the frame state. If so, then it
> > > could verify that common jump targets had identical state and continue walking
> > > through them and annotating. I think this would get this case right, and it
> > > might be necessary anyway to handle jumps within functions.
> >
> > This would definitely add complexity to both asmvalidate and the CFI generator.
> > In fact it sounds like it would push the CFI generator out of its current awk
> > script territory and more into complex C code territory.
>
> I'd count that as a plus: awk isn't a common skillset while C is, and properly
> written it doesn't have to be _that_ complex.
The thing is, C is quite painful for text processing. And I think we'd
have to do the analysis at the source text level in order to generate
the .cfi_* instructions to pass to the gnu assembler.
C would definitely make more sense when analyzing object code. In fact,
asmvalidate is written in C. But then I guess we'd have to re-implement
the .cfi stuff and populate the DWARF sections manually instead of
letting the assembler do it.
--
Josh
On Thu, Jun 11, 2015 at 08:46:33AM +0200, Pavel Machek wrote:
> On Thu 2015-06-11 06:22:49, Jiri Kosina wrote:
> > On Wed, 10 Jun 2015, Pavel Machek wrote:
> >
> > > > diff --git a/arch/x86/include/asm/func.h b/arch/x86/include/asm/func.h
> > > > new file mode 100644
> > > > index 0000000..4d62782
> > > > --- /dev/null
> > > > +++ b/arch/x86/include/asm/func.h
> > > > @@ -0,0 +1,24 @@
> > > > +#ifndef _ASM_X86_FUNC_H
> > > > +#define _ASM_X86_FUNC_H
> > > > +
> > > > +#include <linux/linkage.h>
> > > > +#include <asm/asm.h>
> > > > +
> > > > +/*
> > > > + * These are frame pointer save and restore macros. They should be used by
> > > > + * every callable non-leaf asm function.
> > > > + */
> > > > +.macro FP_SAVE name
> > > > + .if CONFIG_FRAME_POINTER
> > > > + push %_ASM_BP
> > > > + _ASM_MOV %_ASM_SP, %_ASM_BP
> > > > + .endif
> > > > +.endm
> > >
> > > Hmm. This will not compile when included into .c file. Should it have
> > > other extension than .h? (Or can the macros be done in different way?
> >
> > We have quite a few of .h headers in the kernel already which are supposed
> > to be included only by .S files, so what exactly is the problem you are
> > seeing here?
>
> Such as...? Can this be merged into one of them so that we don't have
> a separate file? For example "frame.h"?
>
> (I thought asm includes traditionally had different extension, but I
> may be mistaken).
>
> And at the very least, dwarf2.h includes
>
> #ifndef __ASSEMBLY__
> #warning "asm/dwarf2.h should be only included in pure assembly files"
> #endif
>
> and guards stuff that would not compile in C with
>
> #ifdef __ASSEMBLY__
> ....
Ok, I'll move the FP_SAVE/RESTORE stuff into frame.h (which seems to be
completely unused). And I'll make sure to use "#ifdef __ASSEMBLY__".
--
Josh
On 06/11/2015 03:10 PM, Josh Poimboeuf wrote:
> C would definitely make more sense when analyzing object code. In fact,
> asmvalidate is written in C. But then I guess we'd have to re-implement
> the .cfi stuff and populate the DWARF sections manually instead of
> letting the assembler do it.
Was doing all this directly in the assembler considered? That is,
e.g., add some knob that makes it error/warn in the same conditions
you're making the validator catch. For tail calls, you'd e.g., add
some new ".nonlocal" directive that you'd use to whitelist the
following jump. And then if it's possible run a CFI generator
as a separate step over the source, it sounds like it should also
be possible to have the assembler do it instead too (again with
some new high level directive to trigger/help it).
Thanks,
Pedro Alves
On Fri, Jun 12, 2015 at 12:18:16PM +0100, Pedro Alves wrote:
> On 06/11/2015 03:10 PM, Josh Poimboeuf wrote:
>
> > C would definitely make more sense when analyzing object code. In fact,
> > asmvalidate is written in C. But then I guess we'd have to re-implement
> > the .cfi stuff and populate the DWARF sections manually instead of
> > letting the assembler do it.
>
> Was doing all this directly in the assembler considered? That is,
> e.g., add some knob that makes it error/warn in the same conditions
> you're making the validator catch. For tail calls, you'd e.g., add
> some new ".nonlocal" directive that you'd use to whitelist the
> following jump. And then if it's possible run a CFI generator
> as a separate step over the source, it sounds like it should also
> be possible to have the assembler do it instead too (again with
> some new high level directive to trigger/help it).
In general I think doing these types of things in the assembler would be
a good idea. Missing or inaccurate debug data for asm code seems to be
a common problem for other projects as well. As Andy pointed out,
they're doing similar things in musl [1]. So it might be useful to add
an option to the assembler which validates that the code conforms to
certain structural rules, and then inserts frame pointer and/or .cfi
directives.
That said, the kernel has much more custom features than other projects.
There are some sneaky macros, like _ASM_EXTABLE and ALTERNATIVE, which
hide code in various sections. Unless we're able to somehow either stop
using these macros or isolate them to a few places, I doubt that such a
general purpose assembler option would work.
[1] http://www.openwall.com/lists/musl/2015/05/31/5
--
Josh
On 06/12/2015 03:10 PM, Josh Poimboeuf wrote:
> On Fri, Jun 12, 2015 at 12:18:16PM +0100, Pedro Alves wrote:
>> On 06/11/2015 03:10 PM, Josh Poimboeuf wrote:
>>
>>> C would definitely make more sense when analyzing object code. In fact,
>>> asmvalidate is written in C. But then I guess we'd have to re-implement
>>> the .cfi stuff and populate the DWARF sections manually instead of
>>> letting the assembler do it.
>>
>> Was doing all this directly in the assembler considered? That is,
>> e.g., add some knob that makes it error/warn in the same conditions
>> you're making the validator catch. For tail calls, you'd e.g., add
>> some new ".nonlocal" directive that you'd use to whitelist the
>> following jump. And then if it's possible run a CFI generator
>> as a separate step over the source, it sounds like it should also
>> be possible to have the assembler do it instead too (again with
>> some new high level directive to trigger/help it).
>
> In general I think doing these types of things in the assembler would be
> a good idea. Missing or inaccurate debug data for asm code seems to be
> a common problem for other projects as well. As Andy pointed out,
> they're doing similar things in musl [1].
Thanks for the pointer.
> So it might be useful to add
> an option to the assembler which validates that the code conforms to
> certain structural rules, and then inserts frame pointer and/or .cfi
> directives.
> That said, the kernel has much more custom features than other projects.
> There are some sneaky macros, like _ASM_EXTABLE and ALTERNATIVE, which
> hide code in various sections. Unless we're able to somehow either stop
> using these macros or isolate them to a few places, I doubt that such a
> general purpose assembler option would work.
How does the asmvalidator handle these?
> [1] http://www.openwall.com/lists/musl/2015/05/31/5
Thanks,
Pedro Alves
On Fri, Jun 12, 2015 at 05:00:50PM +0100, Pedro Alves wrote:
> On 06/12/2015 03:10 PM, Josh Poimboeuf wrote:
> > That said, the kernel has much more custom features than other projects.
> > There are some sneaky macros, like _ASM_EXTABLE and ALTERNATIVE, which
> > hide code in various sections. Unless we're able to somehow either stop
> > using these macros or isolate them to a few places, I doubt that such a
> > general purpose assembler option would work.
>
> How does the asmvalidator handle these?
They're not easy to deal with...
The ALTERNATIVE macro creates some instructions which can be patched in
at runtime, to replace some original instructions, if the CPU supports
certain features. So we have to look up those replacement instructions
in another section and consider them to be potentially part of the
original function when doing the analysis and generation.
The _ASM_EXTABLE macro creates code which is executed after an
exception. Similarly to the ALTERNATIVE macro, we have to look up those
instructions and consider them to be part of the original function.
--
Josh
On Thu, Jun 11, 2015 at 02:14:39PM +0100, Matt Fleming wrote:
> Yeah, fair enough. Though it's worth noting that because we're calling
> firmware functions, which use the Microsoft ABI, %rbp will be saved by
> the callee function if used.
Yeah, just looked at the spec.
But you know how we don't trust specs. So we get additional paranoid
security that callee won't futz with RBP because we save it before
calling. But we pay the additional penalty in the CONFIG_FRAME_POINTER
case.
Oh what the hell, the 3 additional insns shouldn't be noticeable.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--