diff mbox series

[BUG] crypto: arm64: Avoid indirect branch to bti_c

Message ID 20201006034854.2277538-1-jeremy.linton@arm.com
State Superseded
Headers show
Series [BUG] crypto: arm64: Avoid indirect branch to bti_c | expand

Commit Message

Jeremy Linton Oct. 6, 2020, 3:48 a.m. UTC
The AES code uses a 'br x7' as part of a function called by
a macro. That branch needs a bti_j as a target. This results
in a panic as seen below. Instead of trying to replace the branch
target with a bti_jc, lets replace the indirect branch with a
bl/ret, bl sequence that can target the existing bti_c.

  Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI
  CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1
  pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)
  pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
  lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]
  sp : ffff80001052b730

  aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
   __xts_crypt+0xb0/0x2dc [aes_neon_bs]
   xts_encrypt+0x28/0x3c [aes_neon_bs]
  crypto_skcipher_encrypt+0x50/0x84
  simd_skcipher_encrypt+0xc8/0xe0
  crypto_skcipher_encrypt+0x50/0x84
  test_skcipher_vec_cfg+0x224/0x5f0
  test_skcipher+0xbc/0x120
  alg_test_skcipher+0xa0/0x1b0
  alg_test+0x3dc/0x47c
  cryptomgr_test+0x38/0x60

Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/crypto/aes-neonbs-core.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Will Deacon Oct. 6, 2020, 8:27 a.m. UTC | #1
On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:
> The AES code uses a 'br x7' as part of a function called by

> a macro. That branch needs a bti_j as a target. This results

> in a panic as seen below. Instead of trying to replace the branch

> target with a bti_jc, lets replace the indirect branch with a

> bl/ret, bl sequence that can target the existing bti_c.

> 

>   Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI

>   CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1

>   pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)

>   pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

>   lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]

>   sp : ffff80001052b730

> 

>   aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

>    __xts_crypt+0xb0/0x2dc [aes_neon_bs]

>    xts_encrypt+0x28/0x3c [aes_neon_bs]

>   crypto_skcipher_encrypt+0x50/0x84

>   simd_skcipher_encrypt+0xc8/0xe0

>   crypto_skcipher_encrypt+0x50/0x84

>   test_skcipher_vec_cfg+0x224/0x5f0

>   test_skcipher+0xbc/0x120

>   alg_test_skcipher+0xa0/0x1b0

>   alg_test+0x3dc/0x47c

>   cryptomgr_test+0x38/0x60

> 

> Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")


nit: the "commit" string shouldn't be here, and I think the linux-next
scripts will yell at us if we don't remove it.

> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> ---

>  arch/arm64/crypto/aes-neonbs-core.S | 6 +++---

>  1 file changed, 3 insertions(+), 3 deletions(-)

> 

> diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S

> index b357164379f6..32f53ebe5e2c 100644

> --- a/arch/arm64/crypto/aes-neonbs-core.S

> +++ b/arch/arm64/crypto/aes-neonbs-core.S

> @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)

>  

>  0:	mov		bskey, x21

>  	mov		rounds, x22

> -	br		x7

> +	ret

>  SYM_FUNC_END(__xts_crypt8)

>  

>  	.macro		__xts_crypt, do8, o0, o1, o2, o3, o4, o5, o6, o7

> @@ -806,8 +806,8 @@ SYM_FUNC_END(__xts_crypt8)

>  	uzp1		v30.4s, v30.4s, v25.4s

>  	ld1		{v25.16b}, [x24]

>  

> -99:	adr		x7, \do8

> -	bl		__xts_crypt8

> +99:	bl		__xts_crypt8

> +	bl 		\do8

>  

