diff mbox series

[RFC,1/2] x86/kprobes: Prohibit kprobing on INT and UD

Message ID 20240127044124.57594-2-jinghao7@illinois.edu
State New
Headers
Series x86/kprobes: add exception opcode detector and boost more opcodes |

Commit Message

Jinghao Jia Jan. 27, 2024, 4:41 a.m. UTC 
  Both INTs (INT n, INT1, INT3, INTO) and UDs (UD0, UD1, UD2) serve
special purposes in the kernel, e.g., INT3 is used by KGDB and UD2 is
involved in LLVM-KCFI instrumentation. At the same time, attaching
kprobes on these instructions (particularly UDs) will pollute the stack
trace dumped in the kernel ring buffer, since the exception is triggered
in the copy buffer rather than the original location.

Check for INTs and UDs in can_probe and reject any kprobes trying to
attach to these instructions.

Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
---
 arch/x86/kernel/kprobes/core.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Xin Li (Intel) Jan. 27, 2024, 7:47 p.m. UTC | #1 
On 1/26/2024 8:41 PM, Jinghao Jia wrote:
> Both INTs (INT n, INT1, INT3, INTO) and UDs (UD0, UD1, UD2) serve
> special purposes in the kernel, e.g., INT3 is used by KGDB and UD2 is
> involved in LLVM-KCFI instrumentation. At the same time, attaching
> kprobes on these instructions (particularly UDs) will pollute the stack
> trace dumped in the kernel ring buffer, since the exception is triggered
> in the copy buffer rather than the original location.
> 
> Check for INTs and UDs in can_probe and reject any kprobes trying to
> attach to these instructions.
> 
> Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
> ---
>   arch/x86/kernel/kprobes/core.c | 33 ++++++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index e8babebad7b8..792b38d22126 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -252,6 +252,22 @@ unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add
>   	return __recover_probed_insn(buf, addr);
>   }
>   
> +static inline int is_exception_insn(struct insn *insn)

s/int/bool

> +{
> +	if (insn->opcode.bytes[0] == 0x0f) {
> +		/* UD0 / UD1 / UD2 */
> +		return insn->opcode.bytes[1] == 0xff ||
> +		       insn->opcode.bytes[1] == 0xb9 ||
> +		       insn->opcode.bytes[1] == 0x0b;
> +	} else {
> +		/* INT3 / INT n / INTO / INT1 */
> +		return insn->opcode.bytes[0] == 0xcc ||
> +		       insn->opcode.bytes[0] == 0xcd ||
> +		       insn->opcode.bytes[0] == 0xce ||
> +		       insn->opcode.bytes[0] == 0xf1;
> +	}
> +}
> +
>   /* Check if paddr is at an instruction boundary */
>   static int can_probe(unsigned long paddr)
>   {
> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr)
>   #endif
>   		addr += insn.length;
>   	}
> +	__addr = recover_probed_instruction(buf, addr);
> +	if (!__addr)
> +		return 0;
> +
> +	if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> +		return 0;
> +
> +	if (is_exception_insn(&insn))
> +		return 0;
> +
>   	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
>   		/*
>   		 * The compiler generates the following instruction sequence
> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr)
>   		 * Also, these movl and addl are used for showing expected
>   		 * type. So those must not be touched.
>   		 */
> -		__addr = recover_probed_instruction(buf, addr);
> -		if (!__addr)
> -			return 0;
> -
> -		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> -			return 0;
> -
>   		if (insn.opcode.value == 0xBA)
>   			offset = 12;
>   		else if (insn.opcode.value == 0x3)
Masami Hiramatsu (Google) Jan. 28, 2024, 1:19 a.m. UTC | #2 
On Fri, 26 Jan 2024 22:41:23 -0600
Jinghao Jia <jinghao7@illinois.edu> wrote:

> Both INTs (INT n, INT1, INT3, INTO) and UDs (UD0, UD1, UD2) serve
> special purposes in the kernel, e.g., INT3 is used by KGDB and UD2 is
> involved in LLVM-KCFI instrumentation. At the same time, attaching
> kprobes on these instructions (particularly UDs) will pollute the stack
> trace dumped in the kernel ring buffer, since the exception is triggered
> in the copy buffer rather than the original location.
> 
> Check for INTs and UDs in can_probe and reject any kprobes trying to
> attach to these instructions.
> 

