diff mbox series

[5.10] kprobes/x86: Fix kprobe debug exception handling logic

Message ID 20230630020845.227939-1-lihuafei1@huawei.com
State New
Headers
Series [5.10] kprobes/x86: Fix kprobe debug exception handling logic |

Commit Message

Li Huafei June 30, 2023, 2:08 a.m. UTC 
  We get the following crash caused by a null pointer access:

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 ...
 RIP: 0010:resume_execution+0x35/0x190
 ...
 Call Trace:
  <#DB>
  kprobe_debug_handler+0x41/0xd0
  exc_debug+0xe5/0x1b0
  asm_exc_debug+0x19/0x30
 RIP: 0010:copy_from_kernel_nofault.part.0+0x55/0xc0
 ...
  </#DB>
  process_fetch_insn+0xfb/0x720
  kprobe_trace_func+0x199/0x2c0
  ? kernel_clone+0x5/0x2f0
  kprobe_dispatcher+0x3d/0x60
  aggr_pre_handler+0x40/0x80
  ? kernel_clone+0x1/0x2f0
  kprobe_ftrace_handler+0x82/0xf0
  ? __se_sys_clone+0x65/0x90
  ftrace_ops_assist_func+0x86/0x110
  ? rcu_nocb_try_bypass+0x1f3/0x370
  0xffffffffc07e60c8
  ? kernel_clone+0x1/0x2f0
  kernel_clone+0x5/0x2f0

The analysis reveals that kprobe and hardware breakpoints conflict in
the use of debug exceptions.

If we set a hardware breakpoint on a memory address and also have a
kprobe event to fetch the memory at this address. Then when kprobe
triggers, it goes to read the memory and triggers hardware breakpoint
monitoring. This time, since kprobe handles debug exceptions earlier
than hardware breakpoints, it will cause kprobe to incorrectly assume
that the exception is a kprobe trigger.

