* xen/arm: potential bug in advance_pc
@ 2019-05-14 16:03 Lukas Jünger
2019-05-14 16:03 ` [Xen-devel] " Lukas Jünger
2019-05-15 11:35 ` Julien Grall
0 siblings, 2 replies; 4+ messages in thread
From: Lukas Jünger @ 2019-05-14 16:03 UTC (permalink / raw)
To: xen-devel; +Cc: Lukas Juenger
[-- Attachment #1.1: Type: text/plain, Size: 1062 bytes --]
Hello all,
in the function advance_pc in xen/arch/arm/traps.c in line 1655,1656 you
can find the following code:
1655 BUG_ON( (!psr_mode_is_32bit(cpsr)||!(cpsr&PSR_THUMB))
1656 && (cpsr&PSR_IT_MASK) );
This code seems to check that we are not running in thumb mode and that
the PSR_IT_MASK is not set.
On ARMv8.5-BTI systems bits [11:10] of spsr_el2 indicate the BTYPE (see
https://developer.arm.com/docs/ddi0595/b/aarch64-system-registers/spsr_el2).
If an exception is taken in the guest (e.g. write to system register)
from AArch64 state these bits might be set.
The PSR_IT_MASK for thumb mode overlaps with these bits and BUG_ON is
executed.
This seems to be a bug.
Is it really necessary to check the PSR_IT_MASK for BUG_ON here?
Why is the execution mode checked twice with psr_mode_is_32bit and
cpsr&PSR_THUMB, as they seem to do the same thing?
If PSR_IT_MASK does not need to be checked for BUG_ON, the if statement
in the following line should check for thumb mode again, right?
Best regards,
Lukas
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5388 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Xen-devel] xen/arm: potential bug in advance_pc
2019-05-14 16:03 xen/arm: potential bug in advance_pc Lukas Jünger
@ 2019-05-14 16:03 ` Lukas Jünger
2019-05-15 11:35 ` Julien Grall
1 sibling, 0 replies; 4+ messages in thread
From: Lukas Jünger @ 2019-05-14 16:03 UTC (permalink / raw)
To: xen-devel; +Cc: Lukas Juenger
[-- Attachment #1.1: Type: text/plain, Size: 1062 bytes --]
Hello all,
in the function advance_pc in xen/arch/arm/traps.c in line 1655,1656 you
can find the following code:
1655 BUG_ON( (!psr_mode_is_32bit(cpsr)||!(cpsr&PSR_THUMB))
1656 && (cpsr&PSR_IT_MASK) );
This code seems to check that we are not running in thumb mode and that
the PSR_IT_MASK is not set.
On ARMv8.5-BTI systems bits [11:10] of spsr_el2 indicate the BTYPE (see
https://developer.arm.com/docs/ddi0595/b/aarch64-system-registers/spsr_el2).
If an exception is taken in the guest (e.g. write to system register)
from AArch64 state these bits might be set.
The PSR_IT_MASK for thumb mode overlaps with these bits and BUG_ON is
executed.
This seems to be a bug.
Is it really necessary to check the PSR_IT_MASK for BUG_ON here?
Why is the execution mode checked twice with psr_mode_is_32bit and
cpsr&PSR_THUMB, as they seem to do the same thing?
If PSR_IT_MASK does not need to be checked for BUG_ON, the if statement
in the following line should check for thumb mode again, right?
Best regards,
Lukas
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5388 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: xen/arm: potential bug in advance_pc
2019-05-14 16:03 xen/arm: potential bug in advance_pc Lukas Jünger
2019-05-14 16:03 ` [Xen-devel] " Lukas Jünger
@ 2019-05-15 11:35 ` Julien Grall
2019-05-15 11:35 ` [Xen-devel] " Julien Grall
1 sibling, 1 reply; 4+ messages in thread
From: Julien Grall @ 2019-05-15 11:35 UTC (permalink / raw)
To: Lukas Jünger, xen-devel; +Cc: Stefano Stabellini, Lukas Juenger
On 14/05/2019 17:03, Lukas Jünger wrote:
> Hello all,
Hello,
Thank you for the report.
Please try to CC the maintainers of the code (in this case Stefano and I) when
asking a question. So your e-mail don't get lost in the noise of xen-devel.
> in the function advance_pc in xen/arch/arm/traps.c in line 1655,1656 you can
> find the following code:
>
> 1655 BUG_ON( (!psr_mode_is_32bit(cpsr)||!(cpsr&PSR_THUMB))
> 1656 && (cpsr&PSR_IT_MASK) );
>
> This code seems to check that we are not running in thumb mode and that the
> PSR_IT_MASK is not set.
> On ARMv8.5-BTI systems bits [11:10] of spsr_el2 indicate the BTYPE (see
> https://developer.arm.com/docs/ddi0595/b/aarch64-system-registers/spsr_el2).
> If an exception is taken in the guest (e.g. write to system register) from
> AArch64 state these bits might be set.
> The PSR_IT_MASK for thumb mode overlaps with these bits and BUG_ON is executed.
> This seems to be a bug.
Xen code blindly assumed that RES0 means the bits cannot be used for other
purpose in later revision of the architecture. So this is clearly a bug.
> Is it really necessary to check the PSR_IT_MASK for BUG_ON here?
No, and it is a pretty bad idea to check guest architectural behavior with
BUG_ON(). The BUG_ON() needs to be replaced with proper check on the CPSR (see
below).
> Why is the execution mode checked twice with psr_mode_is_32bit and
> cpsr&PSR_THUMB, as they seem to do the same thing?
32-bit mode support two instruction set: ARM and Thumb. From my understanding
the value of IT is unknown when using the ARM instruction set. Furthermore, the
bit 5 only means Thumb on 32-bit mode. For 64-bit mode, the bit is RES0.
So we would need to check the 2 bits.
> If PSR_IT_MASK does not need to be checked for BUG_ON, the if statement in the
> following line should check for thumb mode again, right?
The check should be something like:
if ( psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB) && (cpsr & PSR_IT_MASK) )
{
}
I have noticed few more issues in the traps.c code regarding RES0 . I will send
a series with that fixed later today.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Xen-devel] xen/arm: potential bug in advance_pc
2019-05-15 11:35 ` Julien Grall
@ 2019-05-15 11:35 ` Julien Grall
0 siblings, 0 replies; 4+ messages in thread
From: Julien Grall @ 2019-05-15 11:35 UTC (permalink / raw)
To: Lukas Jünger, xen-devel; +Cc: Stefano Stabellini, Lukas Juenger
On 14/05/2019 17:03, Lukas Jünger wrote:
> Hello all,
Hello,
Thank you for the report.
Please try to CC the maintainers of the code (in this case Stefano and I) when
asking a question. So your e-mail don't get lost in the noise of xen-devel.
> in the function advance_pc in xen/arch/arm/traps.c in line 1655,1656 you can
> find the following code:
>
> 1655 BUG_ON( (!psr_mode_is_32bit(cpsr)||!(cpsr&PSR_THUMB))
> 1656 && (cpsr&PSR_IT_MASK) );
>
> This code seems to check that we are not running in thumb mode and that the
> PSR_IT_MASK is not set.
> On ARMv8.5-BTI systems bits [11:10] of spsr_el2 indicate the BTYPE (see
> https://developer.arm.com/docs/ddi0595/b/aarch64-system-registers/spsr_el2).
> If an exception is taken in the guest (e.g. write to system register) from
> AArch64 state these bits might be set.
> The PSR_IT_MASK for thumb mode overlaps with these bits and BUG_ON is executed.
> This seems to be a bug.
Xen code blindly assumed that RES0 means the bits cannot be used for other
purpose in later revision of the architecture. So this is clearly a bug.
> Is it really necessary to check the PSR_IT_MASK for BUG_ON here?
No, and it is a pretty bad idea to check guest architectural behavior with
BUG_ON(). The BUG_ON() needs to be replaced with proper check on the CPSR (see
below).
> Why is the execution mode checked twice with psr_mode_is_32bit and
> cpsr&PSR_THUMB, as they seem to do the same thing?
32-bit mode support two instruction set: ARM and Thumb. From my understanding
the value of IT is unknown when using the ARM instruction set. Furthermore, the
bit 5 only means Thumb on 32-bit mode. For 64-bit mode, the bit is RES0.
So we would need to check the 2 bits.
> If PSR_IT_MASK does not need to be checked for BUG_ON, the if statement in the
> following line should check for thumb mode again, right?
The check should be something like:
if ( psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB) && (cpsr & PSR_IT_MASK) )
{
}
I have noticed few more issues in the traps.c code regarding RES0 . I will send
a series with that fixed later today.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-15 11:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14 16:03 xen/arm: potential bug in advance_pc Lukas Jünger
2019-05-14 16:03 ` [Xen-devel] " Lukas Jünger
2019-05-15 11:35 ` Julien Grall
2019-05-15 11:35 ` [Xen-devel] " Julien Grall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).