render / rpms / libvirt

Forked from rpms/libvirt 9 months ago
Clone
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