[PATCH] powerpc: Fix initrd corruption with relative jump labels

[PATCH] powerpc: Fix initrd corruption with relative jump labels

[Michael Ellerman]

Commit b0b3b2c78ec0 ("powerpc: Switch to relative jump labels") switched
us to using relative jump labels. That involves changing the code,
target and key members in struct jump_entry to be relative to the
address of the jump_entry, rather than absolute addresses.

We have two static inlines that create a struct jump_entry,
arch_static_branch() and arch_static_branch_jump(), as well as an asm
macro ARCH_STATIC_BRANCH, which is used by the pseries-only hypervisor
tracing code.

Unfortunately we missed updating the key to be a relative reference in
ARCH_STATIC_BRANCH.

That causes a pseries kernel to have a handful of jump_entry structs
with bad key values. Instead of being a relative reference they instead
hold the full address of the key.

However the code doesn't expect that, it still adds the key value to the
address of the jump_entry (see jump_entry_key()) expecting to get a
pointer to a key somewhere in kernel data.

The table of jump_entry structs sits in rodata, which comes after the
kernel text. In a typical build this will be somewhere around 15MB. The
address of the key will be somewhere in data, typically around 20MB.
Adding the two values together gets us a pointer somewhere around 45MB.

We then call static_key_set_entries() with that bad pointer and modify
some members of the struct static_key we think we are pointing at.

A pseries kernel is typically ~30MB in size, so writing to ~45MB won't
corrupt the kernel itself. However if we're booting with an initrd,
depending on the size and exact location of the initrd, we can corrupt
the initrd. Depending on how exactly we corrupt the initrd it can either
cause the system to not boot, or just corrupt one of the files in the
initrd.

The fix is simply to make the key value relative to the jump_entry
struct in the ARCH_STATIC_BRANCH macro.

Fixes: b0b3b2c78ec0 ("powerpc: Switch to relative jump labels")
Reported-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
Reported-by: Roman Bolshakov <r.bolshakov@yadro.com>
Reported-by: Greg Kurz <groug@kaod.org>
Reported-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/jump_label.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
index 2d5c6bec2b4f..93ce3ec25387 100644
--- a/arch/powerpc/include/asm/jump_label.h
+++ b/arch/powerpc/include/asm/jump_label.h
@@ -50,7 +50,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
 1098:	nop;					\
 	.pushsection __jump_table, "aw";	\
 	.long 1098b - ., LABEL - .;		\
-	FTR_ENTRY_LONG KEY;			\
+	FTR_ENTRY_LONG KEY - .;			\
 	.popsection
 #endif
 
-- 
2.25.1

Re: [PATCH] powerpc: Fix initrd corruption with relative jump labels

[Greg Kurz]

On Mon, 14 Jun 2021 23:14:40 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Commit b0b3b2c78ec0 ("powerpc: Switch to relative jump labels") switched
> us to using relative jump labels. That involves changing the code,
> target and key members in struct jump_entry to be relative to the
> address of the jump_entry, rather than absolute addresses.
> 
> We have two static inlines that create a struct jump_entry,
> arch_static_branch() and arch_static_branch_jump(), as well as an asm
> macro ARCH_STATIC_BRANCH, which is used by the pseries-only hypervisor
> tracing code.
> 
> Unfortunately we missed updating the key to be a relative reference in
> ARCH_STATIC_BRANCH.
> 
> That causes a pseries kernel to have a handful of jump_entry structs
> with bad key values. Instead of being a relative reference they instead
> hold the full address of the key.
> 
> However the code doesn't expect that, it still adds the key value to the
> address of the jump_entry (see jump_entry_key()) expecting to get a
> pointer to a key somewhere in kernel data.
> 
> The table of jump_entry structs sits in rodata, which comes after the
> kernel text. In a typical build this will be somewhere around 15MB. The
> address of the key will be somewhere in data, typically around 20MB.
> Adding the two values together gets us a pointer somewhere around 45MB.
> 
> We then call static_key_set_entries() with that bad pointer and modify
> some members of the struct static_key we think we are pointing at.
> 
> A pseries kernel is typically ~30MB in size, so writing to ~45MB won't
> corrupt the kernel itself. However if we're booting with an initrd,
> depending on the size and exact location of the initrd, we can corrupt
> the initrd. Depending on how exactly we corrupt the initrd it can either
> cause the system to not boot, or just corrupt one of the files in the
> initrd.
> 
> The fix is simply to make the key value relative to the jump_entry
> struct in the ARCH_STATIC_BRANCH macro.
> 
> Fixes: b0b3b2c78ec0 ("powerpc: Switch to relative jump labels")
> Reported-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
> Reported-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Reported-by: Greg Kurz <groug@kaod.org>
> Reported-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---