Thanks for implement this check!


> Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
> ---
>  arch/x86/kernel/kprobes/core.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index e8babebad7b8..792b38d22126 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -252,6 +252,22 @@ unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add
>  	return __recover_probed_insn(buf, addr);
>  }
>  
> +static inline int is_exception_insn(struct insn *insn)
> +{
> +	if (insn->opcode.bytes[0] == 0x0f) {
> +		/* UD0 / UD1 / UD2 */
> +		return insn->opcode.bytes[1] == 0xff ||
> +		       insn->opcode.bytes[1] == 0xb9 ||
> +		       insn->opcode.bytes[1] == 0x0b;
> +	} else {

If "else" block just return, you don't need this "else".

bool func()
{
	if (cond)
		return ...

	return ...
}

Is preferrable because this puts "return val" always at the end of non-void
function.

> +		/* INT3 / INT n / INTO / INT1 */
> +		return insn->opcode.bytes[0] == 0xcc ||
> +		       insn->opcode.bytes[0] == 0xcd ||
> +		       insn->opcode.bytes[0] == 0xce ||
> +		       insn->opcode.bytes[0] == 0xf1;
> +	}
> +}
> +
>  /* Check if paddr is at an instruction boundary */
>  static int can_probe(unsigned long paddr)
>  {
> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr)
>  #endif
>  		addr += insn.length;
>  	}
> +	__addr = recover_probed_instruction(buf, addr);
> +	if (!__addr)
> +		return 0;
> +
> +	if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> +		return 0;
> +
> +	if (is_exception_insn(&insn))
> +		return 0;
> +

Please don't put this outside of decoding loop. You should put these in
the loop which decodes the instruction from the beginning of the function.
Since the x86 instrcution is variable length, can_probe() needs to check
whether that the address is instruction boundary and decodable.

Thank you,

>  	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
>  		/*
>  		 * The compiler generates the following instruction sequence
> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr)
>  		 * Also, these movl and addl are used for showing expected
>  		 * type. So those must not be touched.
>  		 */
> -		__addr = recover_probed_instruction(buf, addr);
> -		if (!__addr)
> -			return 0;
> -
> -		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> -			return 0;
> -
>  		if (insn.opcode.value == 0xBA)
>  			offset = 12;
>  		else if (insn.opcode.value == 0x3)
> -- 
> 2.43.0
>
Jinghao Jia Jan. 28, 2024, 9:09 p.m. UTC | #3 
On 1/27/24 13:47, Xin Li wrote:
> On 1/26/2024 8:41 PM, Jinghao Jia wrote:
>> Both INTs (INT n, INT1, INT3, INTO) and UDs (UD0, UD1, UD2) serve
>> special purposes in the kernel, e.g., INT3 is used by KGDB and UD2 is
>> involved in LLVM-KCFI instrumentation. At the same time, attaching
>> kprobes on these instructions (particularly UDs) will pollute the stack
>> trace dumped in the kernel ring buffer, since the exception is triggered
>> in the copy buffer rather than the original location.
>>
>> Check for INTs and UDs in can_probe and reject any kprobes trying to
>> attach to these instructions.
>>
>> Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>> Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
>> ---
>>   arch/x86/kernel/kprobes/core.c | 33 ++++++++++++++++++++++++++-------
>>   1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
>> index e8babebad7b8..792b38d22126 100644
>> --- a/arch/x86/kernel/kprobes/core.c
>> +++ b/arch/x86/kernel/kprobes/core.c
>> @@ -252,6 +252,22 @@ unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add
>>       return __recover_probed_insn(buf, addr);
>>   }
>>   +static inline int is_exception_insn(struct insn *insn)
> 
> s/int/bool
> 

Oh yes, the return type should be bool. Thanks for pointing out!

--Jinghao