>  	ldp		q16, q17, [sp, #.Lframe_local_offset]

>  	ldp		q18, q19, [sp, #.Lframe_local_offset + 32]


Acked-by: Will Deacon <will@kernel.org>


Catalin -- can you pick this for 5.9 please?

Will
Dave Martin Oct. 6, 2020, 10:01 a.m. UTC | #2
On Tue, Oct 06, 2020 at 09:27:48AM +0100, Will Deacon wrote:
> On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:
> > The AES code uses a 'br x7' as part of a function called by
> > a macro. That branch needs a bti_j as a target. This results
> > in a panic as seen below. Instead of trying to replace the branch
> > target with a bti_jc, lets replace the indirect branch with a
> > bl/ret, bl sequence that can target the existing bti_c.
> > 
> >   Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI
> >   CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1
> >   pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)
> >   pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> >   lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]
> >   sp : ffff80001052b730
> > 
> >   aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> >    __xts_crypt+0xb0/0x2dc [aes_neon_bs]
> >    xts_encrypt+0x28/0x3c [aes_neon_bs]
> >   crypto_skcipher_encrypt+0x50/0x84
> >   simd_skcipher_encrypt+0xc8/0xe0
> >   crypto_skcipher_encrypt+0x50/0x84
> >   test_skcipher_vec_cfg+0x224/0x5f0
> >   test_skcipher+0xbc/0x120
> >   alg_test_skcipher+0xa0/0x1b0
> >   alg_test+0x3dc/0x47c
> >   cryptomgr_test+0x38/0x60
> > 
> > Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")
> 
> nit: the "commit" string shouldn't be here, and I think the linux-next
> scripts will yell at us if we don't remove it.
> 
> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > ---
> >  arch/arm64/crypto/aes-neonbs-core.S | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S
> > index b357164379f6..32f53ebe5e2c 100644
> > --- a/arch/arm64/crypto/aes-neonbs-core.S
> > +++ b/arch/arm64/crypto/aes-neonbs-core.S
> > @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)
> >  
> >  0:	mov		bskey, x21
> >  	mov		rounds, x22
> > -	br		x7
> > +	ret

Dang, replied on an old version.

Since this is logically a tail call, could we simply be using br x16 or
br x17 for this?

The architecture makes special provision for that so that the compiler
can generate tail-calls.


This assumes that those regs aren't clobbered by any veneered function
call in the meantime, but all the calls here are local, so I don't think
that is a concern.

[...]

Cheers
---Dave
Will Deacon Oct. 6, 2020, 10:21 a.m. UTC | #3
On Tue, Oct 06, 2020 at 11:01:21AM +0100, Dave Martin wrote:
> On Tue, Oct 06, 2020 at 09:27:48AM +0100, Will Deacon wrote:

> > On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:

> > > The AES code uses a 'br x7' as part of a function called by

> > > a macro. That branch needs a bti_j as a target. This results

> > > in a panic as seen below. Instead of trying to replace the branch

> > > target with a bti_jc, lets replace the indirect branch with a

> > > bl/ret, bl sequence that can target the existing bti_c.

> > > 

> > >   Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI

> > >   CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1

> > >   pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)

> > >   pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

> > >   lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]

> > >   sp : ffff80001052b730

> > > 

> > >   aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

> > >    __xts_crypt+0xb0/0x2dc [aes_neon_bs]

> > >    xts_encrypt+0x28/0x3c [aes_neon_bs]

> > >   crypto_skcipher_encrypt+0x50/0x84

> > >   simd_skcipher_encrypt+0xc8/0xe0

> > >   crypto_skcipher_encrypt+0x50/0x84

> > >   test_skcipher_vec_cfg+0x224/0x5f0

> > >   test_skcipher+0xbc/0x120

> > >   alg_test_skcipher+0xa0/0x1b0

> > >   alg_test+0x3dc/0x47c

> > >   cryptomgr_test+0x38/0x60

> > > 

> > > Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")

> > 

> > nit: the "commit" string shouldn't be here, and I think the linux-next

> > scripts will yell at us if we don't remove it.

> > 

> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > > ---

> > >  arch/arm64/crypto/aes-neonbs-core.S | 6 +++---

> > >  1 file changed, 3 insertions(+), 3 deletions(-)

> > > 

> > > diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S

> > > index b357164379f6..32f53ebe5e2c 100644

> > > --- a/arch/arm64/crypto/aes-neonbs-core.S

> > > +++ b/arch/arm64/crypto/aes-neonbs-core.S

> > > @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)

> > >  

> > >  0:	mov		bskey, x21

> > >  	mov		rounds, x22

> > > -	br		x7

> > > +	ret

> 

> Dang, replied on an old version.


They're not versioned, so who knows which one is older!

> Since this is logically a tail call, could we simply be using br x16 or

> br x17 for this?


Yup, that would work too. This was just "obviously correct" and it would
be nice to get a fix in for 5.9.

