[PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Boris Ostrovsky]

Commit 198d208df4371734ac4728f69cb585c284d20a15 made 32-bit kernels use
kernel_stack to point to thread_info. That change missed a couple of
updates needed by Xen's PV guests:

1. kernel_stack needs to be initialized for secondary CPUs
2. GET_THREAD_INFO() now uses %fs register which may not be the kernel's
version when executing xen_iret()

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/xen/smp.c        |    3 ++-
 arch/x86/xen/xen-asm_32.S |    4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index a18eadd..7005974 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -441,10 +441,11 @@ static int xen_cpu_up(unsigned int cpu, struct task_struct *idle)
 	irq_ctx_init(cpu);
 #else
 	clear_tsk_thread_flag(idle, TIF_FORK);
+#endif
 	per_cpu(kernel_stack, cpu) =
 		(unsigned long)task_stack_page(idle) -
 		KERNEL_STACK_OFFSET + THREAD_SIZE;
-#endif
+
 	xen_setup_runstate_info(cpu);
 	xen_setup_timer(cpu);
 	xen_init_lock_cpu(cpu);
diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
index 33ca6e4..d433637 100644
--- a/arch/x86/xen/xen-asm_32.S
+++ b/arch/x86/xen/xen-asm_32.S
@@ -88,7 +88,11 @@ ENTRY(xen_iret)
 	 * avoid having to reload %fs
 	 */
 #ifdef CONFIG_SMP
+	pushw %fs
+	movl $(__KERNEL_PERCPU), %eax
+	movl %eax, %fs
 	GET_THREAD_INFO(%eax)
+	popw %fs
 	movl %ss:TI_cpu(%eax), %eax
 	movl %ss:__per_cpu_offset(,%eax,4), %eax
 	mov %ss:xen_vcpu(%eax), %eax
-- 
1.7.10.4