>> +{
>> +    if (insn->opcode.bytes[0] == 0x0f) {
>> +        /* UD0 / UD1 / UD2 */
>> +        return insn->opcode.bytes[1] == 0xff ||
>> +               insn->opcode.bytes[1] == 0xb9 ||
>> +               insn->opcode.bytes[1] == 0x0b;
>> +    } else {
>> +        /* INT3 / INT n / INTO / INT1 */
>> +        return insn->opcode.bytes[0] == 0xcc ||
>> +               insn->opcode.bytes[0] == 0xcd ||
>> +               insn->opcode.bytes[0] == 0xce ||
>> +               insn->opcode.bytes[0] == 0xf1;
>> +    }
>> +}
>> +
>>   /* Check if paddr is at an instruction boundary */
>>   static int can_probe(unsigned long paddr)
>>   {
>> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr)
>>   #endif
>>           addr += insn.length;
>>       }
>> +    __addr = recover_probed_instruction(buf, addr);
>> +    if (!__addr)
>> +        return 0;
>> +
>> +    if (insn_decode_kernel(&insn, (void *)__addr) < 0)
>> +        return 0;
>> +
>> +    if (is_exception_insn(&insn))
>> +        return 0;
>> +
>>       if (IS_ENABLED(CONFIG_CFI_CLANG)) {
>>           /*
>>            * The compiler generates the following instruction sequence
>> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr)
>>            * Also, these movl and addl are used for showing expected
>>            * type. So those must not be touched.
>>            */
>> -        __addr = recover_probed_instruction(buf, addr);
>> -        if (!__addr)
>> -            return 0;
>> -
>> -        if (insn_decode_kernel(&insn, (void *)__addr) < 0)
>> -            return 0;
>> -
>>           if (insn.opcode.value == 0xBA)
>>               offset = 12;
>>           else if (insn.opcode.value == 0x3)
>
Jinghao Jia Jan. 28, 2024, 9:25 p.m. UTC | #4 
On 1/27/24 19:19, Masami Hiramatsu (Google) wrote:
> On Fri, 26 Jan 2024 22:41:23 -0600
> Jinghao Jia <jinghao7@illinois.edu> wrote:
> 
>> Both INTs (INT n, INT1, INT3, INTO) and UDs (UD0, UD1, UD2) serve
>> special purposes in the kernel, e.g., INT3 is used by KGDB and UD2 is
>> involved in LLVM-KCFI instrumentation. At the same time, attaching
>> kprobes on these instructions (particularly UDs) will pollute the stack
>> trace dumped in the kernel ring buffer, since the exception is triggered
>> in the copy buffer rather than the original location.
>>
>> Check for INTs and UDs in can_probe and reject any kprobes trying to
>> attach to these instructions.
>>
> 
> Thanks for implement this check!
> 

You are very welcome :)

> 
>> Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>> Signed-off-by: Jinghao Jia <jinghao7@illinois.edu>
>> ---
>>  arch/x86/kernel/kprobes/core.c | 33 ++++++++++++++++++++++++++-------
>>  1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
>> index e8babebad7b8..792b38d22126 100644
>> --- a/arch/x86/kernel/kprobes/core.c
>> +++ b/arch/x86/kernel/kprobes/core.c
>> @@ -252,6 +252,22 @@ unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add
>>  	return __recover_probed_insn(buf, addr);
>>  }
>>  
>> +static inline int is_exception_insn(struct insn *insn)
>> +{
>> +	if (insn->opcode.bytes[0] == 0x0f) {
>> +		/* UD0 / UD1 / UD2 */
>> +		return insn->opcode.bytes[1] == 0xff ||
>> +		       insn->opcode.bytes[1] == 0xb9 ||
>> +		       insn->opcode.bytes[1] == 0x0b;
>> +	} else {
> 
> If "else" block just return, you don't need this "else".
> 
> bool func()
> {
> 	if (cond)
> 		return ...
> 
> 	return ...
> }
> 
> Is preferrable because this puts "return val" always at the end of non-void
> function.
> 

I will fix this in the v2.

>> +		/* INT3 / INT n / INTO / INT1 */
>> +		return insn->opcode.bytes[0] == 0xcc ||
>> +		       insn->opcode.bytes[0] == 0xcd ||
>> +		       insn->opcode.bytes[0] == 0xce ||
>> +		       insn->opcode.bytes[0] == 0xf1;
>> +	}
>> +}
>> +
>>  /* Check if paddr is at an instruction boundary */
>>  static int can_probe(unsigned long paddr)
>>  {
>> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr)
>>  #endif
>>  		addr += insn.length;
>>  	}
>> +	__addr = recover_probed_instruction(buf, addr);
>> +	if (!__addr)
>> +		return 0;
>> +
>> +	if (insn_decode_kernel(&insn, (void *)__addr) < 0)
>> +		return 0;
>> +
>> +	if (is_exception_insn(&insn))
>> +		return 0;
>> +
> 
> Please don't put this outside of decoding loop. You should put these in
> the loop which decodes the instruction from the beginning of the function.
> Since the x86 instrcution is variable length, can_probe() needs to check
> whether that the address is instruction boundary and decodable.
> 
> Thank you,

