[libvirt] [PATCH] don't let a bogus packet trigger over-allocation and segfault
Another not-really-urgent fix:
From 4b56f03dee82657be3af5c79c826ae3091fbf522 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 3 Mar 2010 16:50:02 +0100 Subject: [PATCH] don't let a bogus packet trigger over-allocation and segfault
* src/xen/proxy_internal.c (xenProxyDomainDumpXML): An invalid packet could include a too-large "ans.len" value, which would make us allocate too much memory and then copy data from beyond the end of "ans", possibly evoking a segfault. Ensure that the value we use is no larger than the remaining portion of "ans". Also, change unnecessary memmove to memcpy (src and dest obviously do not overlap, so no need to use memmove). --- src/xen/proxy_internal.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index 8e1c226..bd234ec 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -978,26 +978,27 @@ xenProxyDomainDumpXML(virDomainPtr domain, int flags ATTRIBUTE_UNUSED) memset(&req, 0, sizeof(req)); req.command = VIR_PROXY_DOMAIN_XML; req.data.arg = domain->id; req.len = sizeof(req); ret = xenProxyCommand(domain->conn, &req, &ans, 0); if (ret < 0) { return(NULL); } - if (ans.len <= sizeof(virProxyPacket)) { + if (ans.len <= sizeof(virProxyPacket) + || ans.len > sizeof (ans) - sizeof(virProxyPacket)) { virProxyError(domain->conn, VIR_ERR_OPERATION_FAILED, __FUNCTION__); return (NULL); } xmllen = ans.len - sizeof(virProxyPacket); if (VIR_ALLOC_N(xml, xmllen+1) < 0) { virReportOOMError(); return NULL; } - memmove(xml, &ans.extra.dinfo, xmllen); + memcpy(xml, &ans.extra.dinfo, xmllen); xml[xmllen] = '\0'; return(xml); } /** * xenProxyDomainGetOSType: * @domain: a domain object -- 1.7.0.1.464.g0adc7
Jim Meyering wrote:
Another not-really-urgent fix: ... Subject: [PATCH] don't let a bogus packet trigger over-allocation and segfault
* src/xen/proxy_internal.c (xenProxyDomainDumpXML): An invalid packet could include a too-large "ans.len" value, which would make us allocate too much memory and then copy data from beyond the end of "ans", possibly evoking a segfault. Ensure that the value we use is no larger than the remaining portion of "ans". Also, change unnecessary memmove to memcpy (src and dest obviously do not overlap, so no need to use memmove).
Here's another. It is nearly identical, so I'll squash it onto the above.
From 3e89214bb9d4c42e683fb3fe2ff5a46a0988730f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 3 Mar 2010 17:20:33 +0100 Subject: [PATCH] xen: don't let bogus packets trigger over-allocation and segfault
* src/xen/proxy_internal.c (xenProxyDomainGetOSType): Likewise. --- src/xen/proxy_internal.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index bd234ec..8cb8896 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1034,22 +1034,23 @@ xenProxyDomainGetOSType(virDomainPtr domain) } if ((ans.len == sizeof(virProxyPacket)) && (ans.data.arg < 0)) { virRaiseError (domain->conn, NULL, NULL, VIR_FROM_REMOTE, VIR_ERR_OPERATION_FAILED, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, "%s", _("Cannot get domain details")); return(NULL); } - if (ans.len <= sizeof(virProxyPacket)) { + if (ans.len <= sizeof(virProxyPacket) + || ans.len > sizeof (ans) - sizeof(virProxyPacket)) { virProxyError(domain->conn, VIR_ERR_OPERATION_FAILED, __FUNCTION__); return (NULL); } oslen = ans.len - sizeof(virProxyPacket); if (VIR_ALLOC_N(ostype, oslen+1) < 0) { virReportOOMError(); return NULL; } - memmove(ostype, &ans.extra.dinfo, oslen); + memcpy(ostype, &ans.extra.dinfo, oslen); ostype[oslen] = '\0'; return(ostype); } -- 1.7.0.1.464.g0adc7
Jim Meyering wrote:
Jim Meyering wrote:
Another not-really-urgent fix: ... Subject: [PATCH] don't let a bogus packet trigger over-allocation and segfault
* src/xen/proxy_internal.c (xenProxyDomainDumpXML): An invalid packet could include a too-large "ans.len" value, which would make us allocate too much memory and then copy data from beyond the end of "ans", possibly evoking a segfault. Ensure that the value we use is no larger than the remaining portion of "ans". Also, change unnecessary memmove to memcpy (src and dest obviously do not overlap, so no need to use memmove).
Here's another. It is nearly identical, so I'll squash it onto the above.
And here's a third one from that file:
From 717e7129572cafb072dccd5c0a49940801a99f7b Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 3 Mar 2010 17:24:17 +0100 Subject: [PATCH] xen: don't let bogus packets trigger over-allocation and segfault
... (xenProxyGetCapabilities): Likewise. --- src/xen/proxy_internal.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index 8cb8896..be79d56 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -927,27 +927,28 @@ xenProxyGetCapabilities (virConnectPtr conn) req.data.arg = 0; req.len = sizeof(req); ret = xenProxyCommand(conn, &req, &ans, 0); if (ret < 0) { return NULL; } if (ans.data.arg == -1) return NULL; - if (ans.len <= sizeof(virProxyPacket)) { + if (ans.len <= sizeof(virProxyPacket) + || ans.len > sizeof (ans) - sizeof(virProxyPacket)) { virProxyError(conn, VIR_ERR_OPERATION_FAILED, __FUNCTION__); return NULL; } xmllen = ans.len - sizeof (virProxyPacket); if (VIR_ALLOC_N(xml, xmllen+1) < 0) { virReportOOMError(); return NULL; } - memmove (xml, ans.extra.str, xmllen); + memcpy (xml, ans.extra.str, xmllen); xml[xmllen] = '\0'; return xml; } /** * xenProxyDomainDumpXML: * @domain: a domain object -- 1.7.0.1.464.g0adc7
According to Jim Meyering on 3/3/2010 9:26 AM:
Another not-really-urgent fix:
Not sure whether this is 0.7.7 material to plug the crash, or whether it can wait. Either way,...
Subject: [PATCH] xen: don't let bogus packets trigger over-allocation and segfault
if (ans.data.arg == -1) return NULL; - if (ans.len <= sizeof(virProxyPacket)) { + if (ans.len <= sizeof(virProxyPacket) + || ans.len > sizeof (ans) - sizeof(virProxyPacket)) { virProxyError(conn, VIR_ERR_OPERATION_FAILED, __FUNCTION__); return NULL; }
xmllen = ans.len - sizeof (virProxyPacket); if (VIR_ALLOC_N(xml, xmllen+1) < 0) { virReportOOMError(); return NULL; } - memmove (xml, ans.extra.str, xmllen); + memcpy (xml, ans.extra.str, xmllen);
ACK to all three portions of the squashed patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Jim Meyering