Re: [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[David Vrabel]

On 09/04/14 15:06, Boris Ostrovsky wrote:
> Commit 198d208df4371734ac4728f69cb585c284d20a15 made 32-bit kernels use
> kernel_stack to point to thread_info. That change missed a couple of
> updates needed by Xen's PV guests:

Can you put the commit subject in addition to the hash?

> 1. kernel_stack needs to be initialized for secondary CPUs
> 2. GET_THREAD_INFO() now uses %fs register which may not be the kernel's
> version when executing xen_iret()

Otherwise,

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [Xen-devel] [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Jan Beulich]

>>> On 09.04.14 at 16:06, <boris.ostrovsky@oracle.com> wrote:
> --- a/arch/x86/xen/xen-asm_32.S
> +++ b/arch/x86/xen/xen-asm_32.S
> @@ -88,7 +88,11 @@ ENTRY(xen_iret)
>  	 * avoid having to reload %fs
>  	 */
>  #ifdef CONFIG_SMP
> +	pushw %fs
> +	movl $(__KERNEL_PERCPU), %eax
> +	movl %eax, %fs
>  	GET_THREAD_INFO(%eax)
> +	popw %fs

I don't think it's guaranteed that this can't fault.

Jan

Re: [Xen-devel] [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[David Vrabel]

On 09/04/14 15:21, Jan Beulich wrote:
>>>> On 09.04.14 at 16:06, <boris.ostrovsky@oracle.com> wrote:
>> --- a/arch/x86/xen/xen-asm_32.S
>> +++ b/arch/x86/xen/xen-asm_32.S
>> @@ -88,7 +88,11 @@ ENTRY(xen_iret)
>>  	 * avoid having to reload %fs
>>  	 */
>>  #ifdef CONFIG_SMP
>> +	pushw %fs
>> +	movl $(__KERNEL_PERCPU), %eax
>> +	movl %eax, %fs
>>  	GET_THREAD_INFO(%eax)
>> +	popw %fs
> 
> I don't think it's guaranteed that this can't fault.

If loading %fs faults when it is restored previously, the fixup zeros
the value.  However, this later load could still fault even if the first
succeeded.

Suggest copying the fixup section from the RESTORE_REGS macros in
arch/x86/kernel/entry_32.S

David

Re: [Xen-devel] [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Andrew Cooper]

On 09/04/14 15:29, David Vrabel wrote:
> On 09/04/14 15:21, Jan Beulich wrote:
>>>>> On 09.04.14 at 16:06, <boris.ostrovsky@oracle.com> wrote:
>>> --- a/arch/x86/xen/xen-asm_32.S
>>> +++ b/arch/x86/xen/xen-asm_32.S
>>> @@ -88,7 +88,11 @@ ENTRY(xen_iret)
>>>  	 * avoid having to reload %fs
>>>  	 */
>>>  #ifdef CONFIG_SMP
>>> +	pushw %fs
>>> +	movl $(__KERNEL_PERCPU), %eax
>>> +	movl %eax, %fs
>>>  	GET_THREAD_INFO(%eax)
>>> +	popw %fs
>> I don't think it's guaranteed that this can't fault.
> If loading %fs faults when it is restored previously, the fixup zeros
> the value.  However, this later load could still fault even if the first
> succeeded.
>
> Suggest copying the fixup section from the RESTORE_REGS macros in
> arch/x86/kernel/entry_32.S
>
> David

If loading __KERNEL_PERCPU info fs faults, the kernel has bigger
problems to worry about.

The latter load however can easy fault; The arguments for %ds in 
XSA-42/ CVE-2013-0228 applies to %{e,f,g}s as well.

Furthermore, I am a little concerned about the performance impact of
this.  I would have thought that in most cases, %fs will already be
correct, at which point reloading it twice is a waste of time.

~Andrew

Re: [Xen-devel] [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Jan Beulich]

>>> On 09.04.14 at 16:41, <andrew.cooper3@citrix.com> wrote:
> The latter load however can easy fault; The arguments for %ds in 
> XSA-42/ CVE-2013-0228 applies to %{e,f,g}s as well.

And it was only that latter operation that I pointed at.

> Furthermore, I am a little concerned about the performance impact of
> this.  I would have thought that in most cases, %fs will already be
> correct, at which point reloading it twice is a waste of time.

Why would you expect %fs on the IRET path to commonly point to the
kernel segment rather than whatever user mode wants/needs? Also, I'm
not sure adding conditionals here wouldn't harm performance about as
much as the save/load/restore. If anything I'd look into open coding
GET_THREAD_INFO() without using %fs for this single case.

Jan

Re: [Xen-devel] [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Boris Ostrovsky]

On 04/09/2014 11:01 AM, Jan Beulich wrote:
>>>> On 09.04.14 at 16:41, <andrew.cooper3@citrix.com> wrote:
>> The latter load however can easy fault; The arguments for %ds in
>> XSA-42/ CVE-2013-0228 applies to %{e,f,g}s as well.
> And it was only that latter operation that I pointed at.

We don't seem to reference %fs after the pop so doing the fixup (as 
David suggested) should be enough?

-boris


>
>> Furthermore, I am a little concerned about the performance impact of
>> this.  I would have thought that in most cases, %fs will already be
>> correct, at which point reloading it twice is a waste of time.
> Why would you expect %fs on the IRET path to commonly point to the
> kernel segment rather than whatever user mode wants/needs? Also, I'm
> not sure adding conditionals here wouldn't harm performance about as
> much as the save/load/restore. If anything I'd look into open coding
> GET_THREAD_INFO() without using %fs for this single case.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Jan Beulich]

>>> On 09.04.14 at 17:38, <boris.ostrovsky@oracle.com> wrote:
> On 04/09/2014 11:01 AM, Jan Beulich wrote:
>>>>> On 09.04.14 at 16:41, <andrew.cooper3@citrix.com> wrote:
>>> The latter load however can easy fault; The arguments for %ds in
>>> XSA-42/ CVE-2013-0228 applies to %{e,f,g}s as well.
>> And it was only that latter operation that I pointed at.
> 
> We don't seem to reference %fs after the pop so doing the fixup (as 
> David suggested) should be enough?

Of course.

Jan

Re: [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Steven Rostedt]

On Wed, 09 Apr 2014 11:12:33 -0400
Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> Commit 198d208df4371734ac4728f69cb585c284d20a15 made 32-bit kernels use
> kernel_stack to point to thread_info. That change missed a couple of
> updates needed by Xen's PV guests:
> 
> 1. kernel_stack needs to be initialized for secondary CPUs
> 2. GET_THREAD_INFO() now uses %fs register which may not be the kernel's
> version when executing xen_iret()
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  arch/x86/xen/smp.c        |    3 ++-
>  arch/x86/xen/xen-asm_32.S |    4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index a18eadd..7005974 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -441,10 +441,11 @@ static int xen_cpu_up(unsigned int cpu, struct task_struct *idle)
>  	irq_ctx_init(cpu);
>  #else
>  	clear_tsk_thread_flag(idle, TIF_FORK);
> +#endif
>  	per_cpu(kernel_stack, cpu) =
>  		(unsigned long)task_stack_page(idle) -
>  		KERNEL_STACK_OFFSET + THREAD_SIZE;
> -#endif
> +
>  	xen_setup_runstate_info(cpu);
>  	xen_setup_timer(cpu);
>  	xen_init_lock_cpu(cpu);
> diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
> index 33ca6e4..d433637 100644
> --- a/arch/x86/xen/xen-asm_32.S
> +++ b/arch/x86/xen/xen-asm_32.S
> @@ -88,7 +88,11 @@ ENTRY(xen_iret)
>  	 * avoid having to reload %fs