Will
Catalin Marinas Oct. 6, 2020, 10:25 a.m. UTC | #4
On Tue, Oct 06, 2020 at 11:01:21AM +0100, Dave P Martin wrote:
> On Tue, Oct 06, 2020 at 09:27:48AM +0100, Will Deacon wrote:
> > On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:
> > > The AES code uses a 'br x7' as part of a function called by
> > > a macro. That branch needs a bti_j as a target. This results
> > > in a panic as seen below. Instead of trying to replace the branch
> > > target with a bti_jc, lets replace the indirect branch with a
> > > bl/ret, bl sequence that can target the existing bti_c.
> > > 
> > >   Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI
> > >   CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1
> > >   pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)
> > >   pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> > >   lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]
> > >   sp : ffff80001052b730
> > > 
> > >   aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> > >    __xts_crypt+0xb0/0x2dc [aes_neon_bs]
> > >    xts_encrypt+0x28/0x3c [aes_neon_bs]
> > >   crypto_skcipher_encrypt+0x50/0x84
> > >   simd_skcipher_encrypt+0xc8/0xe0
> > >   crypto_skcipher_encrypt+0x50/0x84
> > >   test_skcipher_vec_cfg+0x224/0x5f0
> > >   test_skcipher+0xbc/0x120
> > >   alg_test_skcipher+0xa0/0x1b0
> > >   alg_test+0x3dc/0x47c
> > >   cryptomgr_test+0x38/0x60
> > > 
> > > Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")
> > 
> > nit: the "commit" string shouldn't be here, and I think the linux-next
> > scripts will yell at us if we don't remove it.
> > 
> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > ---
> > >  arch/arm64/crypto/aes-neonbs-core.S | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S
> > > index b357164379f6..32f53ebe5e2c 100644
> > > --- a/arch/arm64/crypto/aes-neonbs-core.S
> > > +++ b/arch/arm64/crypto/aes-neonbs-core.S
> > > @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)
> > >  
> > >  0:	mov		bskey, x21
> > >  	mov		rounds, x22
> > > -	br		x7
> > > +	ret
> 
> Dang, replied on an old version.

Which I ignored (by default, when the kbuild test robot complains ;)).

> Since this is logically a tail call, could we simply be using br x16 or
> br x17 for this?
> 
> The architecture makes special provision for that so that the compiler
> can generate tail-calls.

So a "br x16" is compatible with a bti_c landing pad. I think it makes
more sense to keep it as a tail call.
Mark Brown Oct. 6, 2020, 10:35 a.m. UTC | #5
On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:
> The AES code uses a 'br x7' as part of a function called by

> a macro. That branch needs a bti_j as a target. This results

> in a panic as seen below. Instead of trying to replace the branch

> target with a bti_jc, lets replace the indirect branch with a

> bl/ret, bl sequence that can target the existing bti_c.


Reviewed-by: Mark Brown <broonie@kernel.org>
Dave Martin Oct. 6, 2020, 10:43 a.m. UTC | #6
On Tue, Oct 06, 2020 at 11:25:11AM +0100, Catalin Marinas wrote:
> On Tue, Oct 06, 2020 at 11:01:21AM +0100, Dave P Martin wrote:
> > On Tue, Oct 06, 2020 at 09:27:48AM +0100, Will Deacon wrote:
> > > On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:
> > > > The AES code uses a 'br x7' as part of a function called by
> > > > a macro. That branch needs a bti_j as a target. This results
> > > > in a panic as seen below. Instead of trying to replace the branch
> > > > target with a bti_jc, lets replace the indirect branch with a
> > > > bl/ret, bl sequence that can target the existing bti_c.
> > > > 
> > > >   Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI
> > > >   CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1
> > > >   pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)
> > > >   pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> > > >   lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]
> > > >   sp : ffff80001052b730
> > > > 
> > > >   aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> > > >    __xts_crypt+0xb0/0x2dc [aes_neon_bs]
> > > >    xts_encrypt+0x28/0x3c [aes_neon_bs]
> > > >   crypto_skcipher_encrypt+0x50/0x84
> > > >   simd_skcipher_encrypt+0xc8/0xe0
> > > >   crypto_skcipher_encrypt+0x50/0x84
> > > >   test_skcipher_vec_cfg+0x224/0x5f0
> > > >   test_skcipher+0xbc/0x120
> > > >   alg_test_skcipher+0xa0/0x1b0
> > > >   alg_test+0x3dc/0x47c
> > > >   cryptomgr_test+0x38/0x60
> > > > 
> > > > Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")
> > > 
> > > nit: the "commit" string shouldn't be here, and I think the linux-next
> > > scripts will yell at us if we don't remove it.
> > > 
> > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > > ---
> > > >  arch/arm64/crypto/aes-neonbs-core.S | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S
> > > > index b357164379f6..32f53ebe5e2c 100644
> > > > --- a/arch/arm64/crypto/aes-neonbs-core.S
> > > > +++ b/arch/arm64/crypto/aes-neonbs-core.S
> > > > @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)
> > > >  
> > > >  0:	mov		bskey, x21
> > > >  	mov		rounds, x22
> > > > -	br		x7
> > > > +	ret
> > 
> > Dang, replied on an old version.
> 
> Which I ignored (by default, when the kbuild test robot complains ;)).
> 
> > Since this is logically a tail call, could we simply be using br x16 or
> > br x17 for this?
> > 
> > The architecture makes special provision for that so that the compiler
> > can generate tail-calls.
> 
> So a "br x16" is compatible with a bti_c landing pad. I think it makes
> more sense to keep it as a tail call.