Great thanks for debugging this issue ! I'll try it out tomorrow morning.

Cheers,

--
Greg

>  arch/powerpc/include/asm/jump_label.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
> index 2d5c6bec2b4f..93ce3ec25387 100644
> --- a/arch/powerpc/include/asm/jump_label.h
> +++ b/arch/powerpc/include/asm/jump_label.h
> @@ -50,7 +50,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
>  1098:	nop;					\
>  	.pushsection __jump_table, "aw";	\
>  	.long 1098b - ., LABEL - .;		\
> -	FTR_ENTRY_LONG KEY;			\
> +	FTR_ENTRY_LONG KEY - .;			\
>  	.popsection
>  #endif
>  

Re: [PATCH] powerpc: Fix initrd corruption with relative jump labels

[Greg Kurz]

On Mon, 14 Jun 2021 17:57:40 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Mon, 14 Jun 2021 23:14:40 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> > Commit b0b3b2c78ec0 ("powerpc: Switch to relative jump labels") switched
> > us to using relative jump labels. That involves changing the code,
> > target and key members in struct jump_entry to be relative to the
> > address of the jump_entry, rather than absolute addresses.
> > 
> > We have two static inlines that create a struct jump_entry,
> > arch_static_branch() and arch_static_branch_jump(), as well as an asm
> > macro ARCH_STATIC_BRANCH, which is used by the pseries-only hypervisor
> > tracing code.
> > 
> > Unfortunately we missed updating the key to be a relative reference in
> > ARCH_STATIC_BRANCH.
> > 
> > That causes a pseries kernel to have a handful of jump_entry structs
> > with bad key values. Instead of being a relative reference they instead
> > hold the full address of the key.
> > 
> > However the code doesn't expect that, it still adds the key value to the
> > address of the jump_entry (see jump_entry_key()) expecting to get a
> > pointer to a key somewhere in kernel data.
> > 
> > The table of jump_entry structs sits in rodata, which comes after the
> > kernel text. In a typical build this will be somewhere around 15MB. The
> > address of the key will be somewhere in data, typically around 20MB.
> > Adding the two values together gets us a pointer somewhere around 45MB.
> > 
> > We then call static_key_set_entries() with that bad pointer and modify
> > some members of the struct static_key we think we are pointing at.
> > 
> > A pseries kernel is typically ~30MB in size, so writing to ~45MB won't
> > corrupt the kernel itself. However if we're booting with an initrd,
> > depending on the size and exact location of the initrd, we can corrupt
> > the initrd. Depending on how exactly we corrupt the initrd it can either
> > cause the system to not boot, or just corrupt one of the files in the
> > initrd.
> > 
> > The fix is simply to make the key value relative to the jump_entry
> > struct in the ARCH_STATIC_BRANCH macro.
> > 
> > Fixes: b0b3b2c78ec0 ("powerpc: Switch to relative jump labels")
> > Reported-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
> > Reported-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > Reported-by: Greg Kurz <groug@kaod.org>
> > Reported-by: Daniel Axtens <dja@axtens.net>
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> 
> Great thanks for debugging this issue ! I'll try it out tomorrow morning.
> 

This fixes the issue. Great thanks again :)

Tested-by: Greg Kurz <groug@kaod.org>

> Cheers,
> 
> --
> Greg
> 
> >  arch/powerpc/include/asm/jump_label.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
> > index 2d5c6bec2b4f..93ce3ec25387 100644
> > --- a/arch/powerpc/include/asm/jump_label.h
> > +++ b/arch/powerpc/include/asm/jump_label.h
> > @@ -50,7 +50,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
> >  1098:	nop;					\
> >  	.pushsection __jump_table, "aw";	\
> >  	.long 1098b - ., LABEL - .;		\
> > -	FTR_ENTRY_LONG KEY;			\
> > +	FTR_ENTRY_LONG KEY - .;			\
> >  	.popsection
> >  #endif
> >  
> 

Re: [PATCH] powerpc: Fix initrd corruption with relative jump labels

[Daniel Axtens]

Hi Michael,

> The fix is simply to make the key value relative to the jump_entry
> struct in the ARCH_STATIC_BRANCH macro.

This fixes the boot issues I observed. Thank you very much!!

Tested-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

Re: [PATCH] powerpc: Fix initrd corruption with relative jump labels

[Anastasia Kovaleva]

