In order to prepare replacing of paravirt patching with alternative
patching, add the capability to replace an indirect call with a direct
one to alternative patching.
This is done via a new flag ALT_FLAG_CALL as the target of the call
instruction needs to be evaluated using the value of the location
addressed by the indirect call. For convenience add a macro for a
default call instruction. In case it is being used without the new
flag being set, it will result in a BUG() when being executed. As in
most cases the feature used will be X86_FEATURE_ALWAYS add another
macro ALT_CALL_ALWAYS usable for the flags parameter of the ALTERNATIVE
macros.
For a complete replacement handle the special cases of calling a nop
function and an indirect call of NULL the same way as paravirt does.
Signed-off-by: Juergen Gross <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
V4:
- 32-bit mode doesn't have %rip relative addressing (kernel test robot)
- define ALT_CALL_INSTR in assembly, too (kernel test robot)
V5:
- BUG() in case of inconsistent flag/instruction (Boris Petkov)
- rename flag (Boris Petkov)
- make target address calculation more readable (Boris Petkov)
---
arch/x86/include/asm/alternative.h | 9 ++++++
arch/x86/kernel/alternative.c | 47 ++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index ce788ab4e77c..472334eed6f3 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -10,6 +10,9 @@
#define ALT_FLAG_NOT (1 << 0)
#define ALT_NOT(feature) ((ALT_FLAG_NOT << ALT_FLAGS_SHIFT) | (feature))
+#define ALT_FLAG_DIRECT_CALL (1 << 1)
+#define ALT_DIRECT_CALL(feature) ((ALT_FLAG_DIRECT_CALL << ALT_FLAGS_SHIFT) | (feature))
+#define ALT_CALL_ALWAYS ALT_DIRECT_CALL(X86_FEATURE_ALWAYS)
#ifndef __ASSEMBLY__
@@ -150,6 +153,8 @@ static inline int alternatives_text_reserved(void *start, void *end)
}
#endif /* CONFIG_SMP */
+#define ALT_CALL_INSTR "call BUG_func"
+
#define b_replacement(num) "664"#num
#define e_replacement(num) "665"#num
@@ -386,6 +391,10 @@ void nop_func(void);
.byte \alt_len
.endm
+.macro ALT_CALL_INSTR
+ call BUG_func
+.endm
+
/*
* Define an alternative between two instructions. If @feature is
* present, early code in apply_alternatives() replaces @oldinstr with
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index ca25dd280b8c..e7e53b9e801b 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -395,6 +395,47 @@ noinstr void BUG_func(void)
}
EXPORT_SYMBOL_GPL(BUG_func);
+/*
+ * Rewrite the "call BUG_func" replacement to point to the target of the
+ * indirect pv_ops call "call *disp(%ip)".
+ */
+static int alt_replace_call(u8 *instr, u8 *insn_buff, struct alt_instr *a)
+{
+ void *target, *bug = &BUG_func;
+ s32 disp;
+
+ if (a->replacementlen != 5 || insn_buff[0] != CALL_INSN_OPCODE) {
+ pr_err("ALT_FLAG_DIRECT_CALL set for a non-call replacement instruction\n");
+ BUG();
+ }
+
+ if (a->instrlen != 6 || instr[0] != 0xff || instr[1] != 0x15) {
+ pr_err("ALT_FLAG_DIRECT_CALL set for unrecognized indirect call\n");
+ BUG();
+ }
+
+ disp = *(s32 *)(instr + 2);
+#ifdef CONFIG_X86_64
+ /* ff 15 00 00 00 00 call *0x0(%rip) */
+ /* target address is stored at "next instruction + disp". */
+ target = *(void **)(instr + a->instrlen + disp);
+#else
+ /* ff 15 00 00 00 00 call *0x0 */
+ /* target address is stored at disp. */
+ target = *(void **)disp;
+#endif
+ if (!target)
+ target = bug;
+
+ /* (BUG_func - .) + (target - BUG_func) := target - . */
+ *(s32 *)(insn_buff + 1) += target - bug;
+
+ if (target == &nop_func)
+ return 0;
+
+ return 5;
+}
+
/*
* Replace instructions with better alternatives for this CPU type. This runs
* before SMP is initialized to avoid SMP problems with self modifying code.
@@ -462,6 +503,12 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
memcpy(insn_buff, replacement, a->replacementlen);
insn_buff_sz = a->replacementlen;
+ if (a->flags & ALT_FLAG_DIRECT_CALL) {
+ insn_buff_sz = alt_replace_call(instr, insn_buff, a);
+ if (insn_buff_sz < 0)
+ continue;
+ }
+
for (; insn_buff_sz < a->instrlen; insn_buff_sz++)
insn_buff[insn_buff_sz] = 0x90;
--
2.35.3
On Wed, Nov 29, 2023 at 02:33:30PM +0100, Juergen Gross wrote:
> In order to prepare replacing of paravirt patching with alternative
> patching, add the capability to replace an indirect call with a direct
> one to alternative patching.
>
> This is done via a new flag ALT_FLAG_CALL as the target of the call
> instruction needs to be evaluated using the value of the location
> addressed by the indirect call. For convenience add a macro for a
> default call instruction. In case it is being used without the new
> flag being set, it will result in a BUG() when being executed. As in
> most cases the feature used will be X86_FEATURE_ALWAYS add another
> macro ALT_CALL_ALWAYS usable for the flags parameter of the ALTERNATIVE
> macros.
>
> For a complete replacement handle the special cases of calling a nop
> function and an indirect call of NULL the same way as paravirt does.
>
> Signed-off-by: Juergen Gross <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
That SOB chain basically says that you're PeterZ and you're sending this
patch. :)
Are you trying to say that he co-developed it or suggested it or
Originally-by?
Documentation/process/submitting-patches.rst has it all.
Also, what I did ontop of this is below as we must dump the full flags
now with the debug output since you're adding more flags than ALT_NOT.
Also, the naked magic numbers need explanation.
Please include into your next submission.
Thx.
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index e7e53b9e801b..888205234f15 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -395,6 +395,9 @@ noinstr void BUG_func(void)
}
EXPORT_SYMBOL_GPL(BUG_func);
+#define CALL_RIP_REL_PREFIX 0xff
+#define CALL_RIP_REL_MODRM 0x15
+
/*
* Rewrite the "call BUG_func" replacement to point to the target of the
* indirect pv_ops call "call *disp(%ip)".
@@ -409,11 +412,14 @@ static int alt_replace_call(u8 *instr, u8 *insn_buff, struct alt_instr *a)
BUG();
}
- if (a->instrlen != 6 || instr[0] != 0xff || instr[1] != 0x15) {
+ if (a->instrlen != 6 ||
+ instr[0] != CALL_RIP_REL_PREFIX ||
+ instr[1] != CALL_RIP_REL_MODRM) {
pr_err("ALT_FLAG_DIRECT_CALL set for unrecognized indirect call\n");
BUG();
}
+ /* Skip CALL_RIP_REL_PREFIX and CALL_RIP_REL_MODRM */
disp = *(s32 *)(instr + 2);
#ifdef CONFIG_X86_64
/* ff 15 00 00 00 00 call *0x0(%rip) */
@@ -493,12 +499,11 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
continue;
}
- DPRINTK(ALT, "feat: %s%d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d)",
- (a->flags & ALT_FLAG_NOT) ? "!" : "",
+ DPRINTK(ALT, "feat: %d32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
a->cpuid >> 5,
a->cpuid & 0x1f,
instr, instr, a->instrlen,
- replacement, a->replacementlen);
+ replacement, a->replacementlen, a->flags);
memcpy(insn_buff, replacement, a->replacementlen);
insn_buff_sz = a->replacementlen;
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
The following commit has been merged into the x86/paravirt branch of tip:
Commit-ID: 7991ed43587d1106315208cc289c851d6915d4a3
Gitweb: https://git.kernel.org/tip/7991ed43587d1106315208cc289c851d6915d4a3
Author: Borislav Petkov (AMD) <[email protected]>
AuthorDate: Sat, 30 Dec 2023 12:20:04 +01:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Sat, 30 Dec 2023 12:25:55 +01:00
x86/alternative: Correct feature bit debug output
In
https://lore.kernel.org/r/20231206110636.GBZXBVvCWj2IDjVk4c@fat_crate.local
I wanted to adjust the alternative patching debug output to the new
changes introduced by
da0fe6e68e10 ("x86/alternative: Add indirect call patching")
but removed the '*' which denotes the ->x86_capability word. The correct
output should be, for example:
[ 0.230071] SMP alternatives: feat: 11*32+15, old: (entry_SYSCALL_64_after_hwframe+0x5a/0x77 (ffffffff81c000c2) len: 16), repl: (ffffffff89ae896a, len: 5) flags: 0x0
while the incorrect one says "... 1132+15" currently.
Add back the '*'.
Fixes: da0fe6e68e10 ("x86/alternative: Add indirect call patching")
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/r/20231206110636.GBZXBVvCWj2IDjVk4c@fat_crate.local
---
arch/x86/kernel/alternative.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index f26983a..f7ea108 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -498,7 +498,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
continue;
}
- DPRINTK(ALT, "feat: %d32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
+ DPRINTK(ALT, "feat: %d*32+%d, old: (%pS (%px) len: %d), repl: (%px, len: %d) flags: 0x%x",
a->cpuid >> 5,
a->cpuid & 0x1f,
instr, instr, a->instrlen,