If my understanding is correct then this is trying to decode the kprobe
target instruction, given that it is after the main decoding loop.  Here I
hoisted the decoding logic out of the if(IS_ENABLED(CONFIG_CFI_CLANG))
block so that we do not need to decode the same instruction twice.  I left
the main decoding loop unchanged so it is still decoding the function from
the start and should handle instruction boundaries. Are there any caveats
that I missed?

--Jinghao

> 
>>  	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
>>  		/*
>>  		 * The compiler generates the following instruction sequence
>> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr)
>>  		 * Also, these movl and addl are used for showing expected
>>  		 * type. So those must not be touched.
>>  		 */
>> -		__addr = recover_probed_instruction(buf, addr);
>> -		if (!__addr)
>> -			return 0;
>> -
>> -		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
>> -			return 0;
>> -
>>  		if (insn.opcode.value == 0xBA)
>>  			offset = 12;
>>  		else if (insn.opcode.value == 0x3)
>> -- 
>> 2.43.0
>>
> 
>
Masami Hiramatsu (Google) Jan. 30, 2024, 1:44 a.m. UTC | #5 
On Sun, 28 Jan 2024 15:25:59 -0600
Jinghao Jia <jinghao7@illinois.edu> wrote:

> >>  /* Check if paddr is at an instruction boundary */
> >>  static int can_probe(unsigned long paddr)
> >>  {
> >> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr)
> >>  #endif
> >>  		addr += insn.length;
> >>  	}
> >> +	__addr = recover_probed_instruction(buf, addr);
> >> +	if (!__addr)
> >> +		return 0;
> >> +
> >> +	if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> >> +		return 0;
> >> +
> >> +	if (is_exception_insn(&insn))
> >> +		return 0;
> >> +
> > 
> > Please don't put this outside of decoding loop. You should put these in
> > the loop which decodes the instruction from the beginning of the function.
> > Since the x86 instrcution is variable length, can_probe() needs to check
> > whether that the address is instruction boundary and decodable.
> > 
> > Thank you,
> 
> If my understanding is correct then this is trying to decode the kprobe
> target instruction, given that it is after the main decoding loop.  Here I
> hoisted the decoding logic out of the if(IS_ENABLED(CONFIG_CFI_CLANG))
> block so that we do not need to decode the same instruction twice.  I left
> the main decoding loop unchanged so it is still decoding the function from
> the start and should handle instruction boundaries. Are there any caveats
> that I missed?

Ah, sorry I misread the patch. You're correct!
This is a good place to do that.

But hmm, I think we should add another patch to check the addr == paddr
soon after the loop so that we will avoid decoding.

Thank you,

