[PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Zhenzhong Duan]

Since CONFIG_RETPOLINE hard depends on compiler support now, so
replacing indirect-jump check with the range check is safe in that case.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/kernel/kprobes/opt.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 40b16b2..1136b29 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -203,6 +203,7 @@ static int copy_optimized_instructions(u8 *dest, u8 *src, u8 *real)
 	return len;
 }
 
+#ifndef CONFIG_RETPOLINE
 /* Check whether insn is indirect jump */
 static int __insn_is_indirect_jump(struct insn *insn)
 {
@@ -210,6 +211,7 @@ static int __insn_is_indirect_jump(struct insn *insn)
 		(X86_MODRM_REG(insn->modrm.value) & 6) == 4) || /* Jump */
 		insn->opcode.bytes[0] == 0xea);	/* Segment based jump */
 }
+#endif
 
 /* Check whether insn jumps into specified address range */
 static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
@@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
 
 static int insn_is_indirect_jump(struct insn *insn)
 {
-	int ret = __insn_is_indirect_jump(insn);
+	int ret;
 
 #ifdef CONFIG_RETPOLINE
-	/*
-	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
-	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
-	 * older gcc may use indirect jump. So we add this check instead of
-	 * replace indirect-jump check.
-	 */
-	if (!ret)
+	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
 		ret = insn_jump_into_range(insn,
 				(unsigned long)__indirect_thunk_start,
 				(unsigned long)__indirect_thunk_end -
 				(unsigned long)__indirect_thunk_start);
+#else
+		ret = __insn_is_indirect_jump(insn);
 #endif
 	return ret;
 }
-- 
1.8.3.1

Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Peter Zijlstra]

On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
> Since CONFIG_RETPOLINE hard depends on compiler support now, so
> replacing indirect-jump check with the range check is safe in that case.

Can we put kprobes on module init text before we run alternatives on it?

> @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
>  
>  static int insn_is_indirect_jump(struct insn *insn)
>  {
> -	int ret = __insn_is_indirect_jump(insn);
> +	int ret;
>  
>  #ifdef CONFIG_RETPOLINE
> -	/*
> -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
> -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
> -	 * older gcc may use indirect jump. So we add this check instead of
> -	 * replace indirect-jump check.
> -	 */
> -	if (!ret)
> +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
>  		ret = insn_jump_into_range(insn,
>  				(unsigned long)__indirect_thunk_start,
>  				(unsigned long)__indirect_thunk_end -
>  				(unsigned long)__indirect_thunk_start);
> +#else
> +		ret = __insn_is_indirect_jump(insn);
>  #endif
>  	return ret;
>  }

The resulting code is indented wrong.

Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Zhenzhong Duan]

On 2018/10/30 16:36, Peter Zijlstra wrote:
> On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
>> Since CONFIG_RETPOLINE hard depends on compiler support now, so
>> replacing indirect-jump check with the range check is safe in that case.
> 
> Can we put kprobes on module init text before we run alternatives on it?

Forgive me I doesn't understand your question. Do you mean this patch 
impact kprobes on module init text?

> 
>> @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
>>   
>>   static int insn_is_indirect_jump(struct insn *insn)
>>   {
>> -	int ret = __insn_is_indirect_jump(insn);
>> +	int ret;
>>   
>>   #ifdef CONFIG_RETPOLINE
>> -	/*
>> -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
>> -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
>> -	 * older gcc may use indirect jump. So we add this check instead of
>> -	 * replace indirect-jump check.
>> -	 */
>> -	if (!ret)
>> +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
>>   		ret = insn_jump_into_range(insn,
>>   				(unsigned long)__indirect_thunk_start,
>>   				(unsigned long)__indirect_thunk_end -
>>   				(unsigned long)__indirect_thunk_start);
>> +#else
>> +		ret = __insn_is_indirect_jump(insn);
>>   #endif
>>   	return ret;
>>   }
> 
> The resulting code is indented wrong.
> 

