* [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback
@ 2012-06-13 10:02 Jan Beulich
2012-06-13 10:51 ` David Vrabel
2012-06-18 14:05 ` Keir Fraser
0 siblings, 2 replies; 4+ messages in thread
From: Jan Beulich @ 2012-06-13 10:02 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]
Rather than deferring the detection of these to the point where they
get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
to be raised by IRET, which invokes the guest's [fragile] fail-safe
callback), don't even allow such to be set.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -736,6 +736,14 @@ int arch_set_info_guest(
{
if ( !compat )
{
+#ifdef __x86_64__
+ if ( !is_canonical_address(c.nat->user_regs.eip) ||
+ !is_canonical_address(c.nat->event_callback_eip) ||
+ !is_canonical_address(c.nat->syscall_callback_eip) ||
+ !is_canonical_address(c.nat->failsafe_callback_eip) )
+ return -EINVAL;
+#endif
+
fixup_guest_stack_selector(d, c.nat->user_regs.ss);
fixup_guest_stack_selector(d, c.nat->kernel_ss);
fixup_guest_code_selector(d, c.nat->user_regs.cs);
@@ -745,7 +753,11 @@ int arch_set_info_guest(
#endif
for ( i = 0; i < 256; i++ )
+ {
+ if ( !is_canonical_address(c.nat->trap_ctxt[i].address) )
+ return -EINVAL;
fixup_guest_code_selector(d, c.nat->trap_ctxt[i].cs);
+ }
/* LDT safety checks. */
if ( ((c.nat->ldt_base & (PAGE_SIZE-1)) != 0) ||
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1033,6 +1033,9 @@ long arch_do_domctl(
#ifdef __x86_64__
if ( !is_hvm_domain(d) )
{
+ if ( !is_canonical_address(evc->sysenter_callback_eip) ||
+ !is_canonical_address(evc->syscall32_callback_eip) )
+ goto ext_vcpucontext_out;
fixup_guest_code_selector(d, evc->sysenter_callback_cs);
v->arch.pv_vcpu.sysenter_callback_cs =
evc->sysenter_callback_cs;
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3581,6 +3581,9 @@ long register_guest_nmi_callback(unsigne
struct domain *d = v->domain;
struct trap_info *t = &v->arch.pv_vcpu.trap_ctxt[TRAP_nmi];
+ if ( !is_canonical_address(address) )
+ return -EINVAL;
+
t->vector = TRAP_nmi;
t->flags = 0;
t->cs = (is_pv_32on64_domain(d) ?
@@ -3708,6 +3711,9 @@ long do_set_trap_table(XEN_GUEST_HANDLE(
if ( cur.address == 0 )
break;
+ if ( !is_canonical_address(cur.address) )
+ return -EINVAL;
+
fixup_guest_code_selector(curr->domain, cur.cs);
memcpy(&dst[cur.vector], &cur, sizeof(cur));
[-- Attachment #2: x86_64-canonical-checks.patch --]
[-- Type: text/plain, Size: 2781 bytes --]
x86-64: don't allow non-canonical addresses to be set for any callback
Rather than deferring the detection of these to the point where they
get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
to be raised by IRET, which invokes the guest's [fragile] fail-safe
callback), don't even allow such to be set.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -736,6 +736,14 @@ int arch_set_info_guest(
{
if ( !compat )
{
+#ifdef __x86_64__
+ if ( !is_canonical_address(c.nat->user_regs.eip) ||
+ !is_canonical_address(c.nat->event_callback_eip) ||
+ !is_canonical_address(c.nat->syscall_callback_eip) ||
+ !is_canonical_address(c.nat->failsafe_callback_eip) )
+ return -EINVAL;
+#endif
+
fixup_guest_stack_selector(d, c.nat->user_regs.ss);
fixup_guest_stack_selector(d, c.nat->kernel_ss);
fixup_guest_code_selector(d, c.nat->user_regs.cs);
@@ -745,7 +753,11 @@ int arch_set_info_guest(
#endif
for ( i = 0; i < 256; i++ )
+ {
+ if ( !is_canonical_address(c.nat->trap_ctxt[i].address) )
+ return -EINVAL;
fixup_guest_code_selector(d, c.nat->trap_ctxt[i].cs);
+ }
/* LDT safety checks. */
if ( ((c.nat->ldt_base & (PAGE_SIZE-1)) != 0) ||
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1033,6 +1033,9 @@ long arch_do_domctl(
#ifdef __x86_64__
if ( !is_hvm_domain(d) )
{
+ if ( !is_canonical_address(evc->sysenter_callback_eip) ||
+ !is_canonical_address(evc->syscall32_callback_eip) )
+ goto ext_vcpucontext_out;
fixup_guest_code_selector(d, evc->sysenter_callback_cs);
v->arch.pv_vcpu.sysenter_callback_cs =
evc->sysenter_callback_cs;
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3581,6 +3581,9 @@ long register_guest_nmi_callback(unsigne
struct domain *d = v->domain;
struct trap_info *t = &v->arch.pv_vcpu.trap_ctxt[TRAP_nmi];
+ if ( !is_canonical_address(address) )
+ return -EINVAL;
+
t->vector = TRAP_nmi;
t->flags = 0;
t->cs = (is_pv_32on64_domain(d) ?
@@ -3708,6 +3711,9 @@ long do_set_trap_table(XEN_GUEST_HANDLE(
if ( cur.address == 0 )
break;
+ if ( !is_canonical_address(cur.address) )
+ return -EINVAL;
+
fixup_guest_code_selector(curr->domain, cur.cs);
memcpy(&dst[cur.vector], &cur, sizeof(cur));
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback
2012-06-13 10:02 [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback Jan Beulich
@ 2012-06-13 10:51 ` David Vrabel
2012-06-13 11:20 ` Jan Beulich
2012-06-18 14:05 ` Keir Fraser
1 sibling, 1 reply; 4+ messages in thread
From: David Vrabel @ 2012-06-13 10:51 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 13/06/12 11:02, Jan Beulich wrote:
> Rather than deferring the detection of these to the point where they
> get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
> to be raised by IRET, which invokes the guest's [fragile] fail-safe
> callback), don't even allow such to be set.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -736,6 +736,14 @@ int arch_set_info_guest(
> {
> if ( !compat )
> {
> +#ifdef __x86_64__
> + if ( !is_canonical_address(c.nat->user_regs.eip) ||
> + !is_canonical_address(c.nat->event_callback_eip) ||
> + !is_canonical_address(c.nat->syscall_callback_eip) ||
> + !is_canonical_address(c.nat->failsafe_callback_eip) )
> + return -EINVAL;
> +#endif
> +
Would it be better to have
#ifndef __x86_64__
# define is_canonical_address(a) 1
#endif
somewhere?
David
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback
2012-06-13 10:51 ` David Vrabel
@ 2012-06-13 11:20 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2012-06-13 11:20 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel
>>> On 13.06.12 at 12:51, David Vrabel <dvrabel@cantab.net> wrote:
> On 13/06/12 11:02, Jan Beulich wrote:
>> Rather than deferring the detection of these to the point where they
>> get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
>> to be raised by IRET, which invokes the guest's [fragile] fail-safe
>> callback), don't even allow such to be set.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -736,6 +736,14 @@ int arch_set_info_guest(
>> {
>> if ( !compat )
>> {
>> +#ifdef __x86_64__
>> + if ( !is_canonical_address(c.nat->user_regs.eip) ||
>> + !is_canonical_address(c.nat->event_callback_eip) ||
>> + !is_canonical_address(c.nat->syscall_callback_eip) ||
>> + !is_canonical_address(c.nat->failsafe_callback_eip) )
>> + return -EINVAL;
>> +#endif
>> +
>
> Would it be better to have
>
> #ifndef __x86_64__
> # define is_canonical_address(a) 1
> #endif
>
> somewhere?
That one we have (otherwise other changes in this patch would
cause build failures), the problem here is that ->syscall_callback
doesn't exist for the 32-bit interface. And rater than just putting
that on line into a conditional, I preferred to frame the whole
addition.
Jan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback
2012-06-13 10:02 [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback Jan Beulich
2012-06-13 10:51 ` David Vrabel
@ 2012-06-18 14:05 ` Keir Fraser
1 sibling, 0 replies; 4+ messages in thread
From: Keir Fraser @ 2012-06-18 14:05 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 13/06/2012 11:02, "Jan Beulich" <JBeulich@suse.com> wrote:
> Rather than deferring the detection of these to the point where they
> get actually used (the fix for XSA-7, 25480:76eaf5966c05, causing a #GP
> to be raised by IRET, which invokes the guest's [fragile] fail-safe
> callback), don't even allow such to be set.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -736,6 +736,14 @@ int arch_set_info_guest(
> {
> if ( !compat )
> {
> +#ifdef __x86_64__
> + if ( !is_canonical_address(c.nat->user_regs.eip) ||
> + !is_canonical_address(c.nat->event_callback_eip) ||
> + !is_canonical_address(c.nat->syscall_callback_eip) ||
> + !is_canonical_address(c.nat->failsafe_callback_eip) )
> + return -EINVAL;
> +#endif
> +
> fixup_guest_stack_selector(d, c.nat->user_regs.ss);
> fixup_guest_stack_selector(d, c.nat->kernel_ss);
> fixup_guest_code_selector(d, c.nat->user_regs.cs);
> @@ -745,7 +753,11 @@ int arch_set_info_guest(
> #endif
>
> for ( i = 0; i < 256; i++ )
> + {
> + if ( !is_canonical_address(c.nat->trap_ctxt[i].address) )
> + return -EINVAL;
> fixup_guest_code_selector(d, c.nat->trap_ctxt[i].cs);
> + }
>
> /* LDT safety checks. */
> if ( ((c.nat->ldt_base & (PAGE_SIZE-1)) != 0) ||
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1033,6 +1033,9 @@ long arch_do_domctl(
> #ifdef __x86_64__
> if ( !is_hvm_domain(d) )
> {
> + if ( !is_canonical_address(evc->sysenter_callback_eip) ||
> + !is_canonical_address(evc->syscall32_callback_eip) )
> + goto ext_vcpucontext_out;
> fixup_guest_code_selector(d, evc->sysenter_callback_cs);
> v->arch.pv_vcpu.sysenter_callback_cs =
> evc->sysenter_callback_cs;
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -3581,6 +3581,9 @@ long register_guest_nmi_callback(unsigne
> struct domain *d = v->domain;
> struct trap_info *t = &v->arch.pv_vcpu.trap_ctxt[TRAP_nmi];
>
> + if ( !is_canonical_address(address) )
> + return -EINVAL;
> +
> t->vector = TRAP_nmi;
> t->flags = 0;
> t->cs = (is_pv_32on64_domain(d) ?
> @@ -3708,6 +3711,9 @@ long do_set_trap_table(XEN_GUEST_HANDLE(
> if ( cur.address == 0 )
> break;
>
> + if ( !is_canonical_address(cur.address) )
> + return -EINVAL;
> +
> fixup_guest_code_selector(curr->domain, cur.cs);
>
> memcpy(&dst[cur.vector], &cur, sizeof(cur));
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-06-18 14:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-13 10:02 [PATCH] x86-64: don't allow non-canonical addresses to be set for any callback Jan Beulich
2012-06-13 10:51 ` David Vrabel
2012-06-13 11:20 ` Jan Beulich
2012-06-18 14:05 ` Keir Fraser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).