Just to be clear, I'm happy either way, but I thought it would make
sense to point this out.

Normally, "bti j" would be used just for weird stuff like jump tables,
but .S files all count as "weird stuff" to some extent -- so there are
no hard and fast rules.

Cheers
---Dave
Catalin Marinas Oct. 6, 2020, 12:33 p.m. UTC | #7
On Tue, Oct 06, 2020 at 11:43:14AM +0100, Dave P Martin wrote:
> On Tue, Oct 06, 2020 at 11:25:11AM +0100, Catalin Marinas wrote:

> > On Tue, Oct 06, 2020 at 11:01:21AM +0100, Dave P Martin wrote:

> > > On Tue, Oct 06, 2020 at 09:27:48AM +0100, Will Deacon wrote:

> > > > On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:

> > > > > The AES code uses a 'br x7' as part of a function called by

> > > > > a macro. That branch needs a bti_j as a target. This results

> > > > > in a panic as seen below. Instead of trying to replace the branch

> > > > > target with a bti_jc, lets replace the indirect branch with a

> > > > > bl/ret, bl sequence that can target the existing bti_c.

> > > > > 

> > > > >   Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI

> > > > >   CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1

> > > > >   pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)

> > > > >   pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

> > > > >   lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]

> > > > >   sp : ffff80001052b730

> > > > > 

> > > > >   aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

> > > > >    __xts_crypt+0xb0/0x2dc [aes_neon_bs]

> > > > >    xts_encrypt+0x28/0x3c [aes_neon_bs]

> > > > >   crypto_skcipher_encrypt+0x50/0x84

> > > > >   simd_skcipher_encrypt+0xc8/0xe0

> > > > >   crypto_skcipher_encrypt+0x50/0x84

> > > > >   test_skcipher_vec_cfg+0x224/0x5f0

> > > > >   test_skcipher+0xbc/0x120

> > > > >   alg_test_skcipher+0xa0/0x1b0

> > > > >   alg_test+0x3dc/0x47c

> > > > >   cryptomgr_test+0x38/0x60

> > > > > 

> > > > > Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")

> > > > 

> > > > nit: the "commit" string shouldn't be here, and I think the linux-next

> > > > scripts will yell at us if we don't remove it.

> > > > 

> > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> > > > > ---

> > > > >  arch/arm64/crypto/aes-neonbs-core.S | 6 +++---

> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)

> > > > > 

> > > > > diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S

> > > > > index b357164379f6..32f53ebe5e2c 100644

> > > > > --- a/arch/arm64/crypto/aes-neonbs-core.S

> > > > > +++ b/arch/arm64/crypto/aes-neonbs-core.S

> > > > > @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)

> > > > >  

> > > > >  0:	mov		bskey, x21

> > > > >  	mov		rounds, x22

> > > > > -	br		x7

> > > > > +	ret

> > > 

> > > Dang, replied on an old version.

> > 

> > Which I ignored (by default, when the kbuild test robot complains ;)).

> > 

> > > Since this is logically a tail call, could we simply be using br x16 or

> > > br x17 for this?

> > > 

> > > The architecture makes special provision for that so that the compiler

> > > can generate tail-calls.

> > 

> > So a "br x16" is compatible with a bti_c landing pad. I think it makes

> > more sense to keep it as a tail call.

> 

> Just to be clear, I'm happy either way, but I thought it would make

> sense to point this out.


I'd prefer the replacement with a br x16/17, it keeps the code pretty
much unchanged.