> On 14 Jun 2021, at 16:14, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Commit b0b3b2c78ec0 ("powerpc: Switch to relative jump labels") switched
> us to using relative jump labels. That involves changing the code,
> target and key members in struct jump_entry to be relative to the
> address of the jump_entry, rather than absolute addresses.
> 
> We have two static inlines that create a struct jump_entry,
> arch_static_branch() and arch_static_branch_jump(), as well as an asm
> macro ARCH_STATIC_BRANCH, which is used by the pseries-only hypervisor
> tracing code.
> 
> Unfortunately we missed updating the key to be a relative reference in
> ARCH_STATIC_BRANCH.
> 
> That causes a pseries kernel to have a handful of jump_entry structs
> with bad key values. Instead of being a relative reference they instead
> hold the full address of the key.
> 
> However the code doesn't expect that, it still adds the key value to the
> address of the jump_entry (see jump_entry_key()) expecting to get a
> pointer to a key somewhere in kernel data.
> 
> The table of jump_entry structs sits in rodata, which comes after the
> kernel text. In a typical build this will be somewhere around 15MB. The
> address of the key will be somewhere in data, typically around 20MB.
> Adding the two values together gets us a pointer somewhere around 45MB.
> 
> We then call static_key_set_entries() with that bad pointer and modify
> some members of the struct static_key we think we are pointing at.
> 
> A pseries kernel is typically ~30MB in size, so writing to ~45MB won't
> corrupt the kernel itself. However if we're booting with an initrd,
> depending on the size and exact location of the initrd, we can corrupt
> the initrd. Depending on how exactly we corrupt the initrd it can either
> cause the system to not boot, or just corrupt one of the files in the
> initrd.
> 
> The fix is simply to make the key value relative to the jump_entry
> struct in the ARCH_STATIC_BRANCH macro.
> 
> Fixes: b0b3b2c78ec0 ("powerpc: Switch to relative jump labels")
> Reported-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
> Reported-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Reported-by: Greg Kurz <groug@kaod.org>
> Reported-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/include/asm/jump_label.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
> index 2d5c6bec2b4f..93ce3ec25387 100644
> --- a/arch/powerpc/include/asm/jump_label.h
> +++ b/arch/powerpc/include/asm/jump_label.h
> @@ -50,7 +50,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
> 1098:	nop;					\
> 	.pushsection __jump_table, "aw";	\
> 	.long 1098b - ., LABEL - .;		\
> -	FTR_ENTRY_LONG KEY;			\
> +	FTR_ENTRY_LONG KEY - .;			\
> 	.popsection
> #endif
> 
> -- 
> 2.25.1
> 

This fixes the issue. Thank you a lot!

Tested-by: Anastasia Kovaleva <a.kovaleva@yadro.com>

Thanks,
Anastasia

Re: [PATCH] powerpc: Fix initrd corruption with relative jump labels

[Roman Bolshakov]

On Mon, Jun 14, 2021 at 11:14:40PM +1000, Michael Ellerman wrote:
> Commit b0b3b2c78ec0 ("powerpc: Switch to relative jump labels") switched
> us to using relative jump labels. That involves changing the code,
> target and key members in struct jump_entry to be relative to the
> address of the jump_entry, rather than absolute addresses.
> 
> We have two static inlines that create a struct jump_entry,
> arch_static_branch() and arch_static_branch_jump(), as well as an asm
> macro ARCH_STATIC_BRANCH, which is used by the pseries-only hypervisor
> tracing code.
> 
> Unfortunately we missed updating the key to be a relative reference in
> ARCH_STATIC_BRANCH.
> 
> That causes a pseries kernel to have a handful of jump_entry structs
> with bad key values. Instead of being a relative reference they instead
> hold the full address of the key.
> 
> However the code doesn't expect that, it still adds the key value to the
> address of the jump_entry (see jump_entry_key()) expecting to get a
> pointer to a key somewhere in kernel data.
> 
> The table of jump_entry structs sits in rodata, which comes after the
> kernel text. In a typical build this will be somewhere around 15MB. The
> address of the key will be somewhere in data, typically around 20MB.
> Adding the two values together gets us a pointer somewhere around 45MB.
> 
> We then call static_key_set_entries() with that bad pointer and modify
> some members of the struct static_key we think we are pointing at.
> 
> A pseries kernel is typically ~30MB in size, so writing to ~45MB won't
> corrupt the kernel itself. However if we're booting with an initrd,
> depending on the size and exact location of the initrd, we can corrupt
> the initrd. Depending on how exactly we corrupt the initrd it can either
> cause the system to not boot, or just corrupt one of the files in the
> initrd.
> 
> The fix is simply to make the key value relative to the jump_entry
> struct in the ARCH_STATIC_BRANCH macro.
> 
> Fixes: b0b3b2c78ec0 ("powerpc: Switch to relative jump labels")
> Reported-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
> Reported-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Reported-by: Greg Kurz <groug@kaod.org>
> Reported-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/jump_label.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
> index 2d5c6bec2b4f..93ce3ec25387 100644
> --- a/arch/powerpc/include/asm/jump_label.h
> +++ b/arch/powerpc/include/asm/jump_label.h
> @@ -50,7 +50,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
>  1098:	nop;					\
>  	.pushsection __jump_table, "aw";	\
>  	.long 1098b - ., LABEL - .;		\
> -	FTR_ENTRY_LONG KEY;			\
> +	FTR_ENTRY_LONG KEY - .;			\
>  	.popsection
>  #endif
>  
> -- 
> 2.25.1
> 