Oh, yes. Thanks for point out.

Zhenzhong

Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Peter Zijlstra]

On Wed, Oct 31, 2018 at 02:01:20PM +0800, Zhenzhong Duan wrote:
> On 2018/10/30 16:36, Peter Zijlstra wrote:
> > On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
> > > Since CONFIG_RETPOLINE hard depends on compiler support now, so
> > > replacing indirect-jump check with the range check is safe in that case.
> > 
> > Can we put kprobes on module init text before we run alternatives on it?
> 
> Forgive me I doesn't understand your question. Do you mean this patch impact
> kprobes on module init text?

In that case we would still see the indirect paravirt calls for example,
and we'd still need that cascade you took out.

Now, I'm not at all sure we're able to use kprobes at those times, so it
might be a non-issue.

> > > @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
> > >   static int insn_is_indirect_jump(struct insn *insn)
> > >   {
> > > -	int ret = __insn_is_indirect_jump(insn);
> > > +	int ret;
> > >   #ifdef CONFIG_RETPOLINE
> > > -	/*
> > > -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
> > > -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
> > > -	 * older gcc may use indirect jump. So we add this check instead of
> > > -	 * replace indirect-jump check.
> > > -	 */
> > > -	if (!ret)
> > > +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
> > >   		ret = insn_jump_into_range(insn,
> > >   				(unsigned long)__indirect_thunk_start,
> > >   				(unsigned long)__indirect_thunk_end -
> > >   				(unsigned long)__indirect_thunk_start);
> > > +#else
> > > +		ret = __insn_is_indirect_jump(insn);
> > >   #endif
> > >   	return ret;
> > >   }
> > 
> > The resulting code is indented wrong.
> > 
> 
> Oh, yes. Thanks for point out.
> 
> Zhenzhong

Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Peter Zijlstra]

On Wed, Oct 31, 2018 at 02:53:20PM +0100, Peter Zijlstra wrote:
> On Wed, Oct 31, 2018 at 02:01:20PM +0800, Zhenzhong Duan wrote:
> > On 2018/10/30 16:36, Peter Zijlstra wrote:
> > > On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
> > > > Since CONFIG_RETPOLINE hard depends on compiler support now, so
> > > > replacing indirect-jump check with the range check is safe in that case.
> > > 
> > > Can we put kprobes on module init text before we run alternatives on it?
> > 
> > Forgive me I doesn't understand your question. Do you mean this patch impact
> > kprobes on module init text?
> 
> In that case we would still see the indirect paravirt calls for example,
> and we'd still need that cascade you took out.
> 
> Now, I'm not at all sure we're able to use kprobes at those times, so it
> might be a non-issue.

Hmm, what about the case where we have RETPOLINE runtime disabled? Then
the CALL_NOSPEC alternative patches in an indirect call again, and the
retpolines are gone.

Does that not need the __insn_is_indirect_jump() thing?

> > > > @@ -240,20 +242,16 @@ static int insn_jump_into_range(struct insn *insn, unsigned long start, int len)
> > > >   static int insn_is_indirect_jump(struct insn *insn)
> > > >   {
> > > > -	int ret = __insn_is_indirect_jump(insn);
> > > > +	int ret;
> > > >   #ifdef CONFIG_RETPOLINE
> > > > -	/*
> > > > -	 * Jump to x86_indirect_thunk_* is treated as an indirect jump.
> > > > -	 * Note that even with CONFIG_RETPOLINE=y, the kernel compiled with
> > > > -	 * older gcc may use indirect jump. So we add this check instead of
> > > > -	 * replace indirect-jump check.
> > > > -	 */
> > > > -	if (!ret)
> > > > +	/* Jump to x86_indirect_thunk_* is treated as an indirect jump. */
> > > >   		ret = insn_jump_into_range(insn,
> > > >   				(unsigned long)__indirect_thunk_start,
> > > >   				(unsigned long)__indirect_thunk_end -
> > > >   				(unsigned long)__indirect_thunk_start);
> > > > +#else
> > > > +		ret = __insn_is_indirect_jump(insn);
> > > >   #endif
> > > >   	return ret;
> > > >   }
> > > 
> > > The resulting code is indented wrong.
> > > 
> > 
> > Oh, yes. Thanks for point out.
> > 
> > Zhenzhong

Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Zhenzhong Duan]