Jeremy, could you please respin this patch and give it a try?

Thanks.

-- 
Catalin
Ard Biesheuvel Oct. 6, 2020, 12:36 p.m. UTC | #8
On Tue, 6 Oct 2020 at 14:33, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Tue, Oct 06, 2020 at 11:43:14AM +0100, Dave P Martin wrote:
> > On Tue, Oct 06, 2020 at 11:25:11AM +0100, Catalin Marinas wrote:
> > > On Tue, Oct 06, 2020 at 11:01:21AM +0100, Dave P Martin wrote:
> > > > On Tue, Oct 06, 2020 at 09:27:48AM +0100, Will Deacon wrote:
> > > > > On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:
> > > > > > The AES code uses a 'br x7' as part of a function called by
> > > > > > a macro. That branch needs a bti_j as a target. This results
> > > > > > in a panic as seen below. Instead of trying to replace the branch
> > > > > > target with a bti_jc, lets replace the indirect branch with a
> > > > > > bl/ret, bl sequence that can target the existing bti_c.
> > > > > >
> > > > > >   Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI
> > > > > >   CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1
> > > > > >   pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)
> > > > > >   pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> > > > > >   lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]
> > > > > >   sp : ffff80001052b730
> > > > > >
> > > > > >   aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> > > > > >    __xts_crypt+0xb0/0x2dc [aes_neon_bs]
> > > > > >    xts_encrypt+0x28/0x3c [aes_neon_bs]
> > > > > >   crypto_skcipher_encrypt+0x50/0x84
> > > > > >   simd_skcipher_encrypt+0xc8/0xe0
> > > > > >   crypto_skcipher_encrypt+0x50/0x84
> > > > > >   test_skcipher_vec_cfg+0x224/0x5f0
> > > > > >   test_skcipher+0xbc/0x120
> > > > > >   alg_test_skcipher+0xa0/0x1b0
> > > > > >   alg_test+0x3dc/0x47c
> > > > > >   cryptomgr_test+0x38/0x60
> > > > > >
> > > > > > Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")
> > > > >
> > > > > nit: the "commit" string shouldn't be here, and I think the linux-next
> > > > > scripts will yell at us if we don't remove it.
> > > > >
> > > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > > > > ---
> > > > > >  arch/arm64/crypto/aes-neonbs-core.S | 6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S
> > > > > > index b357164379f6..32f53ebe5e2c 100644
> > > > > > --- a/arch/arm64/crypto/aes-neonbs-core.S
> > > > > > +++ b/arch/arm64/crypto/aes-neonbs-core.S
> > > > > > @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)
> > > > > >
> > > > > >  0:   mov             bskey, x21
> > > > > >       mov             rounds, x22
> > > > > > -     br              x7
> > > > > > +     ret
> > > >
> > > > Dang, replied on an old version.
> > >
> > > Which I ignored (by default, when the kbuild test robot complains ;)).
> > >
> > > > Since this is logically a tail call, could we simply be using br x16 or
> > > > br x17 for this?
> > > >
> > > > The architecture makes special provision for that so that the compiler
> > > > can generate tail-calls.
> > >
> > > So a "br x16" is compatible with a bti_c landing pad. I think it makes
> > > more sense to keep it as a tail call.
> >
> > Just to be clear, I'm happy either way, but I thought it would make
> > sense to point this out.
>
> I'd prefer the replacement with a br x16/17, it keeps the code pretty
> much unchanged.
>

+1

> Jeremy, could you please respin this patch and give it a try?
>
> Thanks.
>
> --
> Catalin
Jeremy Linton Oct. 6, 2020, 1:45 p.m. UTC | #9
Hi,

On 10/6/20 7:33 AM, Catalin Marinas wrote:
> On Tue, Oct 06, 2020 at 11:43:14AM +0100, Dave P Martin wrote:

>> On Tue, Oct 06, 2020 at 11:25:11AM +0100, Catalin Marinas wrote:

>>> On Tue, Oct 06, 2020 at 11:01:21AM +0100, Dave P Martin wrote:

>>>> On Tue, Oct 06, 2020 at 09:27:48AM +0100, Will Deacon wrote:

>>>>> On Mon, Oct 05, 2020 at 10:48:54PM -0500, Jeremy Linton wrote:

>>>>>> The AES code uses a 'br x7' as part of a function called by

