[PATCH] lguest: disable SYSENTER for guests

[PATCH] lguest: disable SYSENTER for guests

[Rusty Russell]

The SYSENTER instruction jumps to a pre-programmed address at
privilege level 0.  We must not allow execution of guest code at that
privilege level, so disable sysenter when we enter the guest (and
re-enable it on return).  This fixes current case where guest
userspace can crash host.

This save/restore adds 3% to guest context switch times.  (If only
there were some kind of scheduler hook or something which would tell
us when we were being preempted so we could fix this up lazily.  But
what kind of daredevil coder would propose such a thing?)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/lguest/core.c |    7 +++++++
 1 file changed, 7 insertions(+)

===================================================================
--- a/drivers/lguest/core.c
+++ b/drivers/lguest/core.c
@@ -338,6 +338,10 @@ int run_guest(struct lguest *lg, unsigne
 		if (lg->ts)
 			set_ts();
 
+		/* Don't let Guest do SYSENTER: we can't handle it. */
+		if (boot_cpu_has(X86_FEATURE_SEP))
+			wrmsr(MSR_IA32_SYSENTER_CS, 0, 0);
+
 		run_guest_once(lg, lguest_pages(raw_smp_processor_id()));
 
 		/* Save cr2 now if we page-faulted. */
@@ -345,6 +349,9 @@ int run_guest(struct lguest *lg, unsigne
 			cr2 = read_cr2();
 		else if (lg->regs->trapnum == 7)
 			math_state_restore();
+
+		if (boot_cpu_has(X86_FEATURE_SEP))
+			wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
 		local_irq_enable();
 
 		switch (lg->regs->trapnum) {

Re: [PATCH] lguest: disable SYSENTER for guests

[Avi Kivity]

Rusty Russell wrote:
> The SYSENTER instruction jumps to a pre-programmed address at
> privilege level 0.  We must not allow execution of guest code at that
> privilege level, so disable sysenter when we enter the guest (and
> re-enable it on return).  This fixes current case where guest
> userspace can crash host.
>
> This save/restore adds 3% to guest context switch times.  (If only
> there were some kind of scheduler hook or something which would tell
> us when we were being preempted so we could fix this up lazily.  But
> what kind of daredevil coder would propose such a thing?)
>
>   

Ah, so this is why you want ->next in preempt hooks.  Well, my plan for
this sort of thing (for kvm has the same issues with the *STAR family of
msrs) is to add a new hook on switching from kernel to userspace, and
swap those msrs there.  This allows not only the guest1->guest2 case to
be optimized, but also guest->kthread->guest, which is a common pattern
with I/O (and very common with -rt, which runs interrupts in threads).

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH] lguest: disable SYSENTER for guests

[Rusty Russell]

On Thu, 2007-07-12 at 10:47 +0300, Avi Kivity wrote:
> Rusty Russell wrote:
>  But what kind of daredevil coder would propose such a thing?)
> 
> Ah, so this is why you want ->next in preempt hooks.  Well, my plan for
> this sort of thing (for kvm has the same issues with the *STAR family of
> msrs) is to add a new hook on switching from kernel to userspace, and
> swap those msrs there.  This allows not only the guest1->guest2 case to
> be optimized, but also guest->kthread->guest, which is a common pattern
> with I/O (and very common with -rt, which runs interrupts in threads).

Adding instructions to the syscall path is not going to make you
popular.  But if you do it, I'll use it 8)

Thanks,
Rusty.

Re: [PATCH] lguest: disable SYSENTER for guests

[Rusty Russell]

On Thu, 2007-07-12 at 10:47 +0300, Avi Kivity wrote:
> Rusty Russell wrote:
>  But what kind of daredevil coder would propose such a thing?)
> 
> Ah, so this is why you want ->next in preempt hooks.  Well, my plan for
> this sort of thing (for kvm has the same issues with the *STAR family of
> msrs) is to add a new hook on switching from kernel to userspace, and
> swap those msrs there.  This allows not only the guest1->guest2 case to
> be optimized, but also guest->kthread->guest, which is a common pattern
> with I/O (and very common with -rt, which runs interrupts in threads).

Adding instructions to the syscall path is not going to make you
popular.  But if you do it, I'll use it 8)

Thanks,
Rusty.

Re: [PATCH] lguest: disable SYSENTER for guests

[Avi Kivity]

Rusty Russell wrote:
> The SYSENTER instruction jumps to a pre-programmed address at
> privilege level 0.  We must not allow execution of guest code at that
> privilege level, so disable sysenter when we enter the guest (and
> re-enable it on return).  This fixes current case where guest
> userspace can crash host.
>
> This save/restore adds 3% to guest context switch times.  (If only
> there were some kind of scheduler hook or something which would tell
> us when we were being preempted so we could fix this up lazily.  But
> what kind of daredevil coder would propose such a thing?)
>
>   

Ah, so this is why you want ->next in preempt hooks.  Well, my plan for
this sort of thing (for kvm has the same issues with the *STAR family of
msrs) is to add a new hook on switching from kernel to userspace, and
swap those msrs there.  This allows not only the guest1->guest2 case to
be optimized, but also guest->kthread->guest, which is a common pattern
with I/O (and very common with -rt, which runs interrupts in threads).

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

[PATCH] lguest: disable SYSENTER for guests

[Rusty Russell]

The SYSENTER instruction jumps to a pre-programmed address at
privilege level 0.  We must not allow execution of guest code at that
privilege level, so disable sysenter when we enter the guest (and
re-enable it on return).  This fixes current case where guest
userspace can crash host.

This save/restore adds 3% to guest context switch times.  (If only
there were some kind of scheduler hook or something which would tell
us when we were being preempted so we could fix this up lazily.  But
what kind of daredevil coder would propose such a thing?)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/lguest/core.c |    7 +++++++
 1 file changed, 7 insertions(+)

===================================================================
--- a/drivers/lguest/core.c
+++ b/drivers/lguest/core.c
@@ -338,6 +338,10 @@ int run_guest(struct lguest *lg, unsigne
 		if (lg->ts)
 			set_ts();
 
+		/* Don't let Guest do SYSENTER: we can't handle it. */
+		if (boot_cpu_has(X86_FEATURE_SEP))
+			wrmsr(MSR_IA32_SYSENTER_CS, 0, 0);
+
 		run_guest_once(lg, lguest_pages(raw_smp_processor_id()));
 
 		/* Save cr2 now if we page-faulted. */
@@ -345,6 +349,9 @@ int run_guest(struct lguest *lg, unsigne
 			cr2 = read_cr2();
 		else if (lg->regs->trapnum == 7)
 			math_state_restore();
+
+		if (boot_cpu_has(X86_FEATURE_SEP))
+			wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
 		local_irq_enable();
 
 		switch (lg->regs->trapnum) {