[PATCH 0/3] GDBSX fixes

[PATCH 0/3] GDBSX fixes

[Andrew Cooper]

This series implements two fixes to GDBSX.  Patch 1 is a common helper.  Patch
2 fixes a recently-introduced regression, while patch 3 fixes several issues.

The resuts are compile tested only.

Andrew Cooper (3):
  xen/common: Introduce vcpu_{,un}pause_by_systemcontroller() helpers
  x86/gdbsx: Invert preconditions for XEN_DOMCTL_gdbsx_{,un}pausevcpu
    hypercalls
  xen/gdbsx: Security audit of {,un}pausevcpu and domstatus hypercalls

 docs/misc/xsm-flask.txt |    3 ---
 xen/arch/x86/domctl.c   |   20 ++++++++++----------
 xen/common/domain.c     |   40 ++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/sched.h |    7 ++++++-
 4 files changed, 56 insertions(+), 14 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] xen/common: Introduce vcpu_{, un}pause_by_systemcontroller() helpers

[Andrew Cooper]

Which will be used by following patches.  Reorder the function declarations
for vcpu/domain pause/unpause to group by vcpu/domain and visually separate
them slightly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/domain.c     |   40 ++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/sched.h |    7 ++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index cd64aea..d7a84cf 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -878,6 +878,46 @@ void vcpu_unpause(struct vcpu *v)
         vcpu_wake(v);
 }
 
