[PATCH RFC] s390: Fix nospec table alignments

[PATCH RFC] s390: Fix nospec table alignments

[Josh Poimboeuf]

Add proper alignment for .nospec_call_table and .nospec_return_table in
vmlinux.

Fixes: f19fbd5ed642 ("s390: introduce execute-trampolines for branches")
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
This is RFC because I don't know anything about s390 behavior for
unaligned data accesses, but this seemed to fix an issue for me.

While working on another s390 issue, I was getting intermittent boot
failures in __nospec_revert() when it tried to access 'instr[0]'.  I
noticed the __nospec_call_start address ended in 'ff'.  This patch
seemed to fix it.  I have no idea why it was (only sometimes) failing in
the first place.

The intermittent part of it is probably at least partially explained by
CONFIG_RANDOMIZE_BASE.  Except now I can no longer recreate it :-/

Regardless, this patch seems correct.  I just can't explain what I saw.
Any ideas?

 arch/s390/kernel/vmlinux.lds.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
index 2e526f11b91e..5ea3830af0cc 100644
--- a/arch/s390/kernel/vmlinux.lds.S
+++ b/arch/s390/kernel/vmlinux.lds.S
@@ -131,6 +131,7 @@ SECTIONS
 	/*
 	 * Table with the patch locations to undo expolines
 	*/
