[PATCH] x86/static_call: Fix __static_call_fixup()

[PATCH] x86/static_call: Fix __static_call_fixup()

[Peter Zijlstra]

Christian reported spurious module crashes after some of Song's module
memory layout patches.

Turns out that if the very last instruction on the very last page of the
module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
trip a fault and dies.

And while the module rework made this slightly more likely to happen,
it's always been possible.

Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
Reported-by: Christian Bricart <christian@bricart.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/static_call.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
index b70670a98597..2e67512d7104 100644
--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
  */
 bool __static_call_fixup(void *tramp, u8 op, void *dest)
 {
+	/*
+	 * Not all .return_sites are a static_call trampoline (most are not).
+	 * Check if the next 3 bytes are still kernel text, if not, then this
+	 * definitely is not a trampoline and we need not worry further.
+	 *
+	 * This avoids the memcmp() below tripping over pagefaults etc..
+	 */
+	if (!kernel_text_address(tramp+7))
+		return false;
+
 	if (memcmp(tramp+5, tramp_ud, 3)) {
 		/* Not a trampoline site, not our problem. */
 		return false;

Re: [PATCH] x86/static_call: Fix __static_call_fixup()

[Peter Zijlstra]

On Wed, Aug 16, 2023 at 01:08:10AM +0200, Peter Zijlstra wrote:
> 
> Christian reported spurious module crashes after some of Song's module

To clarify: module-load.

Obviously I shouldn't be writing Changelogs after 1am :-)

> memory layout patches.
> 
> Turns out that if the very last instruction on the very last page of the
> module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
> trip a fault and dies.
> 
> And while the module rework made this slightly more likely to happen,
> it's always been possible.
> 
> Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
> Reported-by: Christian Bricart <christian@bricart.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/static_call.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> index b70670a98597..2e67512d7104 100644
> --- a/arch/x86/kernel/static_call.c
> +++ b/arch/x86/kernel/static_call.c
> @@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
>   */
>  bool __static_call_fixup(void *tramp, u8 op, void *dest)
>  {
> +	/*
> +	 * Not all .return_sites are a static_call trampoline (most are not).
> +	 * Check if the next 3 bytes are still kernel text, if not, then this
> +	 * definitely is not a trampoline and we need not worry further.
> +	 *
> +	 * This avoids the memcmp() below tripping over pagefaults etc..
> +	 */
> +	if (!kernel_text_address(tramp+7))
> +		return false;
> +
>  	if (memcmp(tramp+5, tramp_ud, 3)) {
>  		/* Not a trampoline site, not our problem. */
>  		return false;

Re: [PATCH] x86/static_call: Fix __static_call_fixup()

[Josh Poimboeuf]

On Wed, Aug 16, 2023 at 01:08:09AM +0200, Peter Zijlstra wrote:
>  bool __static_call_fixup(void *tramp, u8 op, void *dest)
>  {
> +	/*
> +	 * Not all .return_sites are a static_call trampoline (most are not).
> +	 * Check if the next 3 bytes are still kernel text, if not, then this

s/3/7 ?

> +	 * definitely is not a trampoline and we need not worry further.
> +	 *
> +	 * This avoids the memcmp() below tripping over pagefaults etc..
> +	 */
> +	if (!kernel_text_address(tramp+7))
> +		return false;
> +
>  	if (memcmp(tramp+5, tramp_ud, 3)) {
>  		/* Not a trampoline site, not our problem. */
>  		return false;

kernel_text_address() can be quite heavyweight to call in a loop during
module loading.  Maybe that doesn't matter much.  But it would be a lot
faster to only call kernel_text_address() if tramp+7 is on the next
page.

-- 
Josh

Re: [PATCH] x86/static_call: Fix __static_call_fixup()

[Peter Zijlstra]

On Tue, Aug 15, 2023 at 05:10:32PM -0700, Josh Poimboeuf wrote:
> On Wed, Aug 16, 2023 at 01:08:09AM +0200, Peter Zijlstra wrote:
> >  bool __static_call_fixup(void *tramp, u8 op, void *dest)
> >  {
> > +	/*
> > +	 * Not all .return_sites are a static_call trampoline (most are not).
> > +	 * Check if the next 3 bytes are still kernel text, if not, then this
> 
> s/3/7 ?

Right, so what I meant was the 3 bytes after the return, which is 5+3,
but yeah, that can be said better.

> 
> > +	 * definitely is not a trampoline and we need not worry further.
> > +	 *
> > +	 * This avoids the memcmp() below tripping over pagefaults etc..
> > +	 */
> > +	if (!kernel_text_address(tramp+7))
> > +		return false;
> > +
> >  	if (memcmp(tramp+5, tramp_ud, 3)) {
> >  		/* Not a trampoline site, not our problem. */
> >  		return false;
> 
> kernel_text_address() can be quite heavyweight to call in a loop during
> module loading.  Maybe that doesn't matter much.  But it would be a lot
> faster to only call kernel_text_address() if tramp+7 is on the next
> page.

Oh, right, in those few configs where it doesn't use the tree. Sure can
do.

Re: [PATCH] x86/static_call: Fix __static_call_fixup()

[Steven Rostedt]

On Wed, 16 Aug 2023 01:08:09 +0200
Peter Zijlstra <peterz@infradead.org> wrote:


> diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> index b70670a98597..2e67512d7104 100644
> --- a/arch/x86/kernel/static_call.c
> +++ b/arch/x86/kernel/static_call.c
> @@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
>   */
>  bool __static_call_fixup(void *tramp, u8 op, void *dest)
>  {
> +	/*
> +	 * Not all .return_sites are a static_call trampoline (most are not).
> +	 * Check if the next 3 bytes are still kernel text, if not, then this
> +	 * definitely is not a trampoline and we need not worry further.
> +	 *
> +	 * This avoids the memcmp() below tripping over pagefaults etc..
> +	 */
> +	if (!kernel_text_address(tramp+7))

The comment says "next 3 bytes" and the test is "tramp+7". Why the magic 7 number?

If the tramp is 5 bytes, shouldn't it be +8?

-- Steve


> +		return false;
> +
>  	if (memcmp(tramp+5, tramp_ud, 3)) {
>  		/* Not a trampoline site, not our problem. */
>  		return false;

Re: [PATCH] x86/static_call: Fix __static_call_fixup()

[Peter Zijlstra]

On Tue, Aug 15, 2023 at 08:41:12PM -0400, Steven Rostedt wrote:
> On Wed, 16 Aug 2023 01:08:09 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> 
> > diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
> > index b70670a98597..2e67512d7104 100644
> > --- a/arch/x86/kernel/static_call.c
> > +++ b/arch/x86/kernel/static_call.c
> > @@ -186,6 +186,16 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
> >   */
> >  bool __static_call_fixup(void *tramp, u8 op, void *dest)
> >  {
> > +	/*
> > +	 * Not all .return_sites are a static_call trampoline (most are not).
> > +	 * Check if the next 3 bytes are still kernel text, if not, then this
> > +	 * definitely is not a trampoline and we need not worry further.
> > +	 *
> > +	 * This avoids the memcmp() below tripping over pagefaults etc..
> > +	 */
> > +	if (!kernel_text_address(tramp+7))
> 
> The comment says "next 3 bytes" and the test is "tramp+7". Why the magic 7 number?
> 
> If the tramp is 5 bytes, shouldn't it be +8?

0 based, 7 is the last of the 8 bytes. +8 would be one beyond.

[PATCH v2] x86/static_call: Fix __static_call_fixup()

[Peter Zijlstra]

Christian reported spurious module load crashes after some of Song's
module memory layout patches.

Turns out that if the very last instruction on the very last page of the
module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
trip a fault and die.

And while the module rework made this slightly more likely to happen,
it's always been possible.

Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
Reported-by: Christian Bricart <christian@bricart.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/static_call.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -186,6 +186,19 @@ EXPORT_SYMBOL_GPL(arch_static_call_trans
  */
 bool __static_call_fixup(void *tramp, u8 op, void *dest)
 {
+	unsigned long addr = (unsigned long)tramp;
+	/*
+	 * Not all .return_sites are a static_call trampoline (most are not).
+	 * Check if the 3 bytes after the return are still kernel text, if not,
+	 * then this definitely is not a trampoline and we need not worry
+	 * further.
+	 *
+	 * This avoids the memcmp() below tripping over pagefaults etc..
+	 */
+	if (((addr >> PAGE_SHIFT) != ((addr + 7) >> PAGE_SHIFT)) &&
+	    !kernel_text_address(addr + 7))
+		return false;
+
 	if (memcmp(tramp+5, tramp_ud, 3)) {
 		/* Not a trampoline site, not our problem. */
 		return false;

Re: [PATCH v2] x86/static_call: Fix __static_call_fixup()

[Josh Poimboeuf]

On Wed, Aug 16, 2023 at 12:44:19PM +0200, Peter Zijlstra wrote:
> 
> Christian reported spurious module load crashes after some of Song's
> module memory layout patches.
> 
> Turns out that if the very last instruction on the very last page of the
> module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
> trip a fault and die.
> 
> And while the module rework made this slightly more likely to happen,
> it's always been possible.
> 
> Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
> Reported-by: Christian Bricart <christian@bricart.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>

-- 
Josh

[tip: x86/urgent] x86/static_call: Fix __static_call_fixup()

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     54097309620ef0dc2d7083783dc521c6a5fef957
Gitweb:        https://git.kernel.org/tip/54097309620ef0dc2d7083783dc521c6a5fef957
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Wed, 16 Aug 2023 12:44:19 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 17 Aug 2023 13:24:09 +02:00

x86/static_call: Fix __static_call_fixup()

Christian reported spurious module load crashes after some of Song's
module memory layout patches.

Turns out that if the very last instruction on the very last page of the
module is a 'JMP __x86_return_thunk' then __static_call_fixup() will
trip a fault and die.

And while the module rework made this slightly more likely to happen,
it's always been possible.

Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
Reported-by: Christian Bricart <christian@bricart.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>
Link: https://lkml.kernel.org/r/20230816104419.GA982867@hirez.programming.kicks-ass.net
---
 arch/x86/kernel/static_call.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c
index b70670a..77a9316 100644
--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -186,6 +186,19 @@ EXPORT_SYMBOL_GPL(arch_static_call_transform);
  */
 bool __static_call_fixup(void *tramp, u8 op, void *dest)
 {
+	unsigned long addr = (unsigned long)tramp;
+	/*
+	 * Not all .return_sites are a static_call trampoline (most are not).
+	 * Check if the 3 bytes after the return are still kernel text, if not,
+	 * then this definitely is not a trampoline and we need not worry
+	 * further.
+	 *
+	 * This avoids the memcmp() below tripping over pagefaults etc..
+	 */
+	if (((addr >> PAGE_SHIFT) != ((addr + 7) >> PAGE_SHIFT)) &&
+	    !kernel_text_address(addr + 7))
+		return false;
+
 	if (memcmp(tramp+5, tramp_ud, 3)) {
 		/* Not a trampoline site, not our problem. */
 		return false;