Looks like this patch makes the above comment incorrect.

>  	 */
>  #ifdef CONFIG_SMP
> +	pushw %fs
> +	movl $(__KERNEL_PERCPU), %eax
> +	movl %eax, %fs
>  	GET_THREAD_INFO(%eax)
> +	popw %fs
>  	movl %ss:TI_cpu(%eax), %eax

Hmm, you're using GET_THREAD_INFO() to get the thread info to find the
current CPU for later use to avoid using %fs. ??

Is it possible to just push the cpu on the kernel irq stack and be able
to retrieve it manually? Looks like the only reason to use the
thread_info here is to get to the CPU index for below.

>  	movl %ss:__per_cpu_offset(,%eax,4), %eax
>  	mov %ss:xen_vcpu(%eax), %eax

-- Steve

Re: [PATCH] x86/xen: Fix 32-bit PV guests's usage of kernel_stack

[Boris Ostrovsky]

On 04/09/2014 11:25 AM, Steven Rostedt wrote:
> On Wed, 09 Apr 2014 11:12:33 -0400
> Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>
>> Commit 198d208df4371734ac4728f69cb585c284d20a15 made 32-bit kernels use
>> kernel_stack to point to thread_info. That change missed a couple of
>> updates needed by Xen's PV guests:
>>
>> 1. kernel_stack needs to be initialized for secondary CPUs
>> 2. GET_THREAD_INFO() now uses %fs register which may not be the kernel's
>> version when executing xen_iret()
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   arch/x86/xen/smp.c        |    3 ++-
>>   arch/x86/xen/xen-asm_32.S |    4 ++++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
>> index a18eadd..7005974 100644
>> --- a/arch/x86/xen/smp.c
>> +++ b/arch/x86/xen/smp.c
>> @@ -441,10 +441,11 @@ static int xen_cpu_up(unsigned int cpu, struct task_struct *idle)
>>   	irq_ctx_init(cpu);
>>   #else
>>   	clear_tsk_thread_flag(idle, TIF_FORK);
>> +#endif
>>   	per_cpu(kernel_stack, cpu) =
>>   		(unsigned long)task_stack_page(idle) -
>>   		KERNEL_STACK_OFFSET + THREAD_SIZE;
>> -#endif
>> +
>>   	xen_setup_runstate_info(cpu);
>>   	xen_setup_timer(cpu);
>>   	xen_init_lock_cpu(cpu);
>> diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
>> index 33ca6e4..d433637 100644
>> --- a/arch/x86/xen/xen-asm_32.S
>> +++ b/arch/x86/xen/xen-asm_32.S
>> @@ -88,7 +88,11 @@ ENTRY(xen_iret)
>>   	 * avoid having to reload %fs
> Looks like this patch makes the above comment incorrect.


I didn't notice this (it was full 3 lines above the change), it will 
obviously have to be updated.


>>   	 */
>>   #ifdef CONFIG_SMP
>> +	pushw %fs
>> +	movl $(__KERNEL_PERCPU), %eax
>> +	movl %eax, %fs
>>   	GET_THREAD_INFO(%eax)
>> +	popw %fs
>>   	movl %ss:TI_cpu(%eax), %eax
> Hmm, you're using GET_THREAD_INFO() to get the thread info to find the
> current CPU for later use to avoid using %fs. ??
>
> Is it possible to just push the cpu on the kernel irq stack and be able
> to retrieve it manually? Looks like the only reason to use the
> thread_info here is to get to the CPU index for below.

Or vcpu_info, which is what we are really after.

However, it looks like using %fs/GET_THREAD_INFO() should be OK as well, 
provided I deal with a potential faulting.

-boris

>
>>   	movl %ss:__per_cpu_offset(,%eax,4), %eax
>>   	mov %ss:xen_vcpu(%eax), %eax
> -- Steve
>