Fixed several optkprobe issues, mainly related to the x86 architecture.
Yang Jihong (3):
kprobes: Fixed probe nodes not correctly removed when forcibly
unoptimized
x86/kprobes: Fix __recover_optprobed_insn check optimizing logic
x86/kprobes: Fix arch_check_optimized_kprobe check within
optimized_kprobe range
arch/x86/kernel/kprobes/opt.c | 6 +++---
include/linux/kprobes.h | 1 +
kernel/kprobes.c | 7 +++----
3 files changed, 7 insertions(+), 7 deletions(-)
--
2.30.GIT
When arch_prepare_optimized_kprobe calculating jump destination address,
it copies original instructions from jmp-optimized kprobe (see
__recover_optprobed_insn), and calculated based on length of original
instruction.
arch_check_optimized_kprobe does not check KPROBE_FLAG_OPTIMATED when
checking whether jmp-optimized kprobe exists.
As a result, setup_detour_execution may jump to a range that has been
overwritten by jump destination address, resulting in an inval opcode error.
For example, assume that register two kprobes whose addresses are
<func+9> and <func+11> in "func" function.
The original code of "func" function is as follows:
0xffffffff816cb5e9 <+9>: push %r12
0xffffffff816cb5eb <+11>: xor %r12d,%r12d
0xffffffff816cb5ee <+14>: test %rdi,%rdi
0xffffffff816cb5f1 <+17>: setne %r12b
0xffffffff816cb5f5 <+21>: push %rbp
1.Register the kprobe for <func+11>, assume that is kp1, corresponding optimized_kprobe is op1.
After the optimization, "func" code changes to:
0xffffffff816cc079 <+9>: push %r12
0xffffffff816cc07b <+11>: jmp 0xffffffffa0210000
0xffffffff816cc080 <+16>: incl 0xf(%rcx)
0xffffffff816cc083 <+19>: xchg %eax,%ebp
0xffffffff816cc084 <+20>: (bad)
0xffffffff816cc085 <+21>: push %rbp
Now op1->flags == KPROBE_FLAG_OPTIMATED;
2. Register the kprobe for <func+9>, assume that is kp2, corresponding optimized_kprobe is op2.
register_kprobe(kp2)
register_aggr_kprobe
alloc_aggr_kprobe
__prepare_optimized_kprobe
arch_prepare_optimized_kprobe
__recover_optprobed_insn // copy original bytes from kp1->optinsn.copied_insn,
// jump address = <func+14>
3. disable kp1:
disable_kprobe(kp1)
__disable_kprobe
...
if (p == orig_p || aggr_kprobe_disabled(orig_p)) {
ret = disarm_kprobe(orig_p, true) // add op1 in unoptimizing_list, not unoptimized
orig_p->flags |= KPROBE_FLAG_DISABLED; // op1->flags == KPROBE_FLAG_OPTIMATED | KPROBE_FLAG_DISABLED
...
4. unregister kp2
__unregister_kprobe_top
...
if (!kprobe_disabled(ap) && !kprobes_all_disarmed) {
optimize_kprobe(op)
...
if (arch_check_optimized_kprobe(op) < 0) // because op1 has KPROBE_FLAG_DISABLED, here not return
return;
p->kp.flags |= KPROBE_FLAG_OPTIMIZED; // now op2 has KPROBE_FLAG_OPTIMIZED
}
"func" code now is:
0xffffffff816cc079 <+9>: int3
0xffffffff816cc07a <+10>: push %rsp
0xffffffff816cc07b <+11>: jmp 0xffffffffa0210000
0xffffffff816cc080 <+16>: incl 0xf(%rcx)
0xffffffff816cc083 <+19>: xchg %eax,%ebp
0xffffffff816cc084 <+20>: (bad)
0xffffffff816cc085 <+21>: push %rbp
5. if call "func", int3 handler call setup_detour_execution:
if (p->flags & KPROBE_FLAG_OPTIMIZED) {
...
regs->ip = (unsigned long)op->optinsn.insn + TMPL_END_IDX;
...
}
The code for the destination address is
0xffffffffa021072c: push %r12
0xffffffffa021072e: xor %r12d,%r12d
0xffffffffa0210731: jmp 0xffffffff816cb5ee <func+14>
However, <func+14> is not a valid start instruction address. As a result, an error occurs.
Signed-off-by: Yang Jihong <[email protected]>
---
arch/x86/kernel/kprobes/opt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 3718d6863555..e6d9bd038401 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -353,7 +353,7 @@ int arch_check_optimized_kprobe(struct optimized_kprobe *op)
for (i = 1; i < op->optinsn.size; i++) {
p = get_kprobe(op->kp.addr + i);
- if (p && !kprobe_disabled(p))
+ if (p && (!kprobe_disabled(p) || kprobe_optimized(p)))
return -EEXIST;
}
--
2.30.GIT
Since the following commit:
commit f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
modified the update timing of the KPROBE_FLAG_OPTIMIZED, a optimized_kprobe
may be in the optimizing or unoptimizing state when op.kp->flags
has KPROBE_FLAG_OPTIMIZED and op->list is not empty.
The __recover_optprobed_insn check logic is incorrect, a kprobe in the
unoptimizing state may be incorrectly determined as unoptimizing.
As a result, incorrect instructions are copied.
The optprobe_queued_unopt function needs to be exported for invoking in
arch directory.
Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
Signed-off-by: Yang Jihong <[email protected]>
---
arch/x86/kernel/kprobes/opt.c | 4 ++--
include/linux/kprobes.h | 1 +
kernel/kprobes.c | 2 +-
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index e57e07b0edb6..3718d6863555 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -46,8 +46,8 @@ unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr)
/* This function only handles jump-optimized kprobe */
if (kp && kprobe_optimized(kp)) {
op = container_of(kp, struct optimized_kprobe, kp);
- /* If op->list is not empty, op is under optimizing */
- if (list_empty(&op->list))
+ /* If op is [un]optimized or under unoptimizing */
+ if (list_empty(&op->list) || optprobe_queued_unopt(op))
goto found;
}
}
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index a0b92be98984..ab39285f71a6 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -378,6 +378,7 @@ extern void opt_pre_handler(struct kprobe *p, struct pt_regs *regs);
DEFINE_INSN_CACHE_OPS(optinsn);
extern void wait_for_kprobe_optimizer(void);
+bool optprobe_queued_unopt(struct optimized_kprobe *op);
#else /* !CONFIG_OPTPROBES */
static inline void wait_for_kprobe_optimizer(void) { }
#endif /* CONFIG_OPTPROBES */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 0730e595f4c1..bf60eb26c873 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -661,7 +661,7 @@ void wait_for_kprobe_optimizer(void)
mutex_unlock(&kprobe_mutex);
}
-static bool optprobe_queued_unopt(struct optimized_kprobe *op)
+bool optprobe_queued_unopt(struct optimized_kprobe *op)
{
struct optimized_kprobe *_op;
--
2.30.GIT
When unoptimize_kprobe forcibly unoptimize the kprobe, simply queue it in
the freeing_list, and do_free_cleaned_kprobes directly reclaims the kprobe
if unoptimizing_list is empty (see do_unoptimize_kprobes), which may cause
WARN or UAF problems.
The specific scenarios are as follows:
Thread1
arm_kprobe(p)
mutex_lock(&kprobe_mutex)
__arm_kprobe(kp)
p = get_optimized_kprobe(p->addr)
if (unlikely(_p))
unoptimize_kprobe(_p, true) // now _p is queued in freeing_list
mutex_unlock(&kprobe_mutex)
Thread2
kprobe_optimizer
mutex_lock(&kprobe_mutex)
do_unoptimize_kprobes
if (list_empty(&unoptimizing_list))
return; // here directly returned and does not process freeing_list.
...
do_free_cleaned_kprobes
foreach op in freeing_list:
WARN_ON_ONCE(!kprobe_unused(&op->kp)) // WANR will be triggered here.
free_aggr_kprobe((&op->kp) // Delete op->kp directly, if access hash
// list later, UAF problem will be triggered.
mutex_unlock(&kprobe_mutex)
The freeing_list needs to be processed in do_unoptimize_kprobes regardless
of whether unoptimizing_list is empty.
Signed-off-by: Yang Jihong <[email protected]>
---
kernel/kprobes.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 1c18ecf9f98b..0730e595f4c1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -556,10 +556,9 @@ static void do_unoptimize_kprobes(void)
lockdep_assert_cpus_held();
/* Unoptimization must be done anytime */
- if (list_empty(&unoptimizing_list))
- return;
+ if (!list_empty(&unoptimizing_list))
+ arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list);
- arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list);
/* Loop on 'freeing_list' for disarming */
list_for_each_entry_safe(op, tmp, &freeing_list, list) {
/* Switching from detour code to origin */
--
2.30.GIT
Hi Yang,
Thanks for reporting, but maybe this is a part of following fix.
https://lore.kernel.org/all/167448024501.3253718.13037333683110512967.stgit@devnote3/
Can you confirm that fixes the same issue?
Thank you,
On Wed, 15 Feb 2023 19:54:28 +0800
Yang Jihong <[email protected]> wrote:
> When unoptimize_kprobe forcibly unoptimize the kprobe, simply queue it in
> the freeing_list, and do_free_cleaned_kprobes directly reclaims the kprobe
> if unoptimizing_list is empty (see do_unoptimize_kprobes), which may cause
> WARN or UAF problems.
>
> The specific scenarios are as follows:
>
> Thread1
> arm_kprobe(p)
> mutex_lock(&kprobe_mutex)
> __arm_kprobe(kp)
> p = get_optimized_kprobe(p->addr)
> if (unlikely(_p))
> unoptimize_kprobe(_p, true) // now _p is queued in freeing_list
> mutex_unlock(&kprobe_mutex)
>
> Thread2
> kprobe_optimizer
> mutex_lock(&kprobe_mutex)
> do_unoptimize_kprobes
> if (list_empty(&unoptimizing_list))
> return; // here directly returned and does not process freeing_list.
> ...
> do_free_cleaned_kprobes
> foreach op in freeing_list:
> WARN_ON_ONCE(!kprobe_unused(&op->kp)) // WANR will be triggered here.
> free_aggr_kprobe((&op->kp) // Delete op->kp directly, if access hash
> // list later, UAF problem will be triggered.
> mutex_unlock(&kprobe_mutex)
>
> The freeing_list needs to be processed in do_unoptimize_kprobes regardless
> of whether unoptimizing_list is empty.
>
> Signed-off-by: Yang Jihong <[email protected]>
> ---
> kernel/kprobes.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 1c18ecf9f98b..0730e595f4c1 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -556,10 +556,9 @@ static void do_unoptimize_kprobes(void)
> lockdep_assert_cpus_held();
>
> /* Unoptimization must be done anytime */
> - if (list_empty(&unoptimizing_list))
> - return;
> + if (!list_empty(&unoptimizing_list))
> + arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list);
>
> - arch_unoptimize_kprobes(&unoptimizing_list, &freeing_list);
> /* Loop on 'freeing_list' for disarming */
> list_for_each_entry_safe(op, tmp, &freeing_list, list) {
> /* Switching from detour code to origin */
> --
> 2.30.GIT
>
--
Masami Hiramatsu (Google) <[email protected]>
On Wed, 15 Feb 2023 19:54:29 +0800
Yang Jihong <[email protected]> wrote:
> Since the following commit:
>
> commit f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
>
> modified the update timing of the KPROBE_FLAG_OPTIMIZED, a optimized_kprobe
> may be in the optimizing or unoptimizing state when op.kp->flags
> has KPROBE_FLAG_OPTIMIZED and op->list is not empty.
>
> The __recover_optprobed_insn check logic is incorrect, a kprobe in the
> unoptimizing state may be incorrectly determined as unoptimizing.
> As a result, incorrect instructions are copied.
Ah, good catch!
>
> The optprobe_queued_unopt function needs to be exported for invoking in
> arch directory.
>
> Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
Cc: [email protected]
> Signed-off-by: Yang Jihong <[email protected]>
> ---
> arch/x86/kernel/kprobes/opt.c | 4 ++--
> include/linux/kprobes.h | 1 +
> kernel/kprobes.c | 2 +-
> 3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
> index e57e07b0edb6..3718d6863555 100644
> --- a/arch/x86/kernel/kprobes/opt.c
> +++ b/arch/x86/kernel/kprobes/opt.c
> @@ -46,8 +46,8 @@ unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr)
> /* This function only handles jump-optimized kprobe */
> if (kp && kprobe_optimized(kp)) {
> op = container_of(kp, struct optimized_kprobe, kp);
> - /* If op->list is not empty, op is under optimizing */
> - if (list_empty(&op->list))
> + /* If op is [un]optimized or under unoptimizing */
Hmm, this is a bit confusing comment. If it is unoptimized, the kprobe_optimized() returns false.
Thus the comment should be /* If op is optimized or under unoptimizing */.
Thank you,
> + if (list_empty(&op->list) || optprobe_queued_unopt(op))
> goto found;
> }
> }
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index a0b92be98984..ab39285f71a6 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -378,6 +378,7 @@ extern void opt_pre_handler(struct kprobe *p, struct pt_regs *regs);
> DEFINE_INSN_CACHE_OPS(optinsn);
>
> extern void wait_for_kprobe_optimizer(void);
> +bool optprobe_queued_unopt(struct optimized_kprobe *op);
> #else /* !CONFIG_OPTPROBES */
> static inline void wait_for_kprobe_optimizer(void) { }
> #endif /* CONFIG_OPTPROBES */
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 0730e595f4c1..bf60eb26c873 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -661,7 +661,7 @@ void wait_for_kprobe_optimizer(void)
> mutex_unlock(&kprobe_mutex);
> }
>
> -static bool optprobe_queued_unopt(struct optimized_kprobe *op)
> +bool optprobe_queued_unopt(struct optimized_kprobe *op)
> {
> struct optimized_kprobe *_op;
>
> --
> 2.30.GIT
>
--
Masami Hiramatsu (Google) <[email protected]>
On Wed, 15 Feb 2023 19:54:30 +0800
Yang Jihong <[email protected]> wrote:
> When arch_prepare_optimized_kprobe calculating jump destination address,
> it copies original instructions from jmp-optimized kprobe (see
> __recover_optprobed_insn), and calculated based on length of original
> instruction.
>
> arch_check_optimized_kprobe does not check KPROBE_FLAG_OPTIMATED when
> checking whether jmp-optimized kprobe exists.
> As a result, setup_detour_execution may jump to a range that has been
> overwritten by jump destination address, resulting in an inval opcode error.
OK, good catch !! I missed "delayed unoptimization" case here too.
>
> For example, assume that register two kprobes whose addresses are
> <func+9> and <func+11> in "func" function.
> The original code of "func" function is as follows:
>
> 0xffffffff816cb5e9 <+9>: push %r12
> 0xffffffff816cb5eb <+11>: xor %r12d,%r12d
> 0xffffffff816cb5ee <+14>: test %rdi,%rdi
> 0xffffffff816cb5f1 <+17>: setne %r12b
> 0xffffffff816cb5f5 <+21>: push %rbp
>
> 1.Register the kprobe for <func+11>, assume that is kp1, corresponding optimized_kprobe is op1.
> After the optimization, "func" code changes to:
>
> 0xffffffff816cc079 <+9>: push %r12
> 0xffffffff816cc07b <+11>: jmp 0xffffffffa0210000
> 0xffffffff816cc080 <+16>: incl 0xf(%rcx)
> 0xffffffff816cc083 <+19>: xchg %eax,%ebp
> 0xffffffff816cc084 <+20>: (bad)
> 0xffffffff816cc085 <+21>: push %rbp
>
> Now op1->flags == KPROBE_FLAG_OPTIMATED;
>
> 2. Register the kprobe for <func+9>, assume that is kp2, corresponding optimized_kprobe is op2.
>
> register_kprobe(kp2)
> register_aggr_kprobe
> alloc_aggr_kprobe
> __prepare_optimized_kprobe
> arch_prepare_optimized_kprobe
> __recover_optprobed_insn // copy original bytes from kp1->optinsn.copied_insn,
> // jump address = <func+14>
>
> 3. disable kp1:
>
> disable_kprobe(kp1)
> __disable_kprobe
> ...
> if (p == orig_p || aggr_kprobe_disabled(orig_p)) {
> ret = disarm_kprobe(orig_p, true) // add op1 in unoptimizing_list, not unoptimized
> orig_p->flags |= KPROBE_FLAG_DISABLED; // op1->flags == KPROBE_FLAG_OPTIMATED | KPROBE_FLAG_DISABLED
> ...
>
> 4. unregister kp2
> __unregister_kprobe_top
> ...
> if (!kprobe_disabled(ap) && !kprobes_all_disarmed) {
> optimize_kprobe(op)
> ...
> if (arch_check_optimized_kprobe(op) < 0) // because op1 has KPROBE_FLAG_DISABLED, here not return
> return;
> p->kp.flags |= KPROBE_FLAG_OPTIMIZED; // now op2 has KPROBE_FLAG_OPTIMIZED
> }
>
> "func" code now is:
>
> 0xffffffff816cc079 <+9>: int3
> 0xffffffff816cc07a <+10>: push %rsp
> 0xffffffff816cc07b <+11>: jmp 0xffffffffa0210000
> 0xffffffff816cc080 <+16>: incl 0xf(%rcx)
> 0xffffffff816cc083 <+19>: xchg %eax,%ebp
> 0xffffffff816cc084 <+20>: (bad)
> 0xffffffff816cc085 <+21>: push %rbp
>
> 5. if call "func", int3 handler call setup_detour_execution:
>
> if (p->flags & KPROBE_FLAG_OPTIMIZED) {
> ...
> regs->ip = (unsigned long)op->optinsn.insn + TMPL_END_IDX;
> ...
> }
>
> The code for the destination address is
>
> 0xffffffffa021072c: push %r12
> 0xffffffffa021072e: xor %r12d,%r12d
> 0xffffffffa0210731: jmp 0xffffffff816cb5ee <func+14>
>
> However, <func+14> is not a valid start instruction address. As a result, an error occurs.
OK, it has been introduced by the same commit as previous one. (delayed unoptimization)
>
> Signed-off-by: Yang Jihong <[email protected]>
> ---
> arch/x86/kernel/kprobes/opt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
> index 3718d6863555..e6d9bd038401 100644
> --- a/arch/x86/kernel/kprobes/opt.c
> +++ b/arch/x86/kernel/kprobes/opt.c
> @@ -353,7 +353,7 @@ int arch_check_optimized_kprobe(struct optimized_kprobe *op)
>
> for (i = 1; i < op->optinsn.size; i++) {
> p = get_kprobe(op->kp.addr + i);
> - if (p && !kprobe_disabled(p))
> + if (p && (!kprobe_disabled(p) || kprobe_optimized(p)))
Hmm, can you rewrite this with kprobe_disarmed() instead of kprobe_disabled()?
Since this is checking there are any other kprobes are "armed" on the address
where it will be replaced by jump. So it is natural to use "disarmed" check.
Thank you,
> return -EEXIST;
> }
>
> --
> 2.30.GIT
>
--
Masami Hiramatsu (Google) <[email protected]>
Hello Masami,
On 2023/2/15 22:55, Masami Hiramatsu (Google) wrote:
> Hi Yang,
>
> Thanks for reporting, but maybe this is a part of following fix.
>
> https://lore.kernel.org/all/167448024501.3253718.13037333683110512967.stgit@devnote3/
>
> Can you confirm that fixes the same issue?
Yes, I've tested and confirmed that the patch you mentioned above fixes
the same issue.
Will remove it in next version.
Thanks
Yang.
Hello Masami,
On 2023/2/15 23:08, Masami Hiramatsu (Google) wrote:
> On Wed, 15 Feb 2023 19:54:29 +0800
> Yang Jihong <[email protected]> wrote:
>
>> Since the following commit:
>>
>> commit f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
>>
>> modified the update timing of the KPROBE_FLAG_OPTIMIZED, a optimized_kprobe
>> may be in the optimizing or unoptimizing state when op.kp->flags
>> has KPROBE_FLAG_OPTIMIZED and op->list is not empty.
>>
>> The __recover_optprobed_insn check logic is incorrect, a kprobe in the
>> unoptimizing state may be incorrectly determined as unoptimizing.
>> As a result, incorrect instructions are copied.
>
> Ah, good catch!
>
>>
>> The optprobe_queued_unopt function needs to be exported for invoking in
>> arch directory.
>>
>> Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after unoptimizing code")
>
> Cc: [email protected]
OK, will add in next version.
>
>> Signed-off-by: Yang Jihong <[email protected]>
>> ---
>> arch/x86/kernel/kprobes/opt.c | 4 ++--
>> include/linux/kprobes.h | 1 +
>> kernel/kprobes.c | 2 +-
>> 3 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
>> index e57e07b0edb6..3718d6863555 100644
>> --- a/arch/x86/kernel/kprobes/opt.c
>> +++ b/arch/x86/kernel/kprobes/opt.c
>> @@ -46,8 +46,8 @@ unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr)
>> /* This function only handles jump-optimized kprobe */
>> if (kp && kprobe_optimized(kp)) {
>> op = container_of(kp, struct optimized_kprobe, kp);
>> - /* If op->list is not empty, op is under optimizing */
>> - if (list_empty(&op->list))
>> + /* If op is [un]optimized or under unoptimizing */
>
> Hmm, this is a bit confusing comment. If it is unoptimized, the kprobe_optimized() returns false.
> Thus the comment should be /* If op is optimized or under unoptimizing */.
>
OK, will fix in next version.
Thanks,
Yang.
Hello Masami,
On 2023/2/15 23:48, Masami Hiramatsu (Google) wrote:
> On Wed, 15 Feb 2023 19:54:30 +0800
> Yang Jihong <[email protected]> wrote:
>
>> When arch_prepare_optimized_kprobe calculating jump destination address,
>> it copies original instructions from jmp-optimized kprobe (see
>> __recover_optprobed_insn), and calculated based on length of original
>> instruction.
>>
>> arch_check_optimized_kprobe does not check KPROBE_FLAG_OPTIMATED when
>> checking whether jmp-optimized kprobe exists.
>> As a result, setup_detour_execution may jump to a range that has been
>> overwritten by jump destination address, resulting in an inval opcode error.
>
> OK, good catch !! I missed "delayed unoptimization" case here too.
>
>>
>> For example, assume that register two kprobes whose addresses are
>> <func+9> and <func+11> in "func" function.
>> The original code of "func" function is as follows:
>>
>> 0xffffffff816cb5e9 <+9>: push %r12
>> 0xffffffff816cb5eb <+11>: xor %r12d,%r12d
>> 0xffffffff816cb5ee <+14>: test %rdi,%rdi
>> 0xffffffff816cb5f1 <+17>: setne %r12b
>> 0xffffffff816cb5f5 <+21>: push %rbp
>>
>> 1.Register the kprobe for <func+11>, assume that is kp1, corresponding optimized_kprobe is op1.
>> After the optimization, "func" code changes to:
>>
>> 0xffffffff816cc079 <+9>: push %r12
>> 0xffffffff816cc07b <+11>: jmp 0xffffffffa0210000
>> 0xffffffff816cc080 <+16>: incl 0xf(%rcx)
>> 0xffffffff816cc083 <+19>: xchg %eax,%ebp
>> 0xffffffff816cc084 <+20>: (bad)
>> 0xffffffff816cc085 <+21>: push %rbp
>>
>> Now op1->flags == KPROBE_FLAG_OPTIMATED;
>>
>> 2. Register the kprobe for <func+9>, assume that is kp2, corresponding optimized_kprobe is op2.
>>
>> register_kprobe(kp2)
>> register_aggr_kprobe
>> alloc_aggr_kprobe
>> __prepare_optimized_kprobe
>> arch_prepare_optimized_kprobe
>> __recover_optprobed_insn // copy original bytes from kp1->optinsn.copied_insn,
>> // jump address = <func+14>
>>
>> 3. disable kp1:
>>
>> disable_kprobe(kp1)
>> __disable_kprobe
>> ...
>> if (p == orig_p || aggr_kprobe_disabled(orig_p)) {
>> ret = disarm_kprobe(orig_p, true) // add op1 in unoptimizing_list, not unoptimized
>> orig_p->flags |= KPROBE_FLAG_DISABLED; // op1->flags == KPROBE_FLAG_OPTIMATED | KPROBE_FLAG_DISABLED
>> ...
>>
>> 4. unregister kp2
>> __unregister_kprobe_top
>> ...
>> if (!kprobe_disabled(ap) && !kprobes_all_disarmed) {
>> optimize_kprobe(op)
>> ...
>> if (arch_check_optimized_kprobe(op) < 0) // because op1 has KPROBE_FLAG_DISABLED, here not return
>> return;
>> p->kp.flags |= KPROBE_FLAG_OPTIMIZED; // now op2 has KPROBE_FLAG_OPTIMIZED
>> }
>>
>> "func" code now is:
>>
>> 0xffffffff816cc079 <+9>: int3
>> 0xffffffff816cc07a <+10>: push %rsp
>> 0xffffffff816cc07b <+11>: jmp 0xffffffffa0210000
>> 0xffffffff816cc080 <+16>: incl 0xf(%rcx)
>> 0xffffffff816cc083 <+19>: xchg %eax,%ebp
>> 0xffffffff816cc084 <+20>: (bad)
>> 0xffffffff816cc085 <+21>: push %rbp
>>
>> 5. if call "func", int3 handler call setup_detour_execution:
>>
>> if (p->flags & KPROBE_FLAG_OPTIMIZED) {
>> ...
>> regs->ip = (unsigned long)op->optinsn.insn + TMPL_END_IDX;
>> ...
>> }
>>
>> The code for the destination address is
>>
>> 0xffffffffa021072c: push %r12
>> 0xffffffffa021072e: xor %r12d,%r12d
>> 0xffffffffa0210731: jmp 0xffffffff816cb5ee <func+14>
>>
>> However, <func+14> is not a valid start instruction address. As a result, an error occurs.
>
> OK, it has been introduced by the same commit as previous one. (delayed unoptimization)
>
OK, will add "Fixes: f66c0447cca1 ("kprobes: Set unoptimized flag after
unoptimizing code")" in next version
In addition, "
Cc: [email protected]" is required, same as the previous patch.
>>
>> Signed-off-by: Yang Jihong <[email protected]>
>> ---
>> arch/x86/kernel/kprobes/opt.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
>> index 3718d6863555..e6d9bd038401 100644
>> --- a/arch/x86/kernel/kprobes/opt.c
>> +++ b/arch/x86/kernel/kprobes/opt.c
>> @@ -353,7 +353,7 @@ int arch_check_optimized_kprobe(struct optimized_kprobe *op)
>>
>> for (i = 1; i < op->optinsn.size; i++) {
>> p = get_kprobe(op->kp.addr + i);
>> - if (p && !kprobe_disabled(p))
>> + if (p && (!kprobe_disabled(p) || kprobe_optimized(p)))
>
> Hmm, can you rewrite this with kprobe_disarmed() instead of kprobe_disabled()?
> Since this is checking there are any other kprobes are "armed" on the address
> where it will be replaced by jump. So it is natural to use "disarmed" check.
>
Yes, It is better to change it to use "kprobe_disarmed", will modify in
next version.
Thanks,
Yang