>>>>>> a macro. That branch needs a bti_j as a target. This results

>>>>>> in a panic as seen below. Instead of trying to replace the branch

>>>>>> target with a bti_jc, lets replace the indirect branch with a

>>>>>> bl/ret, bl sequence that can target the existing bti_c.

>>>>>>

>>>>>>    Bad mode in Synchronous Abort handler detected on CPU1, code 0x34000003 -- BTI

>>>>>>    CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1

>>>>>>    pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)

>>>>>>    pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

>>>>>>    lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]

>>>>>>    sp : ffff80001052b730

>>>>>>

>>>>>>    aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]

>>>>>>     __xts_crypt+0xb0/0x2dc [aes_neon_bs]

>>>>>>     xts_encrypt+0x28/0x3c [aes_neon_bs]

>>>>>>    crypto_skcipher_encrypt+0x50/0x84

>>>>>>    simd_skcipher_encrypt+0xc8/0xe0

>>>>>>    crypto_skcipher_encrypt+0x50/0x84

>>>>>>    test_skcipher_vec_cfg+0x224/0x5f0

>>>>>>    test_skcipher+0xbc/0x120

>>>>>>    alg_test_skcipher+0xa0/0x1b0

>>>>>>    alg_test+0x3dc/0x47c

>>>>>>    cryptomgr_test+0x38/0x60

>>>>>>

>>>>>> Fixes: commit 0e89640b640d ("crypto: arm64 - Use modern annotations for assembly functions")

>>>>>

>>>>> nit: the "commit" string shouldn't be here, and I think the linux-next

>>>>> scripts will yell at us if we don't remove it.

>>>>>

>>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

>>>>>> ---

>>>>>>   arch/arm64/crypto/aes-neonbs-core.S | 6 +++---

>>>>>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>>>>>

>>>>>> diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S

>>>>>> index b357164379f6..32f53ebe5e2c 100644

>>>>>> --- a/arch/arm64/crypto/aes-neonbs-core.S

>>>>>> +++ b/arch/arm64/crypto/aes-neonbs-core.S

>>>>>> @@ -788,7 +788,7 @@ SYM_FUNC_START_LOCAL(__xts_crypt8)

>>>>>>   

>>>>>>   0:	mov		bskey, x21

>>>>>>   	mov		rounds, x22

>>>>>> -	br		x7

>>>>>> +	ret

>>>>

>>>> Dang, replied on an old version.

>>>

>>> Which I ignored (by default, when the kbuild test robot complains ;)).

>>>

>>>> Since this is logically a tail call, could we simply be using br x16 or

>>>> br x17 for this?

>>>>

>>>> The architecture makes special provision for that so that the compiler

>>>> can generate tail-calls.

>>>

>>> So a "br x16" is compatible with a bti_c landing pad. I think it makes

>>> more sense to keep it as a tail call.

>>

>> Just to be clear, I'm happy either way, but I thought it would make

>> sense to point this out.

> 

> I'd prefer the replacement with a br x16/17, it keeps the code pretty

> much unchanged.

> 

> Jeremy, could you please respin this patch and give it a try?


Sounds like a plan.

I'm probably going to change the subject again, guess I will put a v3 on 
it too. :)
diff mbox series

Patch

diff --git a/arch/arm64/crypto/aes-neonbs-core.S b/arch/arm64/crypto/aes-neonbs-core.S
index b357164379f6..32f53ebe5e2c 100644
--- a/arch/arm64/crypto/aes-neonbs-core.S
+++ b/arch/arm64/crypto/aes-neonbs-core.S
@@ -788,7 +788,7 @@  SYM_FUNC_START_LOCAL(__xts_crypt8)
 
 0:	mov		bskey, x21
 	mov		rounds, x22
-	br		x7
+	ret
 SYM_FUNC_END(__xts_crypt8)
 
 	.macro		__xts_crypt, do8, o0, o1, o2, o3, o4, o5, o6, o7
@@ -806,8 +806,8 @@  SYM_FUNC_END(__xts_crypt8)
 	uzp1		v30.4s, v30.4s, v25.4s
 	ld1		{v25.16b}, [x24]
 
-99:	adr		x7, \do8
-	bl		__xts_crypt8
+99:	bl		__xts_crypt8
+	bl 		\do8
 
 	ldp		q16, q17, [sp, #.Lframe_local_offset]
 	ldp		q18, q19, [sp, #.Lframe_local_offset + 32]