On 2018/10/31 22:00, Peter Zijlstra wrote:
> On Wed, Oct 31, 2018 at 02:53:20PM +0100, Peter Zijlstra wrote:
>> On Wed, Oct 31, 2018 at 02:01:20PM +0800, Zhenzhong Duan wrote:
>>> On 2018/10/30 16:36, Peter Zijlstra wrote:
>>>> On Mon, Oct 29, 2018 at 11:55:06PM -0700, Zhenzhong Duan wrote:
>>>>> Since CONFIG_RETPOLINE hard depends on compiler support now, so
>>>>> replacing indirect-jump check with the range check is safe in that case.
>>>>
>>>> Can we put kprobes on module init text before we run alternatives on it?
>>>
>>> Forgive me I doesn't understand your question. Do you mean this patch impact
>>> kprobes on module init text?
>>
>> In that case we would still see the indirect paravirt calls for example,
>> and we'd still need that cascade you took out.

Understood.
In another case when loading a non-retpoline module, we suffer the same.
>>
>> Now, I'm not at all sure we're able to use kprobes at those times, so it
>> might be a non-issue.

Not sure, but if it's possible then alternative patching may cover the 
kprobes, it looks like a bug.
> 
> Hmm, what about the case where we have RETPOLINE runtime disabled? Then
> the CALL_NOSPEC alternative patches in an indirect call again, and the
> retpolines are gone.

Is RETPOLINE runtime toggle supported in upstream? I don't see such code.
> 
> Does that not need the __insn_is_indirect_jump() thing?

Yes it's needed if RETPOLINE runtime disabled.

Based on all above reasons, I'd like to drop this patch.

Thanks
Zhenzhong

Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Peter Zijlstra]

On Thu, Nov 01, 2018 at 10:02:14AM +0800, Zhenzhong Duan wrote:
> > Hmm, what about the case where we have RETPOLINE runtime disabled? Then
> > the CALL_NOSPEC alternative patches in an indirect call again, and the
> > retpolines are gone.
> 
> Is RETPOLINE runtime toggle supported in upstream? I don't see such code.

arch/x86/kernel/cpu/bugs.c look for the "nospectre_v2" and related
options. That will avoid X86_FEATURE_RETPOLINE from being set, and thus
the JMP_NOSPEC and CALL_NOSPEC alternatives will not patch out the
indirect jump / call.

Re: [PATCH 3/3] kprobes/x86: Simplify indirect-jump check in retpoline

[Zhenzhong Duan]

On 2018/11/1 16:56, Peter Zijlstra wrote:
> On Thu, Nov 01, 2018 at 10:02:14AM +0800, Zhenzhong Duan wrote:
>>> Hmm, what about the case where we have RETPOLINE runtime disabled? Then
>>> the CALL_NOSPEC alternative patches in an indirect call again, and the
>>> retpolines are gone.
>>
>> Is RETPOLINE runtime toggle supported in upstream? I don't see such code.
> 
> arch/x86/kernel/cpu/bugs.c look for the "nospectre_v2" and related
> options. That will avoid X86_FEATURE_RETPOLINE from being set, and thus
> the JMP_NOSPEC and CALL_NOSPEC alternatives will not patch out the
> indirect jump / call.
> 
Yes, in this case there are also indirect branches. I'll drop 3rd patch 
in v2.

Thanks
Zhenzhong