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 <[email protected]>
---
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
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 <[email protected]>
David
>>> On 09.04.14 at 16:06, <[email protected]> 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
On 09/04/14 15:21, Jan Beulich wrote:
>>>> On 09.04.14 at 16:06, <[email protected]> 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
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, <[email protected]> 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
>>> On 09.04.14 at 16:41, <[email protected]> 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
On Wed, 09 Apr 2014 11:12:33 -0400
Boris Ostrovsky <[email protected]> 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 <[email protected]>
> ---
> 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
On 04/09/2014 11:01 AM, Jan Beulich wrote:
>>>> On 09.04.14 at 16:41, <[email protected]> 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
> [email protected]
> http://lists.xen.org/xen-devel
>>> On 09.04.14 at 17:38, <[email protected]> wrote:
> On 04/09/2014 11:01 AM, Jan Beulich wrote:
>>>>> On 09.04.14 at 16:41, <[email protected]> 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
On 04/09/2014 11:25 AM, Steven Rostedt wrote:
> On Wed, 09 Apr 2014 11:12:33 -0400
> Boris Ostrovsky <[email protected]> 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 <[email protected]>
>> ---
>> 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
>