> 
> --Jinghao
> 
> > 
> >>  	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
> >>  		/*
> >>  		 * The compiler generates the following instruction sequence
> >> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr)
> >>  		 * Also, these movl and addl are used for showing expected
> >>  		 * type. So those must not be touched.
> >>  		 */
> >> -		__addr = recover_probed_instruction(buf, addr);
> >> -		if (!__addr)
> >> -			return 0;
> >> -
> >> -		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> >> -			return 0;
> >> -
> >>  		if (insn.opcode.value == 0xBA)
> >>  			offset = 12;
> >>  		else if (insn.opcode.value == 0x3)
> >> -- 
> >> 2.43.0
> >>
> > 
> >
Jinghao Jia Jan. 30, 2024, 2:50 a.m. UTC | #6 
On 1/29/24 19:44, Masami Hiramatsu (Google) wrote:
> On Sun, 28 Jan 2024 15:25:59 -0600
> Jinghao Jia <jinghao7@illinois.edu> wrote:
> 
>>>>  /* Check if paddr is at an instruction boundary */
>>>>  static int can_probe(unsigned long paddr)
>>>>  {
>>>> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr)
>>>>  #endif
>>>>  		addr += insn.length;
>>>>  	}
>>>> +	__addr = recover_probed_instruction(buf, addr);
>>>> +	if (!__addr)
>>>> +		return 0;
>>>> +
>>>> +	if (insn_decode_kernel(&insn, (void *)__addr) < 0)
>>>> +		return 0;
>>>> +
>>>> +	if (is_exception_insn(&insn))
>>>> +		return 0;
>>>> +
>>>
>>> Please don't put this outside of decoding loop. You should put these in
>>> the loop which decodes the instruction from the beginning of the function.
>>> Since the x86 instrcution is variable length, can_probe() needs to check
>>> whether that the address is instruction boundary and decodable.
>>>
>>> Thank you,
>>
>> If my understanding is correct then this is trying to decode the kprobe
>> target instruction, given that it is after the main decoding loop.  Here I
>> hoisted the decoding logic out of the if(IS_ENABLED(CONFIG_CFI_CLANG))
>> block so that we do not need to decode the same instruction twice.  I left
>> the main decoding loop unchanged so it is still decoding the function from
>> the start and should handle instruction boundaries. Are there any caveats
>> that I missed?
> 
> Ah, sorry I misread the patch. You're correct!
> This is a good place to do that.
> 
> But hmm, I think we should add another patch to check the addr == paddr
> soon after the loop so that we will avoid decoding.
> 
> Thank you,
> 

Yes, that makes sense to me. At the same time, I'm also thinking about
changing the return type of can_probe() to bool, since we are just using
int as bool in this context.

--Jinghao

>>
>> --Jinghao
>>
>>>
>>>>  	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
>>>>  		/*
>>>>  		 * The compiler generates the following instruction sequence
>>>> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr)
>>>>  		 * Also, these movl and addl are used for showing expected
>>>>  		 * type. So those must not be touched.
>>>>  		 */
>>>> -		__addr = recover_probed_instruction(buf, addr);
>>>> -		if (!__addr)
>>>> -			return 0;
>>>> -
>>>> -		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
>>>> -			return 0;
>>>> -
>>>>  		if (insn.opcode.value == 0xBA)
>>>>  			offset = 12;
>>>>  		else if (insn.opcode.value == 0x3)
>>>> -- 
>>>> 2.43.0
>>>>
>>>
>>>
> 
>
Masami Hiramatsu (Google) Jan. 30, 2024, 11:30 a.m. UTC | #7 
On Mon, 29 Jan 2024 20:50:39 -0600
Jinghao Jia <jinghao7@illinois.edu> wrote:

> On 1/29/24 19:44, Masami Hiramatsu (Google) wrote:
> > On Sun, 28 Jan 2024 15:25:59 -0600
> > Jinghao Jia <jinghao7@illinois.edu> wrote:
> > 
> >>>>  /* Check if paddr is at an instruction boundary */
> >>>>  static int can_probe(unsigned long paddr)
> >>>>  {
> >>>> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr)
> >>>>  #endif
> >>>>  		addr += insn.length;
> >>>>  	}
> >>>> +	__addr = recover_probed_instruction(buf, addr);
> >>>> +	if (!__addr)
> >>>> +		return 0;
> >>>> +
> >>>> +	if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> >>>> +		return 0;
> >>>> +
> >>>> +	if (is_exception_insn(&insn))
> >>>> +		return 0;
> >>>> +
> >>>
> >>> Please don't put this outside of decoding loop. You should put these in
> >>> the loop which decodes the instruction from the beginning of the function.
> >>> Since the x86 instrcution is variable length, can_probe() needs to check
> >>> whether that the address is instruction boundary and decodable.
> >>>
> >>> Thank you,
> >>
> >> If my understanding is correct then this is trying to decode the kprobe
> >> target instruction, given that it is after the main decoding loop.  Here I
> >> hoisted the decoding logic out of the if(IS_ENABLED(CONFIG_CFI_CLANG))
> >> block so that we do not need to decode the same instruction twice.  I left
> >> the main decoding loop unchanged so it is still decoding the function from
> >> the start and should handle instruction boundaries. Are there any caveats
> >> that I missed?
> > 
> > Ah, sorry I misread the patch. You're correct!
> > This is a good place to do that.
> > 
> > But hmm, I think we should add another patch to check the addr == paddr
> > soon after the loop so that we will avoid decoding.
> > 
> > Thank you,
> > 
> 
> Yes, that makes sense to me. At the same time, I'm also thinking about
> changing the return type of can_probe() to bool, since we are just using
> int as bool in this context.

Yes, that is also a good change :)

Thank you,

> 
> --Jinghao
> 
> >>
> >> --Jinghao
> >>
> >>>
> >>>>  	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
> >>>>  		/*
> >>>>  		 * The compiler generates the following instruction sequence
> >>>> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr)
> >>>>  		 * Also, these movl and addl are used for showing expected
> >>>>  		 * type. So those must not be touched.
> >>>>  		 */
> >>>> -		__addr = recover_probed_instruction(buf, addr);
> >>>> -		if (!__addr)
> >>>> -			return 0;
> >>>> -
> >>>> -		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> >>>> -			return 0;
> >>>> -
> >>>>  		if (insn.opcode.value == 0xBA)
> >>>>  			offset = 12;
> >>>>  		else if (insn.opcode.value == 0x3)
> >>>> -- 
> >>>> 2.43.0
> >>>>
> >>>
> >>>
> > 
> >
diff mbox series

Patch

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index e8babebad7b8..792b38d22126 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -252,6 +252,22 @@  unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add
 	return __recover_probed_insn(buf, addr);
 }
 
+static inline int is_exception_insn(struct insn *insn)
+{
+	if (insn->opcode.bytes[0] == 0x0f) {
+		/* UD0 / UD1 / UD2 */
+		return insn->opcode.bytes[1] == 0xff ||
+		       insn->opcode.bytes[1] == 0xb9 ||
+		       insn->opcode.bytes[1] == 0x0b;
+	} else {
+		/* INT3 / INT n / INTO / INT1 */
+		return insn->opcode.bytes[0] == 0xcc ||
+		       insn->opcode.bytes[0] == 0xcd ||
+		       insn->opcode.bytes[0] == 0xce ||
+		       insn->opcode.bytes[0] == 0xf1;
+	}
+}
+
 /* Check if paddr is at an instruction boundary */
 static int can_probe(unsigned long paddr)
 {
@@ -294,6 +310,16 @@  static int can_probe(unsigned long paddr)
 #endif
 		addr += insn.length;
 	}
+	__addr = recover_probed_instruction(buf, addr);
+	if (!__addr)
+		return 0;
+
+	if (insn_decode_kernel(&insn, (void *)__addr) < 0)
+		return 0;
+
+	if (is_exception_insn(&insn))
+		return 0;
+
 	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
 		/*
 		 * The compiler generates the following instruction sequence
@@ -308,13 +334,6 @@  static int can_probe(unsigned long paddr)
 		 * Also, these movl and addl are used for showing expected
 		 * type. So those must not be touched.
 		 */
-		__addr = recover_probed_instruction(buf, addr);
-		if (!__addr)
-			return 0;
-
-		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
-			return 0;
-
 		if (insn.opcode.value == 0xBA)
 			offset = 12;
 		else if (insn.opcode.value == 0x3)