[v4,2/5] x86/alternative: add indirect call patching
Commit Message
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 <jgross@suse.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
V4:
- 32-bit mode doesn't have %rip relative addressing (kernel test robot)
- define ALT_CALL_INSTR in assembly, too (kernel test robot)
---
arch/x86/include/asm/alternative.h | 9 ++++++
arch/x86/kernel/alternative.c | 45 ++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
Comments
On Mon, Oct 30, 2023 at 03:25:05PM +0100, Juergen Gross wrote:
> + * 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;
> +
> + if (a->replacementlen != 5 || insn_buff[0] != CALL_INSN_OPCODE) {
> + pr_err("Alternative: ALT_FLAG_CALL set for a non-call replacement instruction\n");
No need for the printk prefix.
> + pr_err(" Ignoring the flag for the instruction at %pS (%px)\n", instr, instr);
No ignoring - BUG
> + return 5;
> + }
> +
> + if (a->instrlen != 6 || instr[0] != 0xff || instr[1] != 0x15) {
> + pr_err("Alternative: ALT_FLAG_CALL set for unrecognized indirect call\n");
> + pr_err(" Not replacing the instruction at %pS (%px)\n", instr, instr);
> + return -1;
Ditto.
> + }
> +
> +#ifdef CONFIG_X86_64
> + /* ff 15 00 00 00 00 call *0x0(%rip) */
> + target = *(void **)(instr + a->instrlen + *(s32 *)(instr + 2));
> +#else
> + /* ff 15 00 00 00 00 call *0x0 */
> + target = *(void **)(*(s32 *)(instr + 2));
> +#endif
> + if (!target)
> + target = bug;
> +
> + /* (BUG_func - .) + (target - BUG_func) := target - . */
> + *(s32 *)(insn_buff + 1) += target - bug;
If I squint hard enough, this looks like it is replacing one call with
another. We have a C macro alternative_call() which does exactly that.
Why can't you define an asm version ALTERNATIVE_CALL and use it
instead of using adding a new flag? We have 16 possible ones in total.
Thx.
On Tue, Nov 14, 2023 at 01:37:37PM +0100, Borislav Petkov wrote:
> > +#ifdef CONFIG_X86_64
> > + /* ff 15 00 00 00 00 call *0x0(%rip) */
> > + target = *(void **)(instr + a->instrlen + *(s32 *)(instr + 2));
> > +#else
> > + /* ff 15 00 00 00 00 call *0x0 */
> > + target = *(void **)(*(s32 *)(instr + 2));
> > +#endif
> > + if (!target)
> > + target = bug;
> > +
> > + /* (BUG_func - .) + (target - BUG_func) := target - . */
> > + *(s32 *)(insn_buff + 1) += target - bug;
>
> If I squint hard enough, this looks like it is replacing one call with
> another. We have a C macro alternative_call() which does exactly that.
> Why can't you define an asm version ALTERNATIVE_CALL and use it
> instead of using adding a new flag? We have 16 possible ones in total.
This loads the function target from the pv_ops table. We can't otherwise
do this.
On 14.11.23 13:37, Borislav Petkov wrote:
> On Mon, Oct 30, 2023 at 03:25:05PM +0100, Juergen Gross wrote:
>> + * 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;
>> +
>> + if (a->replacementlen != 5 || insn_buff[0] != CALL_INSN_OPCODE) {
>> + pr_err("Alternative: ALT_FLAG_CALL set for a non-call replacement instruction\n");
>
> No need for the printk prefix.
Okay.
>
>> + pr_err(" Ignoring the flag for the instruction at %pS (%px)\n", instr, instr);
>
> No ignoring - BUG
Okay.
>
>> + return 5;
>> + }
>> +
>> + if (a->instrlen != 6 || instr[0] != 0xff || instr[1] != 0x15) {
>> + pr_err("Alternative: ALT_FLAG_CALL set for unrecognized indirect call\n");
>> + pr_err(" Not replacing the instruction at %pS (%px)\n", instr, instr);
>> + return -1;
>
> Ditto.
Okay.
>
>> + }
>> +
>> +#ifdef CONFIG_X86_64
>> + /* ff 15 00 00 00 00 call *0x0(%rip) */
>> + target = *(void **)(instr + a->instrlen + *(s32 *)(instr + 2));
>> +#else
>> + /* ff 15 00 00 00 00 call *0x0 */
>> + target = *(void **)(*(s32 *)(instr + 2));
>> +#endif
>> + if (!target)
>> + target = bug;
>> +
>> + /* (BUG_func - .) + (target - BUG_func) := target - . */
>> + *(s32 *)(insn_buff + 1) += target - bug;
>
> If I squint hard enough, this looks like it is replacing one call with
> another. We have a C macro alternative_call() which does exactly that.
> Why can't you define an asm version ALTERNATIVE_CALL and use it
> instead of using adding a new flag? We have 16 possible ones in total.
It is replacing an _indirect_ call with a _direct_ one, taking the call target
from the pointer used by the indirect call.
Juergen
On Tue, Nov 14, 2023 at 01:50:28PM +0100, Peter Zijlstra wrote:
> This loads the function target from the pv_ops table. We can't otherwise
> do this.
On Tue, Nov 14, 2023 at 01:56:37PM +0100, Juergen Gross wrote:
> It is replacing an _indirect_ call with a _direct_ one, taking the
> call target from the pointer used by the indirect call.
Then this is not just a ALT_FLAG_CALL. This is something special. The
flag definition needs a better name along with an explanation what it
does, perhaps best with an example from the final vmlinux - not from the
object file:
call *0x0(%rip)
==>
call *0x0
where the offsets haven't been linked in yet.
If this is going into the generic infrastructure, then it better be
explained properly so that other stuff can potentially use it too.
Thx.
On Tue, Nov 14, 2023 at 02:47:15PM +0100, Borislav Petkov wrote:
> On Tue, Nov 14, 2023 at 01:50:28PM +0100, Peter Zijlstra wrote:
> > This loads the function target from the pv_ops table. We can't otherwise
> > do this.
>
> On Tue, Nov 14, 2023 at 01:56:37PM +0100, Juergen Gross wrote:
> > It is replacing an _indirect_ call with a _direct_ one, taking the
> > call target from the pointer used by the indirect call.
>
> Then this is not just a ALT_FLAG_CALL. This is something special. The
> flag definition needs a better name along with an explanation what it
> does, perhaps best with an example from the final vmlinux - not from the
> object file:
>
> call *0x0(%rip)
>
> ==>
>
> call *0x0
>
> where the offsets haven't been linked in yet.
Well, a random absolute address isn't going to be any better or worse
than 0. But perhaps adding the relocation as a comment helps?
ff 15 00 00 00 00 call *0x0(%rip) # R_X86_64_PC32 pv_ops+0x4
into:
e8 00 00 00 00 90 call +0 # R_X86_64_PC32 *(pv_ops+0x04)
> If this is going into the generic infrastructure, then it better be
> explained properly so that other stuff can potentially use it too.
ALT_FLAG_DEREFERENCE_INDIRECT_CALL ?
I'm going to already raise my hand and say that's too long ;-)
On Tue, Nov 14, 2023 at 03:18:33PM +0100, Peter Zijlstra wrote:
> Well, a random absolute address isn't going to be any better or worse
> than 0. But perhaps adding the relocation as a comment helps?
>
>
> ff 15 00 00 00 00 call *0x0(%rip) # R_X86_64_PC32 pv_ops+0x4
> into:
> e8 00 00 00 00 90 call +0 # R_X86_64_PC32 *(pv_ops+0x04)
A bit better, yeah.
> ALT_FLAG_DEREFERENCE_INDIRECT_CALL ?
>
> I'm going to already raise my hand and say that's too long ;-)
To your own suggestion? :-P
ALT_FLAG_DIRECT_CALL simply, I guess, along with an explanation.
Meaning, this flag tells the alternatives to produce a direct call.
Thx.
On 14.11.23 15:18, Peter Zijlstra wrote:
> On Tue, Nov 14, 2023 at 02:47:15PM +0100, Borislav Petkov wrote:
>> On Tue, Nov 14, 2023 at 01:50:28PM +0100, Peter Zijlstra wrote:
>>> This loads the function target from the pv_ops table. We can't otherwise
>>> do this.
>>
>> On Tue, Nov 14, 2023 at 01:56:37PM +0100, Juergen Gross wrote:
>>> It is replacing an _indirect_ call with a _direct_ one, taking the
>>> call target from the pointer used by the indirect call.
>>
>> Then this is not just a ALT_FLAG_CALL. This is something special. The
>> flag definition needs a better name along with an explanation what it
>> does, perhaps best with an example from the final vmlinux - not from the
>> object file:
>>
>> call *0x0(%rip)
>>
>> ==>
>>
>> call *0x0
>>
>> where the offsets haven't been linked in yet.
>
> Well, a random absolute address isn't going to be any better or worse
> than 0. But perhaps adding the relocation as a comment helps?
>
>
> ff 15 00 00 00 00 call *0x0(%rip) # R_X86_64_PC32 pv_ops+0x4
> into:
> e8 00 00 00 00 90 call +0 # R_X86_64_PC32 *(pv_ops+0x04)
>
>
>> If this is going into the generic infrastructure, then it better be
>> explained properly so that other stuff can potentially use it too.
>
> ALT_FLAG_DEREFERENCE_INDIRECT_CALL ?
ALT_FLAG_MAKE_CALL_DIRECT ?
>
> I'm going to already raise my hand and say that's too long ;-)
I think the length of the name doesn't matter that much, as it is used only
in the patching code and via the ALT_CALL() macro (at least as long as Boris
doesn't ask me to change the macro name, too).
Juergen
On Mon, Oct 30, 2023 at 03:25:05PM +0100, Juergen Gross wrote:
> +#ifdef CONFIG_X86_64
> + /* ff 15 00 00 00 00 call *0x0(%rip) */
> + target = *(void **)(instr + a->instrlen + *(s32 *)(instr + 2));
> +#else
> + /* ff 15 00 00 00 00 call *0x0 */
> + target = *(void **)(*(s32 *)(instr + 2));
Yeah, let's document those a bit better. Either with comments above or
as Peter suggests:
/* Add 2 to skip opcode and ModRM byte: */
disp32 = *(s32 *)(instr + 2);
rip_rela_ptr = (void **)(instr + a->instrlen + disp32);
target = *rip_rela_ptr;
so that it is crystal clear what we're doing here.
Thx.
On 14.11.23 16:06, Borislav Petkov wrote:
> On Mon, Oct 30, 2023 at 03:25:05PM +0100, Juergen Gross wrote:
>> +#ifdef CONFIG_X86_64
>> + /* ff 15 00 00 00 00 call *0x0(%rip) */
>> + target = *(void **)(instr + a->instrlen + *(s32 *)(instr + 2));
>> +#else
>> + /* ff 15 00 00 00 00 call *0x0 */
>> + target = *(void **)(*(s32 *)(instr + 2));
>
> Yeah, let's document those a bit better. Either with comments above or
> as Peter suggests:
>
> /* Add 2 to skip opcode and ModRM byte: */
> disp32 = *(s32 *)(instr + 2);
>
> rip_rela_ptr = (void **)(instr + a->instrlen + disp32);
> target = *rip_rela_ptr;
>
> so that it is crystal clear what we're doing here.
Okay.
Juergen
@@ -10,6 +10,9 @@
#define ALT_FLAG_NOT (1 << 0)
#define ALT_NOT(feature) ((ALT_FLAG_NOT << ALT_FLAGS_SHIFT) | (feature))
+#define ALT_FLAG_CALL (1 << 1)
+#define ALT_CALL(feature) ((ALT_FLAG_CALL << ALT_FLAGS_SHIFT) | (feature))
+#define ALT_CALL_ALWAYS ALT_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
@@ -395,6 +395,45 @@ 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;
+
+ if (a->replacementlen != 5 || insn_buff[0] != CALL_INSN_OPCODE) {
+ pr_err("Alternative: ALT_FLAG_CALL set for a non-call replacement instruction\n");
+ pr_err(" Ignoring the flag for the instruction at %pS (%px)\n", instr, instr);
+ return 5;
+ }
+
+ if (a->instrlen != 6 || instr[0] != 0xff || instr[1] != 0x15) {
+ pr_err("Alternative: ALT_FLAG_CALL set for unrecognized indirect call\n");
+ pr_err(" Not replacing the instruction at %pS (%px)\n", instr, instr);
+ return -1;
+ }
+
+#ifdef CONFIG_X86_64
+ /* ff 15 00 00 00 00 call *0x0(%rip) */
+ target = *(void **)(instr + a->instrlen + *(s32 *)(instr + 2));
+#else
+ /* ff 15 00 00 00 00 call *0x0 */
+ target = *(void **)(*(s32 *)(instr + 2));
+#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 +501,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_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;