From: Lennart Poettering <lennart@poettering.net>
Date: Thu, 4 Jan 2018 10:36:58 +0000 (+0100)
Subject: networkd: fix memory corruption
X-Git-Tag: v237~139^2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F7714%2Fhead;p=thirdparty%2Fsystemd.git

networkd: fix memory corruption

When loading .netdev files we parse them twice: first we do one parsing
iteration to figure out their "kind", and then we do it again to parse
out the kind's parameters. The first iteration is run with a "short"
NetDev structure, that only covers the generic NetDev properties. Which
should be enough, as we don't parse the per-kind properties. However,
before this patch we'd still try to destruct the per-kind properties
which resulted in memory corruption. With this change we distuingish the
two iterations by the state field, so that the destruction only happens
when the state signals we are running with a full NetDev structure.

Since this is not obvious, let's add a lot of comments.
---

diff --git a/src/network/netdev/netdev.c b/src/network/netdev/netdev.c
index 7cf110672f2..0bfe49006b9 100644
--- a/src/network/netdev/netdev.c
+++ b/src/network/netdev/netdev.c
@@ -153,7 +153,15 @@ static void netdev_free(NetDev *netdev) {
         condition_free_list(netdev->match_kernel_version);
         condition_free_list(netdev->match_arch);
 
-        if (NETDEV_VTABLE(netdev) &&
+        /* Invoke the per-kind done() destructor, but only if the state field is initialized. We conditionalize that
+         * because we parse .netdev files twice: once to determine the kind (with a short, minimal NetDev structure
+         * allocation, with no room for per-kind fields), and once to read the kind's properties (with a full,
+         * comprehensive NetDev structure allocation with enough space for whatever the specific kind needs). Now, in
+         * the first case we shouldn't try to destruct the per-kind NetDev fields on destruction, in the second case we
+         * should. We use the state field to discern the two cases: it's _NETDEV_STATE_INVALID on the first "raw"
+         * call. */
+        if (netdev->state != _NETDEV_STATE_INVALID &&
+            NETDEV_VTABLE(netdev) &&
             NETDEV_VTABLE(netdev)->done)
                 NETDEV_VTABLE(netdev)->done(netdev);
 
@@ -615,8 +623,8 @@ static int netdev_load_one(Manager *manager, const char *filename) {
         if (!file) {
                 if (errno == ENOENT)
                         return 0;
-                else
-                        return -errno;
+
+                return -errno;
         }
 
         if (null_or_empty_fd(fileno(file))) {
@@ -630,7 +638,7 @@ static int netdev_load_one(Manager *manager, const char *filename) {
 
         netdev_raw->n_ref = 1;
         netdev_raw->kind = _NETDEV_KIND_INVALID;
-        netdev_raw->state = _NETDEV_STATE_INVALID;
+        netdev_raw->state = _NETDEV_STATE_INVALID; /* an invalid state means done() of the implementation won't be called on destruction */
 
         dropin_dirname = strjoina(basename(filename), ".d");
         r = config_parse_many(filename, network_dirs, dropin_dirname,
@@ -669,7 +677,7 @@ static int netdev_load_one(Manager *manager, const char *filename) {
         netdev->n_ref = 1;
         netdev->manager = manager;
         netdev->kind = netdev_raw->kind;
-        netdev->state = _NETDEV_STATE_INVALID;
+        netdev->state = NETDEV_STATE_LOADING; /* we initialize the state here for the first time, so that done() will be called on destruction */
 
         if (NETDEV_VTABLE(netdev)->init)
                 NETDEV_VTABLE(netdev)->init(netdev);
diff --git a/src/network/netdev/netdev.h b/src/network/netdev/netdev.h
index 507b86a078f..7ae064ba8f9 100644
--- a/src/network/netdev/netdev.h
+++ b/src/network/netdev/netdev.h
@@ -65,6 +65,7 @@ typedef enum NetDevKind {
 } NetDevKind;
 
 typedef enum NetDevState {
+        NETDEV_STATE_LOADING,
         NETDEV_STATE_FAILED,
         NETDEV_STATE_CREATING,
         NETDEV_STATE_READY,
@@ -149,7 +150,9 @@ extern const NetDevVTable * const netdev_vtable[_NETDEV_KIND_MAX];
 /* For casting a netdev into the various netdev kinds */
 #define DEFINE_NETDEV_CAST(UPPERCASE, MixedCase)                            \
         static inline MixedCase* UPPERCASE(NetDev *n) {                     \
-                if (_unlikely_(!n || n->kind != NETDEV_KIND_##UPPERCASE))   \
+                if (_unlikely_(!n ||                                        \
+                               n->kind != NETDEV_KIND_##UPPERCASE) ||       \
+                               n->state == _NETDEV_STATE_INVALID)           \
                         return NULL;                                        \
                                                                             \
                 return (MixedCase*) n;                                      \