+int vcpu_pause_by_systemcontroller(struct vcpu *v)
+{
+    int old, new, prev = v->controller_pause_count;
+
+    do
+    {
+        old = prev;
+        new = old + 1;
+
+        if ( new > 255 )
+            return -EUSERS;
+
+        prev = cmpxchg(&v->controller_pause_count, old, new);
+    } while ( prev != old );
+
+    vcpu_pause(v);
+
+    return 0;
+}
+
+int vcpu_unpause_by_systemcontroller(struct vcpu *v)
+{
+    int old, new, prev = v->controller_pause_count;
+
+    do
+    {
+        old = prev;
+        new = old - 1;
+
+        if ( new < 0 )
+            return -EINVAL;
+
+        prev = cmpxchg(&v->controller_pause_count, old, new);
+    } while ( prev != old );
+
+    vcpu_unpause(v);
+
+    return 0;
+}
+
 void domain_pause(struct domain *d)
 {
     struct vcpu *v;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 62a4785..67eb8f0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -216,6 +216,8 @@ struct vcpu
 
     /* VCPU paused for mem_event replies. */
     atomic_t         mem_event_pause_count;
+    /* VCPU paused by system controller. */
+    int              controller_pause_count;
 
     /* IRQ-safe virq_lock protects against delivering VIRQ to stale evtchn. */
     evtchn_port_t    virq_to_evtchn[NR_VIRQS];
@@ -768,9 +770,12 @@ void vcpu_block(void);
 void vcpu_unblock(struct vcpu *v);
 void vcpu_pause(struct vcpu *v);
 void vcpu_pause_nosync(struct vcpu *v);
+void vcpu_unpause(struct vcpu *v);
+int vcpu_pause_by_systemcontroller(struct vcpu *v);
+int vcpu_unpause_by_systemcontroller(struct vcpu *v);
+
 void domain_pause(struct domain *d);
 void domain_pause_nosync(struct domain *d);
-void vcpu_unpause(struct vcpu *v);
 void domain_unpause(struct domain *d);
 int domain_unpause_by_systemcontroller(struct domain *d);
 int __domain_pause_by_systemcontroller(struct domain *d,
-- 
1.7.10.4

[PATCH 2/3] x86/gdbsx: Invert preconditions for XEN_DOMCTL_gdbsx_{, un}pausevcpu hypercalls

[Andrew Cooper]

c/s 3eb1c708ab "properly reference count DOMCTL_{,un}pausedomain hypercalls"
accidentally inverted the use of d->controller_pause_count.

Revert back to how it was originally, i.e. the XEN_DOMCTL_gdbsx_{,un}pausevcpu
hypercalls are only valid for a domain already paused by the system controller.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/domctl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index d62c715..243f42f 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1027,7 +1027,7 @@ long arch_do_domctl(
         struct vcpu *v;
 
         ret = -EBUSY;
-        if ( d->controller_pause_count > 0 )
+        if ( !d->controller_pause_count )
             break;
         ret = -EINVAL;
         if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
@@ -1043,7 +1043,7 @@ long arch_do_domctl(
         struct vcpu *v;
 
         ret = -EBUSY;
-        if ( d->controller_pause_count > 0 )
+        if ( !d->controller_pause_count )
             break;
         ret = -EINVAL;
         if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
-- 
1.7.10.4

[PATCH 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Andrew Cooper]

XEN_DOMCTL_gdbsx_domstatus is already safe.  It loops at most over every vcpu
in a domain and breaks at the first vcpu with an event pending, marking it as
not-pending.

XEN_DOMCTL_gdbsx_pausevcpu had an incorrect bounds check against the vcpu id,
allowing an overflow of d->vcpu[] with an id between d->max_vcpus and
MAX_VIRT_CPUS.  It was also able to overflow a vcpus pause count by many
repeated hypercalls.

The bounds check is fixed, and vcpu_pause() has been replaced with
vcpu_pause_by_systemcontroller() which cuts out at 255 uses.

XEN_DOMCTL_gdbsx_unpausevcpu suffered from the same bounds problems as its
pause counterpart, and is fixed in exactly the same way.  Despite the
atomic_read(&v->pause_count), this code didn't successfully prevent against an
underflow of the vcpu pause count.

The vcpu_unpause() has been replaced with vcpu_pause_by_systemcontroller()
which correctly prevents against underflow.  The printk() is updated to have a
proper guest logging level, and provide more useful information in the XSM
case of one domain having debugger privileges over another.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 docs/misc/xsm-flask.txt |    3 ---
 xen/arch/x86/domctl.c   |   16 ++++++++--------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt
index 31b9d27..9559028 100644
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -96,9 +96,6 @@ __HYPERVISOR_domctl (xen/include/public/domctl.h)
  * XEN_DOMCTL_set_broken_page_p2m
  * XEN_DOMCTL_setnodeaffinity
  * XEN_DOMCTL_gdbsx_guestmemio
- * XEN_DOMCTL_gdbsx_pausevcpu
- * XEN_DOMCTL_gdbsx_unpausevcpu
- * XEN_DOMCTL_gdbsx_domstatus
 
 __HYPERVISOR_sysctl (xen/include/public/sysctl.h)
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 243f42f..6b2479e 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1030,11 +1030,10 @@ long arch_do_domctl(
         if ( !d->controller_pause_count )
             break;
         ret = -EINVAL;
-        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
+        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu > d->max_vcpus ||
              (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
             break;
-        vcpu_pause(v);
-        ret = 0;
+        ret = vcpu_pause_by_systemcontroller(v);
     }
     break;
 
@@ -1046,13 +1045,14 @@ long arch_do_domctl(
         if ( !d->controller_pause_count )
             break;
         ret = -EINVAL;
-        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
+        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu > d->max_vcpus ||
              (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
             break;
-        if ( !atomic_read(&v->pause_count) )
-            printk("WARN: Unpausing vcpu:%d which is not paused\n", v->vcpu_id);
-        vcpu_unpause(v);
-        ret = 0;
+        ret = vcpu_unpause_by_systemcontroller(v);
+        if ( ret == -EINVAL )
+            printk(XENLOG_G_WARNING
+                   "WARN: d%d attempting to unpause %pv which is not paused\n",
+                   current->domain->domain_id, v);
     }
     break;
 
-- 
1.7.10.4

Re: [PATCH 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Tim Deegan]

At 11:56 +0100 on 24 Jul (1406199410), Andrew Cooper wrote:
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 243f42f..6b2479e 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1030,11 +1030,10 @@ long arch_do_domctl(
>          if ( !d->controller_pause_count )
>              break;
>          ret = -EINVAL;
> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
> +        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu > d->max_vcpus ||

Not >= ?

>               (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
>              break;
> -        vcpu_pause(v);
> -        ret = 0;
> +        ret = vcpu_pause_by_systemcontroller(v);
>      }
>      break;
>  
> @@ -1046,13 +1045,14 @@ long arch_do_domctl(
>          if ( !d->controller_pause_count )
>              break;
>          ret = -EINVAL;
> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
> +        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu > d->max_vcpus ||

Same thing here.

Tim.

Re: [PATCH 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Andrew Cooper]

On 24/07/14 12:07, Tim Deegan wrote:
> At 11:56 +0100 on 24 Jul (1406199410), Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>> index 243f42f..6b2479e 100644
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -1030,11 +1030,10 @@ long arch_do_domctl(
>>          if ( !d->controller_pause_count )
>>              break;
>>          ret = -EINVAL;
>> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
>> +        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu > d->max_vcpus ||
> Not >= ?

Indeed.  I seem to have formatted an old patch.  v2 on its way.

~Andrew

>
>>               (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
>>              break;
>> -        vcpu_pause(v);
>> -        ret = 0;
>> +        ret = vcpu_pause_by_systemcontroller(v);
>>      }
>>      break;
>>  
>> @@ -1046,13 +1045,14 @@ long arch_do_domctl(
>>          if ( !d->controller_pause_count )
>>              break;
>>          ret = -EINVAL;
>> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
>> +        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu > d->max_vcpus ||
> Same thing here.
>
> Tim.

[Patch v2 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Andrew Cooper]

XEN_DOMCTL_gdbsx_domstatus is already safe.  It loops at most over every vcpu
in a domain and breaks at the first vcpu with an event pending, marking it as
not-pending.

XEN_DOMCTL_gdbsx_pausevcpu had an incorrect bounds check against the vcpu id,
allowing an overflow of d->vcpu[] with an id between d->max_vcpus and
MAX_VIRT_CPUS.  It was also able to overflow a vcpus pause count by many
repeated hypercalls.

The bounds check is fixed, and vcpu_pause() has been replaced with
vcpu_pause_by_systemcontroller() which cuts out at 255 uses.

XEN_DOMCTL_gdbsx_unpausevcpu suffered from the same bounds problems as its
pause counterpart, and is fixed in exactly the same way.  Despite the
atomic_read(&v->pause_count), this code didn't successfully prevent against an
underflow of the vcpu pause count.

The vcpu_unpause() has been replaced with vcpu_pause_by_systemcontroller()
which correctly prevents against underflow.  The printk() is updated to have a
proper guest logging level, and provide more useful information in the XSM
case of one domain having debugger privileges over another.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Mukesh Rathor <mukesh.rathor@oracle.com>

---
v2: (re)correct vpu id bounds check
---
 docs/misc/xsm-flask.txt |    3 ---
 xen/arch/x86/domctl.c   |   16 ++++++++--------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt
index 31b9d27..9559028 100644
--- a/docs/misc/xsm-flask.txt
+++ b/docs/misc/xsm-flask.txt
@@ -96,9 +96,6 @@ __HYPERVISOR_domctl (xen/include/public/domctl.h)
  * XEN_DOMCTL_set_broken_page_p2m
  * XEN_DOMCTL_setnodeaffinity
  * XEN_DOMCTL_gdbsx_guestmemio
- * XEN_DOMCTL_gdbsx_pausevcpu
- * XEN_DOMCTL_gdbsx_unpausevcpu
- * XEN_DOMCTL_gdbsx_domstatus
 
 __HYPERVISOR_sysctl (xen/include/public/sysctl.h)
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 243f42f..d1517c4 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1030,11 +1030,10 @@ long arch_do_domctl(
         if ( !d->controller_pause_count )
             break;
         ret = -EINVAL;
-        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
+        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
              (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
             break;
-        vcpu_pause(v);
-        ret = 0;
+        ret = vcpu_pause_by_systemcontroller(v);
     }
     break;
 
@@ -1046,13 +1045,14 @@ long arch_do_domctl(
         if ( !d->controller_pause_count )
             break;
         ret = -EINVAL;
-        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
+        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
              (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
             break;
-        if ( !atomic_read(&v->pause_count) )
-            printk("WARN: Unpausing vcpu:%d which is not paused\n", v->vcpu_id);
-        vcpu_unpause(v);
-        ret = 0;
+        ret = vcpu_unpause_by_systemcontroller(v);
+        if ( ret == -EINVAL )
+            printk(XENLOG_G_WARNING
+                   "WARN: d%d attempting to unpause %pv which is not paused\n",
+                   current->domain->domain_id, v);
     }
     break;
 
-- 
1.7.10.4

Re: [Patch v2 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Tim Deegan]

At 12:16 +0100 on 24 Jul (1406200565), Andrew Cooper wrote:
> XEN_DOMCTL_gdbsx_domstatus is already safe.  It loops at most over every vcpu
> in a domain and breaks at the first vcpu with an event pending, marking it as
> not-pending.
> 
> XEN_DOMCTL_gdbsx_pausevcpu had an incorrect bounds check against the vcpu id,
> allowing an overflow of d->vcpu[] with an id between d->max_vcpus and
> MAX_VIRT_CPUS.  It was also able to overflow a vcpus pause count by many
> repeated hypercalls.
> 
> The bounds check is fixed, and vcpu_pause() has been replaced with
> vcpu_pause_by_systemcontroller() which cuts out at 255 uses.
> 
> XEN_DOMCTL_gdbsx_unpausevcpu suffered from the same bounds problems as its
> pause counterpart, and is fixed in exactly the same way.  Despite the
> atomic_read(&v->pause_count), this code didn't successfully prevent against an
> underflow of the vcpu pause count.
> 
> The vcpu_unpause() has been replaced with vcpu_pause_by_systemcontroller()
> which correctly prevents against underflow.  The printk() is updated to have a
> proper guest logging level, and provide more useful information in the XSM
> case of one domain having debugger privileges over another.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [Patch v2 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Jan Beulich]

>>> On 24.07.14 at 13:16, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1030,11 +1030,10 @@ long arch_do_domctl(
>          if ( !d->controller_pause_count )
>              break;
>          ret = -EINVAL;
> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
> +        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||

As mentioned on IRC - are you sure you can replace (rather than
amend) the previous bounds check? I.e. do you _know_ that gdbsx
can deal with guests having more than MAX_VIRT_CPUS vCPU-s?

Jan

Re: [Patch v2 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Andrew Cooper]

On 24/07/14 13:58, Jan Beulich wrote:
>>>> On 24.07.14 at 13:16, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -1030,11 +1030,10 @@ long arch_do_domctl(
>>          if ( !d->controller_pause_count )
>>              break;
>>          ret = -EINVAL;
>> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
>> +        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
> As mentioned on IRC - are you sure you can replace (rather than
> amend) the previous bounds check? I.e. do you _know_ that gdbsx
> can deal with guests having more than MAX_VIRT_CPUS vCPU-s?
>
> Jan
>

I have no idea whether it can or not, but if it asks for a legitimate ID
greater than MAX_VIRT_CPUS, then I think it is reasonable to assume yes.

~Andrew

Re: [Patch v2 3/3] xen/gdbsx: Security audit of {, un}pausevcpu and domstatus hypercalls

[Mukesh Rathor]

On Thu, 24 Jul 2014 14:02:43 +0100
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 24/07/14 13:58, Jan Beulich wrote:
> >>>> On 24.07.14 at 13:16, <andrew.cooper3@citrix.com> wrote:
> >> --- a/xen/arch/x86/domctl.c
> >> +++ b/xen/arch/x86/domctl.c
> >> @@ -1030,11 +1030,10 @@ long arch_do_domctl(
> >>          if ( !d->controller_pause_count )
> >>              break;
> >>          ret = -EINVAL;
> >> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS
> >> ||
> >> +        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
> > As mentioned on IRC - are you sure you can replace (rather than
> > amend) the previous bounds check? I.e. do you _know_ that gdbsx
> > can deal with guests having more than MAX_VIRT_CPUS vCPU-s?
> >
> > Jan
> >
> 
> I have no idea whether it can or not, but if it asks for a legitimate
> ID greater than MAX_VIRT_CPUS, then I think it is reasonable to
> assume yes.

In earlier days, gdbsx was not part of xen. So I, in my earlier xen
days, added call to unpause a vcpu with INTMAX (~0u) vcpuid, and see
if xen returns ENOSYS. 

The above change doesn't affect that, and it is good to check for
d->max_vcpus instead of MAX_VIRT_CPUS.

vcpuid in gdbsx is int, so it can handle INTMAX-1 vcpus, it thinks
INTMAX is invalid vcpu :).

thanks,
mukesh