[prev in list] [next in list] [prev in thread] [next in thread]
List: oss-security
Subject: [oss-security] Xen Security Advisory 296 v4 (CVE-2019-18420) - VCPUOP_initialise DoS
From: Xen.org security team <security () xen ! org>
Date: 2019-10-31 12:29:09
Message-ID: E1iQ9Zd-0002pV-Ht () xenbits ! xenproject ! org
[Download RAW message or body]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2019-18420 / XSA-296
version 4
VCPUOP_initialise DoS
UPDATES IN VERSION 4
====================
Public release.
ISSUE DESCRIPTION
=================
hypercall_create_continuation() is a variadic function which uses a
printf-like format string to interpret its parameters. Error handling
for a bad format character was done using BUG(), which crashes Xen.
One path, via the VCPUOP_initialise hypercall, has a bad format
character. The BUG() can be hit if VCPUOP_initialise executes for a
sufficiently long period of time for a continuation to be created.
IMPACT
======
Malicious guests may cause a hypervisor crash, resulting in a Denial of
Service (Dos).
VULNERABLE SYSTEMS
==================
Xen versions 4.6 and newer are vulnerable. Xen versions 4.5 and earlier
are not vulnerable.
Only x86 PV guests can exploit the vulnerability. HVM and PVH guests,
and guests on ARM systems, cannot exploit the vulnerability.
MITIGATION
==========
There are no mitigations.
CREDITS
=======
This issue was discovered by Andrew Cooper of Citrix.
RESOLUTION
==========
Applying the appropriate attached patch resolves this issue.
xsa296.patch Xen 4.9 ... unstable
xsa296-4.8.patch Xen 4.7 ... 4.8
$ sha256sum xsa296*
71bd433f788dd511fad90165bc5ba9bcabe949eecd912f2a616e3c996960d67d xsa296.meta
ccfd81b162b8535d952f56b1f87dfdd960e71bf07c1cf8388976e78e2e86cde5 xsa296.patch
b283be3df6789402553172b7fd582bfffb4fa72a6b33543439bd2fb1b87bfbd4 xsa296-4.8.patch
$
DEPLOYMENT DURING EMBARGO
=========================
Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.
But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).
Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.
(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable. This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)
For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----
iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAl2600kMHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZn10IAKQhLY9RfkgZhY/6cACYnXqFGWhS6MvyeZrVs1L4
BaojMJilpAo7kc9Xcf+0ThxKI5M/eEeDUdGjFHfBtOUrjOAhaZjaYI1paJwX0JEV
QoAMQERTtopFnkCNtvykrMiKZQ2xp6hiios+32PvDdVnjO+rkrKESRNoBVNYlC1f
qN8SbZ6m0C5jP4C82ifDEeJHJsVJtfYQSeRl95pgCmsmxxd3x7q7ubPcR6kizT5t
Bu4sAtrWNF5zaBrb5kL29yohn4oBLmMV5NO0hSlCbR3FgeFZ7LTwiz8y4d7tVSHg
fjbsvhmhfhYO1OnZWFea3QXMgfZjsg6qq9jBnpmGu35WVMY=
=xUlU
-----END PGP SIGNATURE-----
["xsa296.meta" (application/octet-stream)]
["xsa296.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: xen/hypercall: Don't use BUG() for parameter checking in hypercall_create_continuation()
Since c/s 1d429034 "hypercall: update vcpu_op to take an unsigned vcpuid",
which incorrectly swapped 'i' for 'u' in the parameter type list, guests have
been able to hit the BUG() in next_args()'s default case.
Correct these back to 'i'.
In addition, make adjustments to prevent this class of issue from occurring in
the future - crashing Xen is not an appropriate form of parameter checking.
Capitalise NEXT_ARG() to catch all uses, to highlight that it is a macro doing
non-function-like things behind the scenes, and undef it when appropriate.
Implement a bad_fmt: block which prints an error, asserts unreachable, and
crashes the guest.
On the ARM side, drop all parameter checking of p. It is asymmetric with the
x86 side, and akin to expecting memcpy() or sprintf() to check their src/fmt
parameter before use. A caller passing "" or something other than a string
literal will be obvious during code review.
This is XSA-296.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 941bbff4fe..a3da8e9c08 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -383,14 +383,15 @@ void sync_vcpu_execstate(struct vcpu *v)
/* Nothing to do -- no lazy switching */
}
-#define next_arg(fmt, args) ({ \
+#define NEXT_ARG(fmt, args) \
+({ \
unsigned long __arg; \
switch ( *(fmt)++ ) \
{ \
case 'i': __arg = (unsigned long)va_arg(args, unsigned int); break; \
case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break; \
case 'h': __arg = (unsigned long)va_arg(args, void *); break; \
- default: __arg = 0; BUG(); \
+ default: goto bad_fmt; \
} \
__arg; \
})
@@ -405,9 +406,6 @@ unsigned long hypercall_create_continuation(
unsigned int i;
va_list args;
- /* All hypercalls take at least one argument */
- BUG_ON( !p || *p == '\0' );
-
current->hcall_preempted = true;
va_start(args, format);
@@ -415,7 +413,7 @@ unsigned long hypercall_create_continuation(
if ( mcs->flags & MCSF_in_multicall )
{
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = next_arg(p, args);
+ mcs->call.args[i] = NEXT_ARG(p, args);
/* Return value gets written back to mcs->call.result */
rc = mcs->call.result;
@@ -431,7 +429,7 @@ unsigned long hypercall_create_continuation(
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
@@ -454,7 +452,7 @@ unsigned long hypercall_create_continuation(
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
@@ -475,8 +473,16 @@ unsigned long hypercall_create_continuation(
va_end(args);
return rc;
+
+ bad_fmt:
+ gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+ ASSERT_UNREACHABLE();
+ domain_crash(current->domain);
+ return 0;
}
+#undef NEXT_ARG
+
void startup_cpu_idle_loop(void)
{
struct vcpu *v = current;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index d483dbaa6b..4643e5eb43 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -80,14 +80,15 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
#undef COMP
#undef ARGS
-#define next_arg(fmt, args) ({ \
+#define NEXT_ARG(fmt, args) \
+({ \
unsigned long __arg; \
switch ( *(fmt)++ ) \
{ \
case 'i': __arg = (unsigned long)va_arg(args, unsigned int); break; \
case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break; \
case 'h': __arg = (unsigned long)va_arg(args, void *); break; \
- default: __arg = 0; BUG(); \
+ default: goto bad_fmt; \
} \
__arg; \
})
@@ -109,7 +110,7 @@ unsigned long hypercall_create_continuation(
if ( mcs->flags & MCSF_in_multicall )
{
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = next_arg(p, args);
+ mcs->call.args[i] = NEXT_ARG(p, args);
}
else
{
@@ -121,7 +122,7 @@ unsigned long hypercall_create_continuation(
{
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
case 0: regs->rdi = arg; break;
@@ -137,7 +138,7 @@ unsigned long hypercall_create_continuation(
{
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
case 0: regs->rbx = arg; break;
@@ -154,8 +155,16 @@ unsigned long hypercall_create_continuation(
va_end(args);
return op;
+
+ bad_fmt:
+ gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+ ASSERT_UNREACHABLE();
+ domain_crash(curr->domain);
+ return 0;
}
+#undef NEXT_ARG
+
int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
unsigned int mask, ...)
{
diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
index 39877b3ab2..2531fa7421 100644
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -81,7 +81,7 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) ar
}
if ( rc == -ERESTART )
- rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+ rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
cmd, vcpuid, arg);
break;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 2308588052..65bcd85e34 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1411,7 +1411,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
rc = arch_initialise_vcpu(v, arg);
if ( rc == -ERESTART )
- rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+ rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
cmd, vcpuid, arg);
break;
["xsa296-4.8.patch" (application/octet-stream)]
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: xen/hypercall: Don't use BUG() for parameter checking in hypercall_create_continuation()
Since c/s 1d429034 "hypercall: update vcpu_op to take an unsigned vcpuid",
which incorrectly swapped 'i' for 'u' in the parameter type list, guests have
been able to hit the BUG() in next_args()'s default case.
Correct these back to 'i'.
In addition, make adjustments to prevent this class of issue from occurring in
the future - crashing Xen is not an appropriate form of parameter checking.
Capitalise NEXT_ARG() to catch all uses, to highlight that it is a macro doing
non-function-like things behind the scenes, and undef it when appropriate.
Implement a bad_fmt: block which prints an error, asserts unreachable, and
crashes the guest.
On the ARM side, drop all parameter checking of p. It is asymmetric with the
x86 side, and akin to expecting memcpy() or sprintf() to check their src/fmt
parameter before use. A caller passing "" or something other than a string
literal will be obvious during code review.
This is XSA-296.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d9e796dcbe..f6678d2227 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -336,14 +336,15 @@ void sync_vcpu_execstate(struct vcpu *v)
/* Nothing to do -- no lazy switching */
}
-#define next_arg(fmt, args) ({ \
+#define NEXT_ARG(fmt, args) \
+({ \
unsigned long __arg; \
switch ( *(fmt)++ ) \
{ \
case 'i': __arg = (unsigned long)va_arg(args, unsigned int); break; \
case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break; \
case 'h': __arg = (unsigned long)va_arg(args, void *); break; \
- default: __arg = 0; BUG(); \
+ default: goto bad_fmt; \
} \
__arg; \
})
@@ -373,9 +374,6 @@ unsigned long hypercall_create_continuation(
unsigned int i;
va_list args;
- /* All hypercalls take at least one argument */
- BUG_ON( !p || *p == '\0' );
-
va_start(args, format);
if ( mcs->flags & MCSF_in_multicall )
@@ -383,7 +381,7 @@ unsigned long hypercall_create_continuation(
__set_bit(_MCSF_call_preempted, &mcs->flags);
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = next_arg(p, args);
+ mcs->call.args[i] = NEXT_ARG(p, args);
/* Return value gets written back to mcs->call.result */
rc = mcs->call.result;
@@ -402,7 +400,7 @@ unsigned long hypercall_create_continuation(
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
@@ -425,7 +423,7 @@ unsigned long hypercall_create_continuation(
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
@@ -446,8 +444,16 @@ unsigned long hypercall_create_continuation(
va_end(args);
return rc;
+
+ bad_fmt:
+ gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+ ASSERT_UNREACHABLE();
+ domain_crash(current->domain);
+ return 0;
}
+#undef NEXT_ARG
+
void startup_cpu_idle_loop(void)
{
struct vcpu *v = current;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ddeb68f967..3946ea38fd 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2403,14 +2403,15 @@ void sync_vcpu_execstate(struct vcpu *v)
flush_tlb_mask(v->vcpu_dirty_cpumask);
}
-#define next_arg(fmt, args) ({ \
+#define NEXT_ARG(fmt, args) \
+({ \
unsigned long __arg; \
switch ( *(fmt)++ ) \
{ \
case 'i': __arg = (unsigned long)va_arg(args, unsigned int); break; \
case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break; \
case 'h': __arg = (unsigned long)va_arg(args, void *); break; \
- default: __arg = 0; BUG(); \
+ default: goto bad_fmt; \
} \
__arg; \
})
@@ -2449,7 +2450,7 @@ unsigned long hypercall_create_continuation(
__set_bit(_MCSF_call_preempted, &mcs->flags);
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = next_arg(p, args);
+ mcs->call.args[i] = NEXT_ARG(p, args);
}
else
{
@@ -2470,7 +2471,7 @@ unsigned long hypercall_create_continuation(
{
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
case 0: regs->rdi = arg; break;
@@ -2486,7 +2487,7 @@ unsigned long hypercall_create_continuation(
{
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
case 0: regs->ebx = arg; break;
@@ -2503,8 +2504,16 @@ unsigned long hypercall_create_continuation(
va_end(args);
return op;
+
+ bad_fmt:
+ gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+ ASSERT_UNREACHABLE();
+ domain_crash(current->domain);
+ return 0;
}
+#undef NEXT_ARG
+
int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
unsigned int mask, ...)
{
diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
index 88bfdc836d..d446ed131b 100644
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -81,7 +81,7 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) ar
}
if ( rc == -ERESTART )
- rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+ rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
cmd, vcpuid, arg);
break;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 740163ee77..28d7903a96 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1277,7 +1277,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
rc = arch_initialise_vcpu(v, arg);
if ( rc == -ERESTART )
- rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+ rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
cmd, vcpuid, arg);
break;
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic