[Xen-devel] [PATCH v2] x86/stackframe/32: repair 32-bit Xen PV

[Xen-devel] [PATCH v2] x86/stackframe/32: repair 32-bit Xen PV

[Jan Beulich]

Once again RPL checks have been introduced which don't account for a
32-bit kernel living in ring 1 when running in a PV Xen domain. The
case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3
as well just in case.

Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Avoid #ifdef and alter comment along the lines of Andy's suggestion.

--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -48,6 +48,13 @@
 
 #include "calling.h"
 
+/*
+ * When running on Xen PV, the actual %cs register's RPL in the kernel is 1,
+ * not 0. If we need to distinguish between a %cs from kernel mode and a %cs
+ * from user mode, we can do test $2 instead of test $3.
+ */
+#define USER_SEGMENT_RPL_MASK 2
+
 	.section .entry.text, "ax"
 
 /*
@@ -172,7 +179,7 @@
 	ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
 	.if \no_user_check == 0
 	/* coming from usermode? */
-	testl	$SEGMENT_RPL_MASK, PT_CS(%esp)
+	testl	$USER_SEGMENT_RPL_MASK, PT_CS(%esp)
 	jz	.Lend_\@
 	.endif
 	/* On user-cr3? */
@@ -217,7 +224,7 @@
 	testl	$X86_EFLAGS_VM, 4*4(%esp)
 	jnz	.Lfrom_usermode_no_fixup_\@
 #endif
-	testl	$SEGMENT_RPL_MASK, 3*4(%esp)
+	testl	$USER_SEGMENT_RPL_MASK, 3*4(%esp)
 	jnz	.Lfrom_usermode_no_fixup_\@
 
 	orl	$CS_FROM_KERNEL, 3*4(%esp)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] x86/stackframe/32: repair 32-bit Xen PV

[Thomas Gleixner]

On Tue, 29 Oct 2019, Jan Beulich wrote:

> Once again RPL checks have been introduced which don't account for a
> 32-bit kernel living in ring 1 when running in a PV Xen domain.
>
> The case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3
> as well just in case.

Either it's required and correct or it's not. Just in case is not helpful
at all.

> Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -48,6 +48,13 @@
>  
>  #include "calling.h"
>  
> +/*
> + * When running on Xen PV, the actual %cs register's RPL in the kernel is 1,
> + * not 0. If we need to distinguish between a %cs from kernel mode and a %cs
> + * from user mode, we can do test $2 instead of test $3.
> + */
> +#define USER_SEGMENT_RPL_MASK 2

No. The define want's to be right next to the SEGMENT_RPL_MASK define
including a less ASM focussed comment like this:

/*
 * When running on Xen PV, the actual priviledge level of the kernel is 1,
 * not 0. Testing the Requested Priviledge Level in a segment selector to
 * determine whether the context is user mode or kernel mode with
 * SEGMENT_RPL_MASK is wrong because the PV kernels priviledge level
 * matches the 0x03 mask.
 *
 * Testing with USER_SEGMENT_RPL_MASK is valid for both native and Xen PV
 * kernels because Priviledge Level 2 is never used.
 */

Hmm?

Thanks,

	tglx

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] x86/stackframe/32: repair 32-bit Xen PV

[Jan Beulich]

On 04.11.2019 23:44, Thomas Gleixner wrote:
> On Tue, 29 Oct 2019, Jan Beulich wrote:
> 
>> Once again RPL checks have been introduced which don't account for a
>> 32-bit kernel living in ring 1 when running in a PV Xen domain.
>>
>> The case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3
>> as well just in case.
> 
> Either it's required and correct or it's not. Just in case is not helpful
> at all.

_If_ this macro sits on any path reachable when running PV under
Xen, then it's wrong. If any such use gets added down the road,
then it's latently wrong, which is bad enough imo, and hence
warrants the change even without analyzing whether there's
already an affected path.

>> Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
>> --- a/arch/x86/entry/entry_32.S
>> +++ b/arch/x86/entry/entry_32.S
>> @@ -48,6 +48,13 @@
>>  
>>  #include "calling.h"
>>  
>> +/*
>> + * When running on Xen PV, the actual %cs register's RPL in the kernel is 1,
>> + * not 0. If we need to distinguish between a %cs from kernel mode and a %cs
>> + * from user mode, we can do test $2 instead of test $3.
>> + */
>> +#define USER_SEGMENT_RPL_MASK 2
> 
> No. The define want's to be right next to the SEGMENT_RPL_MASK define
> including a less ASM focussed comment like this:
> 
> /*
>  * When running on Xen PV, the actual priviledge level of the kernel is 1,
>  * not 0. Testing the Requested Priviledge Level in a segment selector to
>  * determine whether the context is user mode or kernel mode with
>  * SEGMENT_RPL_MASK is wrong because the PV kernels priviledge level
>  * matches the 0x03 mask.
>  *
>  * Testing with USER_SEGMENT_RPL_MASK is valid for both native and Xen PV
>  * kernels because Priviledge Level 2 is never used.
>  */
> 
> Hmm?

I simply used almost exactly what Andy had suggested as a comment. He
also didn't object to the definition sitting here (it's not needed
after all outside of this file). Can the two of you please reach
agreement, for me to act upon?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] Ping: [PATCH v2] x86/stackframe/32: repair 32-bit Xen PV

[Jan Beulich]

Andy,

On 29.10.2019 10:28, Jan Beulich wrote:
> Once again RPL checks have been introduced which don't account for a
> 32-bit kernel living in ring 1 when running in a PV Xen domain. The
> case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3
> as well just in case.
> 
> Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

would you mind clarifying whether I should follow Thomas' request,
overriding what you had asked for an I did carry out for v2? I don't
think this regression should be left unfixed for much longer (as
much as the other part of it, addressed by a later 2-patch series).

Thanks, Jan

> ---
> v2: Avoid #ifdef and alter comment along the lines of Andy's suggestion.
> 
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -48,6 +48,13 @@
>  
>  #include "calling.h"
>  
> +/*
> + * When running on Xen PV, the actual %cs register's RPL in the kernel is 1,
> + * not 0. If we need to distinguish between a %cs from kernel mode and a %cs
> + * from user mode, we can do test $2 instead of test $3.
> + */
> +#define USER_SEGMENT_RPL_MASK 2
> +
>  	.section .entry.text, "ax"
>  
>  /*
> @@ -172,7 +179,7 @@
>  	ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
>  	.if \no_user_check == 0
>  	/* coming from usermode? */
> -	testl	$SEGMENT_RPL_MASK, PT_CS(%esp)
> +	testl	$USER_SEGMENT_RPL_MASK, PT_CS(%esp)
>  	jz	.Lend_\@
>  	.endif
>  	/* On user-cr3? */
> @@ -217,7 +224,7 @@
>  	testl	$X86_EFLAGS_VM, 4*4(%esp)
>  	jnz	.Lfrom_usermode_no_fixup_\@
>  #endif
> -	testl	$SEGMENT_RPL_MASK, 3*4(%esp)
> +	testl	$USER_SEGMENT_RPL_MASK, 3*4(%esp)
>  	jnz	.Lfrom_usermode_no_fixup_\@
>  
>  	orl	$CS_FROM_KERNEL, 3*4(%esp)
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Ping: [PATCH v2] x86/stackframe/32: repair 32-bit Xen PV

[Andy Lutomirski]

On Fri, Nov 15, 2019 at 6:30 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Andy,
>
> On 29.10.2019 10:28, Jan Beulich wrote:
> > Once again RPL checks have been introduced which don't account for a
> > 32-bit kernel living in ring 1 when running in a PV Xen domain. The
> > case in FIXUP_FRAME has been preventing boot; adjust BUG_IF_WRONG_CR3
> > as well just in case.
> >
> > Fixes: 3c88c692c287 ("x86/stackframe/32: Provide consistent pt_regs")
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> would you mind clarifying whether I should follow Thomas' request,
> overriding what you had asked for an I did carry out for v2? I don't
> think this regression should be left unfixed for much longer (as
> much as the other part of it, addressed by a later 2-patch series).
>

I'm fine with doing it Thomas' way.

--Andy

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel