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

[PATCH] 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>

--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -48,6 +48,17 @@
 
 #include "calling.h"
 
+#ifndef CONFIG_XEN_PV
+# define USER_SEGMENT_RPL_MASK SEGMENT_RPL_MASK
+#else
+/*
+ * When running paravirtualized on Xen the kernel runs in ring 1, and hence
+ * simple mask based tests (i.e. ones not comparing against USER_RPL) have to
+ * ignore bit 0. See also the C-level get_kernel_rpl().
+ */
+# define USER_SEGMENT_RPL_MASK (SEGMENT_RPL_MASK & ~1)
+#endif
+
 	.section .entry.text, "ax"
 
 /*
@@ -172,7 +183,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 +228,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)

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

[Jan Beulich]

On 07.10.2019 12:41, 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>

Ping?

I'd like to further note that there appears to a likely related
2nd problem - I'm seeing seemingly random attempts to enter VM86
mode when running PV under Xen. I suspect a never written eflags
value to get inspected. While the issue here kills the kernel
reliably during boot, that other issue sometimes allows the
system to at least come up in a partly functional way (depending
on which user processes get killed because of there not being
any VM86 mode available when running PV under [64-bit] Xen).

Jan

> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -48,6 +48,17 @@
>  
>  #include "calling.h"
>  
> +#ifndef CONFIG_XEN_PV
> +# define USER_SEGMENT_RPL_MASK SEGMENT_RPL_MASK
> +#else
> +/*
> + * When running paravirtualized on Xen the kernel runs in ring 1, and hence
> + * simple mask based tests (i.e. ones not comparing against USER_RPL) have to
> + * ignore bit 0. See also the C-level get_kernel_rpl().
> + */
> +# define USER_SEGMENT_RPL_MASK (SEGMENT_RPL_MASK & ~1)
> +#endif
> +
>  	.section .entry.text, "ax"
>  
>  /*
> @@ -172,7 +183,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 +228,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)
> 

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

[Andy Lutomirski]

On Mon, Oct 7, 2019 at 3:41 AM Jan Beulich <jbeulich@suse.com> 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.

I'm okay with the generated code, but IMO the macro is too indirect
for something that's trivial.

>
> 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,17 @@
>
>  #include "calling.h"
>
> +#ifndef CONFIG_XEN_PV
> +# define USER_SEGMENT_RPL_MASK SEGMENT_RPL_MASK
> +#else
> +/*
> + * When running paravirtualized on Xen the kernel runs in ring 1, and hence
> + * simple mask based tests (i.e. ones not comparing against USER_RPL) have to
> + * ignore bit 0. See also the C-level get_kernel_rpl().
> + */

How about:

/*
 * When running on Xen PV, the actual %cs register 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

but...

> +# define USER_SEGMENT_RPL_MASK (SEGMENT_RPL_MASK & ~1)
> +#endif
> +
>         .section .entry.text, "ax"
>
>  /*
> @@ -172,7 +183,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)

Shouldn't PT_CS(%esp) be 0 if we came from the kernel?  I'm guessing
the actual bug is in whatever code put 1 in here in the first place.

In other words, I'm having trouble understanding why there is any
context in which some value would be 3 for user mode and 1 for kernel
mode.  Obviously if we're manually IRETing to kernel mode, we need to
set CS to 1, but if we're filling in our own PT_CS, we should just
write 0.

The supposedly offending commit (""x86/stackframe/32: Provide
consistent pt_regs") looks correct to me, so I suspect that the
problem is elsewhere.  Or is it intentional that Xen PV's asm
(arch/x86/xen/whatever) sticks 1 into the CS field on the stack?

Also, why are we supporting 32-bit Linux PV guests at all?  Can we
just delete this code instead?

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

[Andrew Cooper]

On 24/10/2019 17:11, Andy Lutomirski wrote:
>> +# define USER_SEGMENT_RPL_MASK (SEGMENT_RPL_MASK & ~1)
>> +#endif
>> +
>>         .section .entry.text, "ax"
>>
>>  /*
>> @@ -172,7 +183,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)
> Shouldn't PT_CS(%esp) be 0 if we came from the kernel?  I'm guessing
> the actual bug is in whatever code put 1 in here in the first place.

Ring1 kernels (32bit) consistently see RPL1 everywhere under Xen.

Back in the days of a 32bit Xen, int $0x80 really was wired directly
from ring 3 to 1, and didn't bounce through Xen.  This isn't possible in
long mode, because all IDT gates are required to be 64bit code segments.

Ring3 kernels (64bit) consistently see RPL0 everywhere under Xen,
because presumably this was less invasive when designing the ABI.

~Andrew

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

[Andy Lutomirski]

On Thu, Oct 24, 2019 at 9:32 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 24/10/2019 17:11, Andy Lutomirski wrote:
> >> +# define USER_SEGMENT_RPL_MASK (SEGMENT_RPL_MASK & ~1)
> >> +#endif
> >> +
> >>         .section .entry.text, "ax"
> >>
> >>  /*
> >> @@ -172,7 +183,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)
> > Shouldn't PT_CS(%esp) be 0 if we came from the kernel?  I'm guessing
> > the actual bug is in whatever code put 1 in here in the first place.
>
> Ring1 kernels (32bit) consistently see RPL1 everywhere under Xen.
>
> Back in the days of a 32bit Xen, int $0x80 really was wired directly
> from ring 3 to 1, and didn't bounce through Xen.  This isn't possible in
> long mode, because all IDT gates are required to be 64bit code segments.
>
> Ring3 kernels (64bit) consistently see RPL0 everywhere under Xen,
> because presumably this was less invasive when designing the ABI.
>

OK, gotcha.

So I'm fine with this patch if you improve the comment and definition.

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

[Jan Beulich]

On 24.10.2019 18:11, Andy Lutomirski wrote:
> On Mon, Oct 7, 2019 at 3:41 AM Jan Beulich <jbeulich@suse.com> 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.
> 
> I'm okay with the generated code, but IMO the macro is too indirect
> for something that's trivial.
> 
>>
>> 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,17 @@
>>
>>   #include "calling.h"
>>
>> +#ifndef CONFIG_XEN_PV
>> +# define USER_SEGMENT_RPL_MASK SEGMENT_RPL_MASK
>> +#else
>> +/*
>> + * When running paravirtualized on Xen the kernel runs in ring 1, and hence
>> + * simple mask based tests (i.e. ones not comparing against USER_RPL) have to
>> + * ignore bit 0. See also the C-level get_kernel_rpl().
>> + */
> 
> How about:
> 
> /*
>   * When running on Xen PV, the actual %cs register 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

I.e. you're fine using just the single bit in all configurations?

>> +# define USER_SEGMENT_RPL_MASK (SEGMENT_RPL_MASK & ~1)
>> +#endif
>> +
>>          .section .entry.text, "ax"
>>
>>   /*
>> @@ -172,7 +183,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)
> 
> Shouldn't PT_CS(%esp) be 0 if we came from the kernel?  I'm guessing
> the actual bug is in whatever code put 1 in here in the first place.
> 
> In other words, I'm having trouble understanding why there is any
> context in which some value would be 3 for user mode and 1 for kernel
> mode.  Obviously if we're manually IRETing to kernel mode, we need to
> set CS to 1, but if we're filling in our own PT_CS, we should just
> write 0.
> 
> The supposedly offending commit (""x86/stackframe/32: Provide
> consistent pt_regs") looks correct to me, so I suspect that the
> problem is elsewhere.  Or is it intentional that Xen PV's asm
> (arch/x86/xen/whatever) sticks 1 into the CS field on the stack?

Manually created / updated frames _could_ in principle modify the
RPL, but ones coming from hardware (old 32-bit hypervisors) or Xen
(64-bit hypervisors) will have an RPL of 1, as already said by
Andrew. We could in principle also add a VM assist for the
hypervisor to store an RPL of 0, but I'd expect this to require
further kernel changes, and together with the old behavior still
being required to support I'm unconvinced this would be worth it.

> Also, why are we supporting 32-bit Linux PV guests at all?  Can we
> just delete this code instead?

This was already suggested by Jürgen (now also CC-ed), but in reply
it was pointed out that the process would be to first deprecate the
code, and remove it only a couple of releases later if no-one comes
up with a reason to retain it.

Jan

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

[Jürgen Groß]

On 25.10.19 08:06, Jan Beulich wrote:
> On 24.10.2019 18:11, Andy Lutomirski wrote:
>> On Mon, Oct 7, 2019 at 3:41 AM Jan Beulich <jbeulich@suse.com> 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.
>>
>> I'm okay with the generated code, but IMO the macro is too indirect
>> for something that's trivial.
>>
>>>
>>> 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,17 @@
>>>
>>>    #include "calling.h"
>>>
>>> +#ifndef CONFIG_XEN_PV
>>> +# define USER_SEGMENT_RPL_MASK SEGMENT_RPL_MASK
>>> +#else
>>> +/*
>>> + * When running paravirtualized on Xen the kernel runs in ring 1, and hence
>>> + * simple mask based tests (i.e. ones not comparing against USER_RPL) have to
>>> + * ignore bit 0. See also the C-level get_kernel_rpl().
>>> + */
>>
>> How about:
>>
>> /*
>>    * When running on Xen PV, the actual %cs register 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
> 
> I.e. you're fine using just the single bit in all configurations?
> 
>>> +# define USER_SEGMENT_RPL_MASK (SEGMENT_RPL_MASK & ~1)
>>> +#endif
>>> +
>>>           .section .entry.text, "ax"
>>>
>>>    /*
>>> @@ -172,7 +183,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)
>>
>> Shouldn't PT_CS(%esp) be 0 if we came from the kernel?  I'm guessing
>> the actual bug is in whatever code put 1 in here in the first place.
>>
>> In other words, I'm having trouble understanding why there is any
>> context in which some value would be 3 for user mode and 1 for kernel
>> mode.  Obviously if we're manually IRETing to kernel mode, we need to
>> set CS to 1, but if we're filling in our own PT_CS, we should just
>> write 0.
>>
>> The supposedly offending commit (""x86/stackframe/32: Provide
>> consistent pt_regs") looks correct to me, so I suspect that the
>> problem is elsewhere.  Or is it intentional that Xen PV's asm
>> (arch/x86/xen/whatever) sticks 1 into the CS field on the stack?
> 
> Manually created / updated frames _could_ in principle modify the
> RPL, but ones coming from hardware (old 32-bit hypervisors) or Xen
> (64-bit hypervisors) will have an RPL of 1, as already said by
> Andrew. We could in principle also add a VM assist for the
> hypervisor to store an RPL of 0, but I'd expect this to require
> further kernel changes, and together with the old behavior still
> being required to support I'm unconvinced this would be worth it.
> 
>> Also, why are we supporting 32-bit Linux PV guests at all?  Can we
>> just delete this code instead?
> 
> This was already suggested by Jürgen (now also CC-ed), but in reply
> it was pointed out that the process would be to first deprecate the
> code, and remove it only a couple of releases later if no-one comes
> up with a reason to retain it.

Thanks for the reminder.

I'll send a patch with the deprecation warning for 32-bit PV.


Juergen