6:55 a.m.
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
8:08 a.m.
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
9:29 a.m.
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
9:52 a.m.
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
1:03 p.m.
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
5:18 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4496
days inactive
4498
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
Jiri Denemark -
Osier Yang