Notice that after the mainline commit 6256e668b7af ("x86/kprobes: Use
int3 instead of debug trap for single-step"), kprobe no longer uses
debug trap, avoiding the conflict with hardware breakpoints here. This
commit is to remove the IRET that returns to kernel, not to fix the
problem we have here. Also there are a bunch of merge conflicts when
trying to apply this commit to older kernels, so fixing it directly in
older kernels is probably a better option.

If the debug exception is triggered by kprobe, then regs->ip should be
located in the kprobe instruction slot. Add this check to
kprobe_debug_handler() to properly determine if a debug exception should
be handled by kprobe.

The stable kernels affected are 5.10, 5.4, 4.19, and 4.14. I made the
fix in 5.10, and we should probably apply this fix to other stable
kernels.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 arch/x86/kernel/kprobes/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Greg KH June 30, 2023, 5:21 a.m. UTC | #1 
On Fri, Jun 30, 2023 at 10:08:45AM +0800, Li Huafei wrote:
> We get the following crash caused by a null pointer access:
> 
>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>  ...
>  RIP: 0010:resume_execution+0x35/0x190
>  ...
>  Call Trace:
>   <#DB>
>   kprobe_debug_handler+0x41/0xd0
>   exc_debug+0xe5/0x1b0
>   asm_exc_debug+0x19/0x30
>  RIP: 0010:copy_from_kernel_nofault.part.0+0x55/0xc0
>  ...
>   </#DB>
>   process_fetch_insn+0xfb/0x720
>   kprobe_trace_func+0x199/0x2c0
>   ? kernel_clone+0x5/0x2f0
>   kprobe_dispatcher+0x3d/0x60
>   aggr_pre_handler+0x40/0x80
>   ? kernel_clone+0x1/0x2f0
>   kprobe_ftrace_handler+0x82/0xf0
>   ? __se_sys_clone+0x65/0x90
>   ftrace_ops_assist_func+0x86/0x110
>   ? rcu_nocb_try_bypass+0x1f3/0x370
>   0xffffffffc07e60c8
>   ? kernel_clone+0x1/0x2f0
>   kernel_clone+0x5/0x2f0
> 
> The analysis reveals that kprobe and hardware breakpoints conflict in
> the use of debug exceptions.
> 
> If we set a hardware breakpoint on a memory address and also have a
> kprobe event to fetch the memory at this address. Then when kprobe
> triggers, it goes to read the memory and triggers hardware breakpoint
> monitoring. This time, since kprobe handles debug exceptions earlier
> than hardware breakpoints, it will cause kprobe to incorrectly assume
> that the exception is a kprobe trigger.
> 
> Notice that after the mainline commit 6256e668b7af ("x86/kprobes: Use
> int3 instead of debug trap for single-step"), kprobe no longer uses
> debug trap, avoiding the conflict with hardware breakpoints here. This
> commit is to remove the IRET that returns to kernel, not to fix the
> problem we have here. Also there are a bunch of merge conflicts when
> trying to apply this commit to older kernels, so fixing it directly in
> older kernels is probably a better option.

What is the list of commits that it would take to resolve this in these
kernels?  We would almost always prefer to do that instead of taking
changes that are not upstream.

thanks,

greg k-h
Li Huafei July 1, 2023, 8:43 a.m. UTC | #2 
On 2023/6/30 13:21, Greg KH wrote:
> On Fri, Jun 30, 2023 at 10:08:45AM +0800, Li Huafei wrote:
>> We get the following crash caused by a null pointer access:
>>
>>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>>  ...
>>  RIP: 0010:resume_execution+0x35/0x190
>>  ...
>>  Call Trace:
>>   <#DB>
>>   kprobe_debug_handler+0x41/0xd0
>>   exc_debug+0xe5/0x1b0
>>   asm_exc_debug+0x19/0x30
>>  RIP: 0010:copy_from_kernel_nofault.part.0+0x55/0xc0
>>  ...
>>   </#DB>
>>   process_fetch_insn+0xfb/0x720
>>   kprobe_trace_func+0x199/0x2c0
>>   ? kernel_clone+0x5/0x2f0
>>   kprobe_dispatcher+0x3d/0x60
>>   aggr_pre_handler+0x40/0x80
>>   ? kernel_clone+0x1/0x2f0
>>   kprobe_ftrace_handler+0x82/0xf0
>>   ? __se_sys_clone+0x65/0x90
>>   ftrace_ops_assist_func+0x86/0x110
>>   ? rcu_nocb_try_bypass+0x1f3/0x370
>>   0xffffffffc07e60c8
>>   ? kernel_clone+0x1/0x2f0
>>   kernel_clone+0x5/0x2f0
>>
>> The analysis reveals that kprobe and hardware breakpoints conflict in
>> the use of debug exceptions.
>>
>> If we set a hardware breakpoint on a memory address and also have a
>> kprobe event to fetch the memory at this address. Then when kprobe
>> triggers, it goes to read the memory and triggers hardware breakpoint
>> monitoring. This time, since kprobe handles debug exceptions earlier
>> than hardware breakpoints, it will cause kprobe to incorrectly assume
>> that the exception is a kprobe trigger.
>>
>> Notice that after the mainline commit 6256e668b7af ("x86/kprobes: Use
>> int3 instead of debug trap for single-step"), kprobe no longer uses
>> debug trap, avoiding the conflict with hardware breakpoints here. This
>> commit is to remove the IRET that returns to kernel, not to fix the
>> problem we have here. Also there are a bunch of merge conflicts when
>> trying to apply this commit to older kernels, so fixing it directly in
>> older kernels is probably a better option.
> 
> What is the list of commits that it would take to resolve this in these
> kernels?  We would almost always prefer to do that instead of taking
> changes that are not upstream.

I have sorted out that for 5.10 there are 9 patches that need to be
backported:

  #9 8924779df820 ("x86/kprobes: Fix JNG/JNLE emulation")
  #8 dec8784c9088 ("x86/kprobes: Update kcb status flag after singlestepping")
  #7 2304d14db659 ("x86/kprobes: Move 'inline' to the beginning of the kprobe_is_ss() declaration")
  #6 2f706e0e5e26 ("x86/kprobes: Fix to identify indirect jmp and others using range case")
  #5 6256e668b7af ("x86/kprobes: Use int3 instead of debug trap for single-step")
  #4 a194acd316f9 ("x86/kprobes: Identify far indirect JMP correctly")
  #3 d60ad3d46f1d ("x86/kprobes: Retrieve correct opcode for group instruction")
  #2 abd82e533d88 ("x86/kprobes: Do not decode opcode in resume_execution()")
  #1 e689b300c99c ("kprobes/x86: Fix fall-through warnings for Clang e689b300c99c")
  
The main one we need to backport is patch 5, patche 1-6 are pre-patches,
and patche 6-9 are fix patches for patch 5. The major modifications are
patch 2 and patch 4. Patch 2 optimizes resume_execution() to avoid
repeated instruction decoding, and patch 5 uses int3 instead of debug
trap, and as Masami said in the commit message this patch will change
some behavior of kprobe, but it has almost no effect on the actual
usage.

I'm not sure backport these patches are acceptable, do I need to send
them out for review?

Thanks,
Huafei

> 
> thanks,
> 
> greg k-h
> .
>
Greg KH July 3, 2023, 6:34 p.m. UTC | #3 
On Sat, Jul 01, 2023 at 04:43:46PM +0800, Li Huafei wrote:
> 
> 
> On 2023/6/30 13:21, Greg KH wrote:
> > On Fri, Jun 30, 2023 at 10:08:45AM +0800, Li Huafei wrote:
> >> We get the following crash caused by a null pointer access:
> >>
> >>  BUG: kernel NULL pointer dereference, address: 0000000000000000
> >>  ...
> >>  RIP: 0010:resume_execution+0x35/0x190
> >>  ...
> >>  Call Trace:
> >>   <#DB>
> >>   kprobe_debug_handler+0x41/0xd0
> >>   exc_debug+0xe5/0x1b0
> >>   asm_exc_debug+0x19/0x30
> >>  RIP: 0010:copy_from_kernel_nofault.part.0+0x55/0xc0
> >>  ...
> >>   </#DB>
> >>   process_fetch_insn+0xfb/0x720
> >>   kprobe_trace_func+0x199/0x2c0
> >>   ? kernel_clone+0x5/0x2f0
> >>   kprobe_dispatcher+0x3d/0x60
> >>   aggr_pre_handler+0x40/0x80
> >>   ? kernel_clone+0x1/0x2f0
> >>   kprobe_ftrace_handler+0x82/0xf0
> >>   ? __se_sys_clone+0x65/0x90
> >>   ftrace_ops_assist_func+0x86/0x110
> >>   ? rcu_nocb_try_bypass+0x1f3/0x370
> >>   0xffffffffc07e60c8
> >>   ? kernel_clone+0x1/0x2f0
> >>   kernel_clone+0x5/0x2f0
> >>
> >> The analysis reveals that kprobe and hardware breakpoints conflict in
> >> the use of debug exceptions.
> >>
> >> If we set a hardware breakpoint on a memory address and also have a
> >> kprobe event to fetch the memory at this address. Then when kprobe
> >> triggers, it goes to read the memory and triggers hardware breakpoint
> >> monitoring. This time, since kprobe handles debug exceptions earlier
> >> than hardware breakpoints, it will cause kprobe to incorrectly assume
> >> that the exception is a kprobe trigger.
> >>
> >> Notice that after the mainline commit 6256e668b7af ("x86/kprobes: Use
> >> int3 instead of debug trap for single-step"), kprobe no longer uses
> >> debug trap, avoiding the conflict with hardware breakpoints here. This
> >> commit is to remove the IRET that returns to kernel, not to fix the
> >> problem we have here. Also there are a bunch of merge conflicts when
> >> trying to apply this commit to older kernels, so fixing it directly in
> >> older kernels is probably a better option.
> > 
> > What is the list of commits that it would take to resolve this in these
> > kernels?  We would almost always prefer to do that instead of taking
> > changes that are not upstream.
> 
> I have sorted out that for 5.10 there are 9 patches that need to be
> backported:
> 
>   #9 8924779df820 ("x86/kprobes: Fix JNG/JNLE emulation")
>   #8 dec8784c9088 ("x86/kprobes: Update kcb status flag after singlestepping")
>   #7 2304d14db659 ("x86/kprobes: Move 'inline' to the beginning of the kprobe_is_ss() declaration")
>   #6 2f706e0e5e26 ("x86/kprobes: Fix to identify indirect jmp and others using range case")
>   #5 6256e668b7af ("x86/kprobes: Use int3 instead of debug trap for single-step")
>   #4 a194acd316f9 ("x86/kprobes: Identify far indirect JMP correctly")
>   #3 d60ad3d46f1d ("x86/kprobes: Retrieve correct opcode for group instruction")
>   #2 abd82e533d88 ("x86/kprobes: Do not decode opcode in resume_execution()")
>   #1 e689b300c99c ("kprobes/x86: Fix fall-through warnings for Clang e689b300c99c")
>   
> The main one we need to backport is patch 5, patche 1-6 are pre-patches,
> and patche 6-9 are fix patches for patch 5. The major modifications are
> patch 2 and patch 4. Patch 2 optimizes resume_execution() to avoid
> repeated instruction decoding, and patch 5 uses int3 instead of debug
> trap, and as Masami said in the commit message this patch will change
> some behavior of kprobe, but it has almost no effect on the actual
> usage.
> 
> I'm not sure backport these patches are acceptable, do I need to send
> them out for review?

Yes, please make up the patch series for these, that's not all that bad,
and looks like it is more "correct" than just your one-off patch.

thanks,

greg k-h
Li Huafei July 5, 2023, 6:51 a.m. UTC | #4 
On 2023/7/4 2:34, Greg KH wrote:
> On Sat, Jul 01, 2023 at 04:43:46PM +0800, Li Huafei wrote:
>>
>>
>> On 2023/6/30 13:21, Greg KH wrote:
>>> On Fri, Jun 30, 2023 at 10:08:45AM +0800, Li Huafei wrote:
>>>> We get the following crash caused by a null pointer access:
>>>>
>>>>  BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>>  ...
>>>>  RIP: 0010:resume_execution+0x35/0x190
>>>>  ...
>>>>  Call Trace:
>>>>   <#DB>
>>>>   kprobe_debug_handler+0x41/0xd0
>>>>   exc_debug+0xe5/0x1b0
>>>>   asm_exc_debug+0x19/0x30
>>>>  RIP: 0010:copy_from_kernel_nofault.part.0+0x55/0xc0
>>>>  ...
>>>>   </#DB>
>>>>   process_fetch_insn+0xfb/0x720
>>>>   kprobe_trace_func+0x199/0x2c0
>>>>   ? kernel_clone+0x5/0x2f0
>>>>   kprobe_dispatcher+0x3d/0x60
>>>>   aggr_pre_handler+0x40/0x80
>>>>   ? kernel_clone+0x1/0x2f0
>>>>   kprobe_ftrace_handler+0x82/0xf0
>>>>   ? __se_sys_clone+0x65/0x90
>>>>   ftrace_ops_assist_func+0x86/0x110
>>>>   ? rcu_nocb_try_bypass+0x1f3/0x370
>>>>   0xffffffffc07e60c8
>>>>   ? kernel_clone+0x1/0x2f0
>>>>   kernel_clone+0x5/0x2f0
>>>>
>>>> The analysis reveals that kprobe and hardware breakpoints conflict in
>>>> the use of debug exceptions.
>>>>
>>>> If we set a hardware breakpoint on a memory address and also have a
>>>> kprobe event to fetch the memory at this address. Then when kprobe
>>>> triggers, it goes to read the memory and triggers hardware breakpoint
>>>> monitoring. This time, since kprobe handles debug exceptions earlier
>>>> than hardware breakpoints, it will cause kprobe to incorrectly assume
>>>> that the exception is a kprobe trigger.
>>>>
>>>> Notice that after the mainline commit 6256e668b7af ("x86/kprobes: Use
>>>> int3 instead of debug trap for single-step"), kprobe no longer uses
>>>> debug trap, avoiding the conflict with hardware breakpoints here. This
>>>> commit is to remove the IRET that returns to kernel, not to fix the
>>>> problem we have here. Also there are a bunch of merge conflicts when
>>>> trying to apply this commit to older kernels, so fixing it directly in
>>>> older kernels is probably a better option.
>>>
>>> What is the list of commits that it would take to resolve this in these
>>> kernels?  We would almost always prefer to do that instead of taking
>>> changes that are not upstream.
>>
>> I have sorted out that for 5.10 there are 9 patches that need to be
>> backported:
>>
>>   #9 8924779df820 ("x86/kprobes: Fix JNG/JNLE emulation")
>>   #8 dec8784c9088 ("x86/kprobes: Update kcb status flag after singlestepping")
>>   #7 2304d14db659 ("x86/kprobes: Move 'inline' to the beginning of the kprobe_is_ss() declaration")
>>   #6 2f706e0e5e26 ("x86/kprobes: Fix to identify indirect jmp and others using range case")
>>   #5 6256e668b7af ("x86/kprobes: Use int3 instead of debug trap for single-step")
>>   #4 a194acd316f9 ("x86/kprobes: Identify far indirect JMP correctly")
>>   #3 d60ad3d46f1d ("x86/kprobes: Retrieve correct opcode for group instruction")
>>   #2 abd82e533d88 ("x86/kprobes: Do not decode opcode in resume_execution()")
>>   #1 e689b300c99c ("kprobes/x86: Fix fall-through warnings for Clang e689b300c99c")
>>   
>> The main one we need to backport is patch 5, patche 1-6 are pre-patches,
>> and patche 6-9 are fix patches for patch 5. The major modifications are
>> patch 2 and patch 4. Patch 2 optimizes resume_execution() to avoid
>> repeated instruction decoding, and patch 5 uses int3 instead of debug
>> trap, and as Masami said in the commit message this patch will change
>> some behavior of kprobe, but it has almost no effect on the actual
>> usage.
>>
>> I'm not sure backport these patches are acceptable, do I need to send
>> them out for review?
> 
> Yes, please make up the patch series for these, that's not all that bad,
> and looks like it is more "correct" than just your one-off patch.
> 

Okay, I've sent out the patch set, thanks for the suggestion!

Thanks,
Huafei

> thanks,
> 
> greg k-h
> .
>
diff mbox series

Patch

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 5de757099186..fd8d7d128807 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -900,7 +900,14 @@  int kprobe_debug_handler(struct pt_regs *regs)
 	struct kprobe *cur = kprobe_running();
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
 
-	if (!cur)
+	if (!cur || !cur->ainsn.insn)
+		return 0;
+
+	/* regs->ip should be the address of next instruction to
+	 * cur->ainsn.insn.
+	 */
+	if (regs->ip < (unsigned long)cur->ainsn.insn ||
+	    regs->ip - (unsigned long)cur->ainsn.insn > MAX_INSN_SIZE)
 		return 0;
 
 	resume_execution(cur, regs, kcb);