|
|
9119d9 |
From 1edb0576352ab667f15120a50b85348f972445c8 Mon Sep 17 00:00:00 2001
|
|
|
9119d9 |
Message-Id: <1edb0576352ab667f15120a50b85348f972445c8@dist-git>
|
|
|
9119d9 |
From: Laine Stump <laine@laine.org>
|
|
|
9119d9 |
Date: Wed, 12 Nov 2014 14:00:43 -0500
|
|
|
9119d9 |
Subject: [PATCH] util: eliminate "use after free" in callers of
|
|
|
9119d9 |
virNetDevLinkDump
|
|
|
9119d9 |
|
|
|
9119d9 |
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1163463
|
|
|
9119d9 |
|
|
|
9119d9 |
virNetDevLinkDump() gets a message from netlink into "resp", then
|
|
|
9119d9 |
calls nlmsg_parse() to fill the table "tb" with pointers into resp. It
|
|
|
9119d9 |
then returns tb to its caller, but not before freeing the buffer at
|
|
|
9119d9 |
resp. That means that all the callers of virNetDevLinkDump() are
|
|
|
9119d9 |
examining memory that has already been freed. This can be verified by
|
|
|
9119d9 |
filling the buffer at resp with garbage prior to freeing it (or, I
|
|
|
9119d9 |
suppose, just running libvirtd under valgrind) then performing some
|
|
|
9119d9 |
operation that calls virNetDevLinkDump().
|
|
|
9119d9 |
|
|
|
9119d9 |
The upstream commit log incorrectly states that the code has been like
|
|
|
9119d9 |
this ever since virNetDevLinkDump() was written. In reality, the
|
|
|
9119d9 |
problem was introduced with commit e95de74d, first in libvirt-1.0.5,
|
|
|
9119d9 |
which was attempting to eliminate a typecast that caused compiler
|
|
|
9119d9 |
warnings. It has only been pure luck (or maybe a lack of heavy load,
|
|
|
9119d9 |
and/or maybe an allocation algorithm in malloc() that delays re-use of
|
|
|
9119d9 |
just-freed memory) that has kept this from causing errors, for example
|
|
|
9119d9 |
when configuring a PCI passthrough or macvtap passthrough network
|
|
|
9119d9 |
interface.
|
|
|
9119d9 |
|
|
|
9119d9 |
The solution taken in this patch is the simplest - just return resp to
|
|
|
9119d9 |
the caller along with tb, then have the caller free it after they are
|
|
|
9119d9 |
finished using the data (pointers) in tb. I alternately could have
|
|
|
9119d9 |
made a cleaner interface by creating a new struct that put tb and resp
|
|
|
9119d9 |
together along with a vir*Free() function for it, but this function is
|
|
|
9119d9 |
only used in a couple places, and I'm not sure there will be
|
|
|
9119d9 |
additional new uses of virNetDevLinkDump(), so the value of adding a
|
|
|
9119d9 |
new type, extra APIs, etc. is dubious.
|
|
|
9119d9 |
|
|
|
9119d9 |
(cherry picked from commit f9f9699f40729556238b905f67a7d6f68c084f6a)
|
|
|
9119d9 |
|
|
|
9119d9 |
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
|
|
|
9119d9 |
---
|
|
|
9119d9 |
src/util/virnetdev.c | 26 +++++++++++++++++---------
|
|
|
9119d9 |
src/util/virnetdev.h | 2 +-
|
|
|
9119d9 |
src/util/virnetdevvportprofile.c | 17 ++++++++++++-----
|
|
|
9119d9 |
3 files changed, 30 insertions(+), 15 deletions(-)
|
|
|
9119d9 |
|
|
|
9119d9 |
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
|
|
|
9119d9 |
index 0382d00..a1e442a 100644
|
|
|
9119d9 |
--- a/src/util/virnetdev.c
|
|
|
9119d9 |
+++ b/src/util/virnetdev.c
|
|
|
9119d9 |
@@ -1387,23 +1387,25 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
|
|
|
9119d9 |
/**
|
|
|
9119d9 |
* virNetDevLinkDump:
|
|
|
9119d9 |
*
|
|
|
9119d9 |
- * @ifname: The name of the interface; only use if ifindex < 0
|
|
|
9119d9 |
- * @ifindex: The interface index; may be < 0 if ifname is given
|
|
|
9119d9 |
- * @nlattr: pointer to a pointer of netlink attributes that will contain
|
|
|
9119d9 |
- * the results
|
|
|
9119d9 |
+ * @ifname: The name of the interface; only use if ifindex <= 0
|
|
|
9119d9 |
+ * @ifindex: The interface index; may be <= 0 if ifname is given
|
|
|
9119d9 |
+ * @data: Gets a pointer to the raw data from netlink.
|
|
|
9119d9 |
+ MUST BE FREED BY CALLER!
|
|
|
9119d9 |
+ * @nlattr: Pointer to a pointer of netlink attributes that will contain
|
|
|
9119d9 |
+ * the results
|
|
|
9119d9 |
* @src_pid: pid used for nl_pid of the local end of the netlink message
|
|
|
9119d9 |
* (0 == "use getpid()")
|
|
|
9119d9 |
* @dst_pid: pid of destination nl_pid if the kernel
|
|
|
9119d9 |
* is not the target of the netlink message but it is to be
|
|
|
9119d9 |
* sent to another process (0 if sending to the kernel)
|
|
|
9119d9 |
*
|
|
|
9119d9 |
- * Get information about an interface given its name or index.
|
|
|
9119d9 |
+ * Get information from netlink about an interface given its name or index.
|
|
|
9119d9 |
*
|
|
|
9119d9 |
* Returns 0 on success, -1 on fatal error.
|
|
|
9119d9 |
*/
|
|
|
9119d9 |
int
|
|
|
9119d9 |
virNetDevLinkDump(const char *ifname, int ifindex,
|
|
|
9119d9 |
- struct nlattr **tb,
|
|
|
9119d9 |
+ void **nlData, struct nlattr **tb,
|
|
|
9119d9 |
uint32_t src_pid, uint32_t dst_pid)
|
|
|
9119d9 |
{
|
|
|
9119d9 |
int rc = -1;
|
|
|
9119d9 |
@@ -1485,7 +1487,9 @@ virNetDevLinkDump(const char *ifname, int ifindex,
|
|
|
9119d9 |
rc = 0;
|
|
|
9119d9 |
cleanup:
|
|
|
9119d9 |
nlmsg_free(nl_msg);
|
|
|
9119d9 |
- VIR_FREE(resp);
|
|
|
9119d9 |
+ if (rc < 0)
|
|
|
9119d9 |
+ VIR_FREE(resp);
|
|
|
9119d9 |
+ *nlData = resp;
|
|
|
9119d9 |
return rc;
|
|
|
9119d9 |
|
|
|
9119d9 |
malformed_resp:
|
|
|
9119d9 |
@@ -1681,15 +1685,18 @@ virNetDevGetVfConfig(const char *ifname, int vf, virMacAddrPtr mac,
|
|
|
9119d9 |
int *vlanid)
|
|
|
9119d9 |
{
|
|
|
9119d9 |
int rc = -1;
|
|
|
9119d9 |
+ void *nlData = NULL;
|
|
|
9119d9 |
struct nlattr *tb[IFLA_MAX + 1] = {NULL, };
|
|
|
9119d9 |
int ifindex = -1;
|
|
|
9119d9 |
|
|
|
9119d9 |
- rc = virNetDevLinkDump(ifname, ifindex, tb, 0, 0);
|
|
|
9119d9 |
+ rc = virNetDevLinkDump(ifname, ifindex, &nlData, tb, 0, 0);
|
|
|
9119d9 |
if (rc < 0)
|
|
|
9119d9 |
- return rc;
|
|
|
9119d9 |
+ goto cleanup;
|
|
|
9119d9 |
|
|
|
9119d9 |
rc = virNetDevParseVfConfig(tb, vf, mac, vlanid);
|
|
|
9119d9 |
|
|
|
9119d9 |
+ cleanup:
|
|
|
9119d9 |
+ VIR_FREE(nlData);
|
|
|
9119d9 |
return rc;
|
|
|
9119d9 |
}
|
|
|
9119d9 |
|
|
|
9119d9 |
@@ -1832,6 +1839,7 @@ virNetDevRestoreNetConfig(const char *linkdev, int vf, const char *stateDir)
|
|
|
9119d9 |
int
|
|
|
9119d9 |
virNetDevLinkDump(const char *ifname ATTRIBUTE_UNUSED,
|
|
|
9119d9 |
int ifindex ATTRIBUTE_UNUSED,
|
|
|
9119d9 |
+ void **nlData ATTRIBUTE_UNUSED,
|
|
|
9119d9 |
struct nlattr **tb ATTRIBUTE_UNUSED,
|
|
|
9119d9 |
uint32_t src_pid ATTRIBUTE_UNUSED,
|
|
|
9119d9 |
uint32_t dst_pid ATTRIBUTE_UNUSED)
|
|
|
9119d9 |
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
|
|
|
9119d9 |
index ac4beff..2791b0d 100644
|
|
|
9119d9 |
--- a/src/util/virnetdev.h
|
|
|
9119d9 |
+++ b/src/util/virnetdev.h
|
|
|
9119d9 |
@@ -166,7 +166,7 @@ int virNetDevGetVirtualFunctions(const char *pfname,
|
|
|
9119d9 |
ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
|
|
|
9119d9 |
|
|
|
9119d9 |
int virNetDevLinkDump(const char *ifname, int ifindex,
|
|
|
9119d9 |
- struct nlattr **tb,
|
|
|
9119d9 |
+ void **nlData, struct nlattr **tb,
|
|
|
9119d9 |
uint32_t src_pid, uint32_t dst_pid)
|
|
|
9119d9 |
ATTRIBUTE_RETURN_CHECK;
|
|
|
9119d9 |
|
|
|
9119d9 |
diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c
|
|
|
9119d9 |
index 0f43b02..33ae9fc 100644
|
|
|
9119d9 |
--- a/src/util/virnetdevvportprofile.c
|
|
|
9119d9 |
+++ b/src/util/virnetdevvportprofile.c
|
|
|
9119d9 |
@@ -787,7 +787,8 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int
|
|
|
9119d9 |
int *parent_ifindex, char *parent_ifname,
|
|
|
9119d9 |
unsigned int *nth)
|
|
|
9119d9 |
{
|
|
|
9119d9 |
- int rc;
|
|
|
9119d9 |
+ int rc = -1;
|
|
|
9119d9 |
+ void *nlData = NULL;
|
|
|
9119d9 |
struct nlattr *tb[IFLA_MAX + 1] = { NULL, };
|
|
|
9119d9 |
bool end = false;
|
|
|
9119d9 |
size_t i = 0;
|
|
|
9119d9 |
@@ -798,7 +799,8 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int
|
|
|
9119d9 |
return -1;
|
|
|
9119d9 |
|
|
|
9119d9 |
while (!end && i <= nthParent) {
|
|
|
9119d9 |
- rc = virNetDevLinkDump(ifname, ifindex, tb, 0, 0);
|
|
|
9119d9 |
+ VIR_FREE(nlData);
|
|
|
9119d9 |
+ rc = virNetDevLinkDump(ifname, ifindex, &nlData, tb, 0, 0);
|
|
|
9119d9 |
if (rc < 0)
|
|
|
9119d9 |
break;
|
|
|
9119d9 |
|
|
|
9119d9 |
@@ -807,7 +809,8 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int
|
|
|
9119d9 |
IFNAMSIZ)) {
|
|
|
9119d9 |
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
|
|
|
9119d9 |
_("buffer for root interface name is too small"));
|
|
|
9119d9 |
- return -1;
|
|
|
9119d9 |
+ rc = -1;
|
|
|
9119d9 |
+ goto cleanup;
|
|
|
9119d9 |
}
|
|
|
9119d9 |
*parent_ifindex = ifindex;
|
|
|
9119d9 |
}
|
|
|
9119d9 |
@@ -823,6 +826,8 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int
|
|
|
9119d9 |
|
|
|
9119d9 |
*nth = i - 1;
|
|
|
9119d9 |
|
|
|
9119d9 |
+ cleanup:
|
|
|
9119d9 |
+ VIR_FREE(nlData);
|
|
|
9119d9 |
return rc;
|
|
|
9119d9 |
}
|
|
|
9119d9 |
|
|
|
9119d9 |
@@ -844,6 +849,7 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
|
|
|
9119d9 |
int rc;
|
|
|
9119d9 |
int src_pid = 0;
|
|
|
9119d9 |
uint32_t dst_pid = 0;
|
|
|
9119d9 |
+ void *nlData = NULL;
|
|
|
9119d9 |
struct nlattr *tb[IFLA_MAX + 1] = { NULL, };
|
|
|
9119d9 |
int repeats = STATUS_POLL_TIMEOUT_USEC / STATUS_POLL_INTERVL_USEC;
|
|
|
9119d9 |
uint16_t status = 0;
|
|
|
9119d9 |
@@ -876,7 +882,8 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
|
|
|
9119d9 |
}
|
|
|
9119d9 |
|
|
|
9119d9 |
while (--repeats >= 0) {
|
|
|
9119d9 |
- rc = virNetDevLinkDump(NULL, ifindex, tb, src_pid, dst_pid);
|
|
|
9119d9 |
+ VIR_FREE(nlData);
|
|
|
9119d9 |
+ rc = virNetDevLinkDump(NULL, ifindex, &nlData, tb, src_pid, dst_pid);
|
|
|
9119d9 |
if (rc < 0)
|
|
|
9119d9 |
goto cleanup;
|
|
|
9119d9 |
|
|
|
9119d9 |
@@ -908,7 +915,7 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex,
|
|
|
9119d9 |
}
|
|
|
9119d9 |
|
|
|
9119d9 |
cleanup:
|
|
|
9119d9 |
-
|
|
|
9119d9 |
+ VIR_FREE(nlData);
|
|
|
9119d9 |
return rc;
|
|
|
9119d9 |
}
|
|
|
9119d9 |
|
|
|
9119d9 |
--
|
|
|
9119d9 |
2.1.3
|
|
|
9119d9 |
|