[PATCH 0/3] kprobes: Fix issues related to optkprobe

[PATCH 0/3] kprobes: Fix issues related to optkprobe

[Yang Jihong]

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

[PATCH 1/3] kprobes: Fixed probe nodes not correctly removed when forcibly unoptimized

[Yang Jihong]

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 <yangjihong1@huawei.com>
---
 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

[PATCH 2/3] x86/kprobes: Fix __recover_optprobed_insn check optimizing logic

[Yang Jihong]

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 <yangjihong1@huawei.com>
---
 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

[PATCH 3/3] x86/kprobes: Fix arch_check_optimized_kprobe check within optimized_kprobe range

[Yang Jihong]

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 <yangjihong1@huawei.com>
---
 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

Re: [PATCH 1/3] kprobes: Fixed probe nodes not correctly removed when forcibly unoptimized

[Masami Hiramatsu]

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 <yangjihong1@huawei.com> 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 <yangjihong1@huawei.com>
> ---
>  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) <mhiramat@kernel.org>

Re: [PATCH 2/3] x86/kprobes: Fix __recover_optprobed_insn check optimizing logic

[Masami Hiramatsu]

On Wed, 15 Feb 2023 19:54:29 +0800
Yang Jihong <yangjihong1@huawei.com> 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: stable@vger.kernel.org

> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> ---
>  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) <mhiramat@kernel.org>

Re: [PATCH 3/3] x86/kprobes: Fix arch_check_optimized_kprobe check within optimized_kprobe range

[Masami Hiramatsu]

On Wed, 15 Feb 2023 19:54:30 +0800
Yang Jihong <yangjihong1@huawei.com> 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 <yangjihong1@huawei.com>
> ---
>  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) <mhiramat@kernel.org>

Re: [PATCH 1/3] kprobes: Fixed probe nodes not correctly removed when forcibly unoptimized

[Yang Jihong]

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.

Re: [PATCH 2/3] x86/kprobes: Fix __recover_optprobed_insn check optimizing logic

[Yang Jihong]

Hello Masami,

On 2023/2/15 23:08, Masami Hiramatsu (Google) wrote:
> On Wed, 15 Feb 2023 19:54:29 +0800
> Yang Jihong <yangjihong1@huawei.com> 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: stable@vger.kernel.org

OK, will add in next version.

> 
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   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.

Re: [PATCH 3/3] x86/kprobes: Fix arch_check_optimized_kprobe check within optimized_kprobe range

[Yang Jihong]

Hello Masami,

On 2023/2/15 23:48, Masami Hiramatsu (Google) wrote:
> On Wed, 15 Feb 2023 19:54:30 +0800
> Yang Jihong <yangjihong1@huawei.com> 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: stable@vger.kernel.org" is required, same as the previous patch.

>>
>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
>> ---
>>   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