Thanks for fixing the issue, Michael.

Perhaps the fix should go to v5.13-rc7 if Linus plans to do it. It'd be
good to avoid broken initrd on pseries guests in the release.

Regards,
Roman

Re: [PATCH] powerpc: Fix initrd corruption with relative jump labels

[Michael Ellerman]

Roman Bolshakov <r.bolshakov@yadro.com> writes:
> On Mon, Jun 14, 2021 at 11:14:40PM +1000, Michael Ellerman wrote:
>> Commit b0b3b2c78ec0 ("powerpc: Switch to relative jump labels") switched
>> us to using relative jump labels. That involves changing the code,
>> target and key members in struct jump_entry to be relative to the
>> address of the jump_entry, rather than absolute addresses.
>> 
>> We have two static inlines that create a struct jump_entry,
>> arch_static_branch() and arch_static_branch_jump(), as well as an asm
>> macro ARCH_STATIC_BRANCH, which is used by the pseries-only hypervisor
>> tracing code.
>> 
>> Unfortunately we missed updating the key to be a relative reference in
>> ARCH_STATIC_BRANCH.
>> 
>> That causes a pseries kernel to have a handful of jump_entry structs
>> with bad key values. Instead of being a relative reference they instead
>> hold the full address of the key.
>> 
>> However the code doesn't expect that, it still adds the key value to the
>> address of the jump_entry (see jump_entry_key()) expecting to get a
>> pointer to a key somewhere in kernel data.
>> 
>> The table of jump_entry structs sits in rodata, which comes after the
>> kernel text. In a typical build this will be somewhere around 15MB. The
>> address of the key will be somewhere in data, typically around 20MB.
>> Adding the two values together gets us a pointer somewhere around 45MB.
>> 
>> We then call static_key_set_entries() with that bad pointer and modify
>> some members of the struct static_key we think we are pointing at.
>> 
>> A pseries kernel is typically ~30MB in size, so writing to ~45MB won't
>> corrupt the kernel itself. However if we're booting with an initrd,
>> depending on the size and exact location of the initrd, we can corrupt
>> the initrd. Depending on how exactly we corrupt the initrd it can either
>> cause the system to not boot, or just corrupt one of the files in the
>> initrd.
>> 
>> The fix is simply to make the key value relative to the jump_entry
>> struct in the ARCH_STATIC_BRANCH macro.
>> 
>> Fixes: b0b3b2c78ec0 ("powerpc: Switch to relative jump labels")
>> Reported-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
>> Reported-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> Reported-by: Greg Kurz <groug@kaod.org>
>> Reported-by: Daniel Axtens <dja@axtens.net>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>  arch/powerpc/include/asm/jump_label.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
>> index 2d5c6bec2b4f..93ce3ec25387 100644
>> --- a/arch/powerpc/include/asm/jump_label.h
>> +++ b/arch/powerpc/include/asm/jump_label.h
>> @@ -50,7 +50,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool
>>  1098:	nop;					\
>>  	.pushsection __jump_table, "aw";	\
>>  	.long 1098b - ., LABEL - .;		\
>> -	FTR_ENTRY_LONG KEY;			\
>> +	FTR_ENTRY_LONG KEY - .;			\
>>  	.popsection
>>  #endif
>>  
>> -- 
>> 2.25.1
>> 
>
> Thanks for fixing the issue, Michael.
>
> Perhaps the fix should go to v5.13-rc7 if Linus plans to do it. It'd be
> good to avoid broken initrd on pseries guests in the release.

Yes it's in my fixes branch and I'll ask Linus to pull that before the
next rc.

cheers

Re: [PATCH] powerpc: Fix initrd corruption with relative jump labels

[Michael Ellerman]

On Mon, 14 Jun 2021 23:14:40 +1000, Michael Ellerman wrote:
> Commit b0b3b2c78ec0 ("powerpc: Switch to relative jump labels") switched
> us to using relative jump labels. That involves changing the code,
> target and key members in struct jump_entry to be relative to the
> address of the jump_entry, rather than absolute addresses.
> 
> We have two static inlines that create a struct jump_entry,
> arch_static_branch() and arch_static_branch_jump(), as well as an asm
> macro ARCH_STATIC_BRANCH, which is used by the pseries-only hypervisor
> tracing code.
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc: Fix initrd corruption with relative jump labels
      https://git.kernel.org/powerpc/c/478036c4cd1a16e613a2f883d79c03cf187faacb

cheers