+	. = ALIGN(4);
 	.nospec_call_table : {
 		__nospec_call_start = . ;
 		*(.s390_indirect*)
-- 
2.37.2

Re: [PATCH RFC] s390: Fix nospec table alignments

[Heiko Carstens]

On Fri, Aug 26, 2022 at 04:55:44PM -0700, Josh Poimboeuf wrote:
> Add proper alignment for .nospec_call_table and .nospec_return_table in
> vmlinux.
> 
> Fixes: f19fbd5ed642 ("s390: introduce execute-trampolines for branches")
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
> This is RFC because I don't know anything about s390 behavior for
> unaligned data accesses, but this seemed to fix an issue for me.
> 
> While working on another s390 issue, I was getting intermittent boot
> failures in __nospec_revert() when it tried to access 'instr[0]'.  I
> noticed the __nospec_call_start address ended in 'ff'.  This patch
> seemed to fix it.  I have no idea why it was (only sometimes) failing in
> the first place.
> 
> The intermittent part of it is probably at least partially explained by
> CONFIG_RANDOMIZE_BASE.  Except now I can no longer recreate it :-/
> 
> Regardless, this patch seems correct.  I just can't explain what I saw.
> Any ideas?
> 
>  arch/s390/kernel/vmlinux.lds.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
> index 2e526f11b91e..5ea3830af0cc 100644
> --- a/arch/s390/kernel/vmlinux.lds.S
> +++ b/arch/s390/kernel/vmlinux.lds.S
> @@ -131,6 +131,7 @@ SECTIONS
>  	/*
>  	 * Table with the patch locations to undo expolines
>  	*/
> +	. = ALIGN(4);
>  	.nospec_call_table : {
>  		__nospec_call_start = . ;
>  		*(.s390_indirect*)

On s390 labels must be at an even address due to instructions like
"larl" (load address relative long), which generate a pc-relative
address adding the number of words encoded into the instruction to the
current IP.

With commit e6ed91fd0768 ("s390/alternatives: remove padding
generation code") I managed to reduce the size of struct alt_instr by
one byte, so it is now only 11 bytes in size. So depending on the size
/ number of members of the __alt_instructions array nospec_call_table
starts at an uneven address, which would explain this.

Unfortunately I was unable to let any compiler generate code, that
would use the larl instruction. Instead the address of
nospec_call_table was loaded indirectly via the GOT, which again works
always, regardless if the table starts at an even or uneven address.

This needs to be fixed anyway, and your patch certainly is correct.

Could you maybe share your kernel config + compiler version, if you
are still able to reproduce this?

Re: [PATCH RFC] s390: Fix nospec table alignments

[Josh Poimboeuf]

On Sat, Aug 27, 2022 at 08:23:17PM +0200, Heiko Carstens wrote:
> On Fri, Aug 26, 2022 at 04:55:44PM -0700, Josh Poimboeuf wrote:
> > Add proper alignment for .nospec_call_table and .nospec_return_table in
> > vmlinux.
> > 
> > Fixes: f19fbd5ed642 ("s390: introduce execute-trampolines for branches")
> > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> > ---
> > This is RFC because I don't know anything about s390 behavior for
> > unaligned data accesses, but this seemed to fix an issue for me.
> > 
> > While working on another s390 issue, I was getting intermittent boot
> > failures in __nospec_revert() when it tried to access 'instr[0]'.  I
> > noticed the __nospec_call_start address ended in 'ff'.  This patch
> > seemed to fix it.  I have no idea why it was (only sometimes) failing in
> > the first place.
> > 
> > The intermittent part of it is probably at least partially explained by
> > CONFIG_RANDOMIZE_BASE.  Except now I can no longer recreate it :-/
> > 
> > Regardless, this patch seems correct.  I just can't explain what I saw.
> > Any ideas?
> > 
> >  arch/s390/kernel/vmlinux.lds.S | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/s390/kernel/vmlinux.lds.S b/arch/s390/kernel/vmlinux.lds.S
> > index 2e526f11b91e..5ea3830af0cc 100644
> > --- a/arch/s390/kernel/vmlinux.lds.S
> > +++ b/arch/s390/kernel/vmlinux.lds.S
> > @@ -131,6 +131,7 @@ SECTIONS
> >  	/*
> >  	 * Table with the patch locations to undo expolines
> >  	*/
> > +	. = ALIGN(4);
> >  	.nospec_call_table : {
> >  		__nospec_call_start = . ;
> >  		*(.s390_indirect*)
> 
> On s390 labels must be at an even address due to instructions like
> "larl" (load address relative long), which generate a pc-relative
> address adding the number of words encoded into the instruction to the
> current IP.
> 
> With commit e6ed91fd0768 ("s390/alternatives: remove padding
> generation code") I managed to reduce the size of struct alt_instr by
> one byte, so it is now only 11 bytes in size. So depending on the size
> / number of members of the __alt_instructions array nospec_call_table
> starts at an uneven address, which would explain this.
> 
> Unfortunately I was unable to let any compiler generate code, that
> would use the larl instruction. Instead the address of
> nospec_call_table was loaded indirectly via the GOT, which again works
> always, regardless if the table starts at an even or uneven address.
> 
> This needs to be fixed anyway, and your patch certainly is correct.
> 
> Could you maybe share your kernel config + compiler version, if you
> are still able to reproduce this?

I think the trick is to disable CONFIG_RELOCATABLE.  When I compile with
CONFIG_RELOCATABLE=n and "gcc version 11.3.1 20220421 (Red Hat 11.3.1-2)
(GCC)", I get the following in nospec_init_branches():

 2a8:   c0 20 00 00 00 00       larl    %r2,2a8 <nospec_init_branches+0x30>     2aa: R_390_PC32DBL      __nospec_call_start+0x2

That said, I still haven't been able to figure out how to recreate the
program check in __nospec_revert(), even when the nospec_call_table
starts at an odd offset.

BTW, I only discovered this when testing with my pending patches which
propose getting rid of -fPIE for CONFIG_RELOCATABLE, so that more than
64k sections can be supported.  But that's a separate topic :-)

-- 
Josh

Re: [PATCH RFC] s390: Fix nospec table alignments

[Heiko Carstens]

On Sat, Aug 27, 2022 at 03:59:37PM -0700, Josh Poimboeuf wrote:
> > > While working on another s390 issue, I was getting intermittent boot
> > > failures in __nospec_revert() when it tried to access 'instr[0]'.  I
> > > noticed the __nospec_call_start address ended in 'ff'.  This patch
> > > seemed to fix it.  I have no idea why it was (only sometimes) failing in
> > > the first place.
...
> > > +	. = ALIGN(4);
> > >  	.nospec_call_table : {
> > >  		__nospec_call_start = . ;
> > >  		*(.s390_indirect*)
> > 
...
> > Unfortunately I was unable to let any compiler generate code, that
> > would use the larl instruction. Instead the address of
> > nospec_call_table was loaded indirectly via the GOT, which again works
> > always, regardless if the table starts at an even or uneven address.
> > 
> > This needs to be fixed anyway, and your patch certainly is correct.
> > 
> > Could you maybe share your kernel config + compiler version, if you
> > are still able to reproduce this?
> 
> I think the trick is to disable CONFIG_RELOCATABLE.  When I compile with
> CONFIG_RELOCATABLE=n and "gcc version 11.3.1 20220421 (Red Hat 11.3.1-2)
> (GCC)", I get the following in nospec_init_branches():
> 
>  2a8:   c0 20 00 00 00 00       larl    %r2,2a8 <nospec_init_branches+0x30>     2aa: R_390_PC32DBL      __nospec_call_start+0x2
> 
> That said, I still haven't been able to figure out how to recreate the
> program check in __nospec_revert(), even when the nospec_call_table
> starts at an odd offset.

Right, CONFIG_RELOCATABLE=n will do the trick.

I don't know why you cannot recreate it, however on my system it
crashes instantly when I make sure that __nospec_call_start starts at
an odd address.

Apparently 'instr = (u8 *) epo + *epo;' in __nospec_revert() may
result in a very large address, since without KASLR the kernel is
located at a low address, and it only takes one entry within the
incorrectly accessed nospec_call_table which results in a large
negative value for '*epo' and we end up with an overflow and a very
large address for 'instr'.
This will then result in the program check / addressing exception
you've seen when the kernel tried to access 'instr[0]'.

I'll apply your patch. Thanks a lot for debugging and reporting!