[libvirt] [PATCH] daemon: Fix crash in virTypedParameterArrayClear
Daemon uses the following pattern when dispatching APIs with typed parameters: VIR_ALLOC_N(params, nparams); virDomain*(dom, params, &nparams, flags); virTypedParameterArrayClear(params, nparams); In case nparams was originally set to 0, virDomain* API would fill it with the number of typed parameters it can provide and we would use this number (rather than zero) to clear params. Because VIR_ALLOC* returns non-NULL pointer even if size is 0, the code would end up walking through random memory. If we were lucky enough and the memory contained 7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a random pointer and crash. Let's make sure params stays NULL when nparams is 0. --- daemon/remote.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 80626a2..d25717c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -989,7 +989,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) + if (nparams && VIR_ALLOC_N(params, nparams) < 0) goto no_memory; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) @@ -1098,7 +1098,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) + if (nparams && VIR_ALLOC_N(params, nparams) < 0) goto no_memory; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) @@ -1279,7 +1279,7 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (nparams && VIR_ALLOC_N(params, nparams) < 0) { virReportOOMError(); goto cleanup; } @@ -1753,7 +1753,7 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (nparams && VIR_ALLOC_N(params, nparams) < 0) { virReportOOMError(); goto cleanup; } @@ -1818,7 +1818,7 @@ remoteDispatchDomainGetNumaParameters(virNetServerPtr server ATTRIBUTE_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (nparams && VIR_ALLOC_N(params, nparams) < 0) { virReportOOMError(); goto cleanup; } @@ -1883,7 +1883,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (nparams && VIR_ALLOC_N(params, nparams) < 0) { virReportOOMError(); goto cleanup; } @@ -2143,7 +2143,7 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (nparams && VIR_ALLOC_N(params, nparams) < 0) { virReportOOMError(); goto cleanup; } @@ -3646,7 +3646,7 @@ remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUS virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (nparams && VIR_ALLOC_N(params, nparams) < 0) { virReportOOMError(); goto cleanup; } -- 1.7.11.1
On 2012年07月30日 19:55, Jiri Denemark wrote:
Daemon uses the following pattern when dispatching APIs with typed parameters:
VIR_ALLOC_N(params, nparams); virDomain*(dom, params,&nparams, flags); virTypedParameterArrayClear(params, nparams);
In case nparams was originally set to 0, virDomain* API would fill it with the number of typed parameters it can provide and we would use this number (rather than zero) to clear params. Because VIR_ALLOC* returns non-NULL pointer even if size is 0, the code would end up walking through random memory. If we were lucky enough and the memory contained 7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a random pointer and crash.
Let's make sure params stays NULL when nparams is 0. --- daemon/remote.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 80626a2..d25717c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -989,7 +989,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams)< 0) + if (nparams&& VIR_ALLOC_N(params, nparams)< 0) goto no_memory;
if (!(dom = get_nonnull_domain(priv->conn, args->dom))) @@ -1098,7 +1098,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams)< 0) + if (nparams&& VIR_ALLOC_N(params, nparams)< 0)
Isn't nparams for these 2 APIs are guarantee positive in the APIs? virCheckPositiveArgGoto(*nparams, error); Others look good. Regards, Osier
On Mon, Jul 30, 2012 at 21:08:22 +0800, Osier Yang wrote:
On 2012年07月30日 19:55, Jiri Denemark wrote:
Daemon uses the following pattern when dispatching APIs with typed parameters:
VIR_ALLOC_N(params, nparams); virDomain*(dom, params,&nparams, flags); virTypedParameterArrayClear(params, nparams);
In case nparams was originally set to 0, virDomain* API would fill it with the number of typed parameters it can provide and we would use this number (rather than zero) to clear params. Because VIR_ALLOC* returns non-NULL pointer even if size is 0, the code would end up walking through random memory. If we were lucky enough and the memory contained 7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a random pointer and crash.
Let's make sure params stays NULL when nparams is 0. --- daemon/remote.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 80626a2..d25717c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -989,7 +989,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams)< 0) + if (nparams&& VIR_ALLOC_N(params, nparams)< 0) goto no_memory;
if (!(dom = get_nonnull_domain(priv->conn, args->dom))) @@ -1098,7 +1098,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams)< 0) + if (nparams&& VIR_ALLOC_N(params, nparams)< 0)
Isn't nparams for these 2 APIs are guarantee positive in the APIs?
virCheckPositiveArgGoto(*nparams, error);
Yes but this function is an entry point from RPC and the daemon should not rely on client-side checking. Jirka
On 2012年07月30日 22:29, Jiri Denemark wrote:
On Mon, Jul 30, 2012 at 21:08:22 +0800, Osier Yang wrote:
On 2012年07月30日 19:55, Jiri Denemark wrote:
Daemon uses the following pattern when dispatching APIs with typed parameters:
VIR_ALLOC_N(params, nparams); virDomain*(dom, params,&nparams, flags); virTypedParameterArrayClear(params, nparams);
In case nparams was originally set to 0, virDomain* API would fill it with the number of typed parameters it can provide and we would use this number (rather than zero) to clear params. Because VIR_ALLOC* returns non-NULL pointer even if size is 0, the code would end up walking through random memory. If we were lucky enough and the memory contained 7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a random pointer and crash.
Let's make sure params stays NULL when nparams is 0. --- daemon/remote.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 80626a2..d25717c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -989,7 +989,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams)< 0) + if (nparams&& VIR_ALLOC_N(params, nparams)< 0) goto no_memory;
if (!(dom = get_nonnull_domain(priv->conn, args->dom))) @@ -1098,7 +1098,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams)< 0) + if (nparams&& VIR_ALLOC_N(params, nparams)< 0)
Isn't nparams for these 2 APIs are guarantee positive in the APIs?
virCheckPositiveArgGoto(*nparams, error);
Yes but this function is an entry point from RPC and the daemon should not rely on client-side checking.
Makes sense, ACK. Osier
On Mon, Jul 30, 2012 at 22:52:23 +0800, Osier Yang wrote:
On 2012年07月30日 19:55, Jiri Denemark wrote:
Daemon uses the following pattern when dispatching APIs with typed parameters:
VIR_ALLOC_N(params, nparams); virDomain*(dom, params,&nparams, flags); virTypedParameterArrayClear(params, nparams);
In case nparams was originally set to 0, virDomain* API would fill it with the number of typed parameters it can provide and we would use this number (rather than zero) to clear params. Because VIR_ALLOC* returns non-NULL pointer even if size is 0, the code would end up walking through random memory. If we were lucky enough and the memory contained 7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a random pointer and crash.
Let's make sure params stays NULL when nparams is 0.
Makes sense, ACK.
Pushed, thanks. Jirka
On 07/30/2012 12:03 PM, Jiri Denemark wrote:
On Mon, Jul 30, 2012 at 22:52:23 +0800, Osier Yang wrote:
On 2012年07月30日 19:55, Jiri Denemark wrote:
Daemon uses the following pattern when dispatching APIs with typed parameters:
VIR_ALLOC_N(params, nparams); virDomain*(dom, params,&nparams, flags); virTypedParameterArrayClear(params, nparams);
In case nparams was originally set to 0, virDomain* API would fill it with the number of typed parameters it can provide and we would use this number (rather than zero) to clear params. Because VIR_ALLOC* returns non-NULL pointer even if size is 0, the code would end up walking through random memory. If we were lucky enough and the memory contained 7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a random pointer and crash.
Let's make sure params stays NULL when nparams is 0.
Makes sense, ACK.
Pushed, thanks.
Per https://bugzilla.redhat.com/show_bug.cgi?id=844745, this has been assigned CVE-2012-3445. I'm therefore pushing backports of this patch to v0.9.6-maint and v0.9.11-maint, and we will be releasing new minor releases on the stable branches in the near future. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Jiri Denemark -
Osier Yang