[systemd-devel] [PATCH] rtnl: fix memory corruptions after realloc

[Zbigniew Jędrzejewski-Szmek]

struct sd_rtnl_message would keep two additional pointers into the hdr
field. Every time hdr was realloced, those pointers should be adjusted,
but weren't. It seems less error-prone to keep offsets instead.
---
Hi Tom,
this fixes errors reported by valgrind and should make the code more
robust, imo.

Zbyszek

 src/libsystemd-rtnl/rtnl-message.c | 54 ++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/src/libsystemd-rtnl/rtnl-message.c b/src/libsystemd-rtnl/rtnl-message.c
index 24f2e6f..f2ca98c 100644
--- a/src/libsystemd-rtnl/rtnl-message.c
+++ b/src/libsystemd-rtnl/rtnl-message.c
@@ -35,14 +35,14 @@ struct sd_rtnl_message {
         RefCount n_ref;
 
         struct nlmsghdr *hdr;
-
-        struct rtattr *current_container;
-
+        size_t container_offset;
         struct rtattr *next_rta;
 
         bool sealed:1;
 };
 
+#define CURRENT_CONTAINER(m) ((m)->container_offset ? (struct rtattr*)((uint8_t*)(m)->hdr + (m)->container_offset) : NULL)
+
 static int message_new(sd_rtnl_message **ret, size_t initial_size) {
         sd_rtnl_message *m;
 
@@ -296,8 +296,8 @@ int sd_rtnl_message_link_get_flags(sd_rtnl_message *m, unsigned *flags) {
         return 0;
 }
 
-/* If successful the updated message will be correctly aligned, if unsuccessful the old message is
-   untouched */
+/* If successful the updated message will be correctly aligned, if
+   unsuccessful the old message is untouched. */
 static int add_rtattr(sd_rtnl_message *m, unsigned short type, const void *data, size_t data_length) {
         uint32_t rta_length, message_length;
         struct nlmsghdr *new_hdr;
@@ -311,48 +311,46 @@ static int add_rtattr(sd_rtnl_message *m, unsigned short type, const void *data,
 
         /* get the size of the new rta attribute (with padding at the end) */
         rta_length = RTA_LENGTH(data_length);
-        /* get the new message size (with padding at the end)
-         */
+
+        /* get the new message size (with padding at the end) */
         message_length = m->hdr->nlmsg_len + RTA_ALIGN(rta_length);
 
         /* realloc to fit the new attribute */
         new_hdr = realloc(m->hdr, message_length);
         if (!new_hdr)
                 return -ENOMEM;
-        /* update the location of the next rta for reading */
-        m->next_rta = (struct rtattr *) ((uint8_t *) m->next_rta +
-                                         ((uint8_t *) new_hdr -
-                                          (uint8_t *) m->hdr));
+
+        /* update the location of the next rta */
+        m->next_rta = (struct rtattr *) ((uint8_t *) m->next_rta - (uint8_t *) m->hdr
+                                         + (uint8_t *) new_hdr);
         m->hdr = new_hdr;
 
         /* get pointer to the attribute we are about to add */
         rta = (struct rtattr *) ((uint8_t *) m->hdr + m->hdr->nlmsg_len);
-        /* update message size */
-        m->hdr->nlmsg_len = message_length;
 
-        /* we are inside a container, extend it */
-        if (m->current_container)
-                m->current_container->rta_len = (uint8_t *) m->hdr +
-                                                m->hdr->nlmsg_len -
-                                                (uint8_t *) m->current_container;
+        /* if we are inside a container, extend it */
+        if (CURRENT_CONTAINER(m))
+                CURRENT_CONTAINER(m)->rta_len += message_length - m->hdr->nlmsg_len;
 
         /* fill in the attribute */
         rta->rta_type = type;
         rta->rta_len = rta_length;
         if (!data) {
-                /* this is a container, set pointer */
-                m->current_container = rta;
+                /* this is the start of a new container */
+                m->container_offset = m->hdr->nlmsg_len;
         } else {
                 /* we don't deal with the case where the user lies about the type
                  * and gives us too little data (so don't do that)
                 */
                 padding = mempcpy(RTA_DATA(rta), data, data_length);
                 /* make sure also the padding at the end of the message is initialized */
-                memset(padding, '\0', (uint8_t *) m->hdr +
-                                      m->hdr->nlmsg_len -
-                                      (uint8_t *) padding);
+                memzero(padding,
+                        (uint8_t *) m->hdr + message_length - (uint8_t *) padding);
         }
 
+        /* update message size */
+        m->hdr->nlmsg_len = message_length;
+
         return 0;
 }
 
@@ -373,8 +371,8 @@ int sd_rtnl_message_append_string(sd_rtnl_message *m, unsigned short type, const
                 case RTM_SETLINK:
                 case RTM_GETLINK:
                 case RTM_DELLINK:
-                        if (m->current_container) {
-                                if (m->current_container->rta_type != IFLA_LINKINFO ||
+                        if (CURRENT_CONTAINER(m)) {
+                                if (CURRENT_CONTAINER(m)->rta_type != IFLA_LINKINFO ||
                                     type != IFLA_INFO_KIND)
                                         return -ENOTSUP;
                         } else {
@@ -612,7 +610,7 @@ int sd_rtnl_message_open_container(sd_rtnl_message *m, unsigned short type) {
         uint16_t rtm_type;
 
         assert_return(m, -EINVAL);
-        assert_return(!m->current_container, -EINVAL);
+        assert_return(!CURRENT_CONTAINER(m), -EINVAL);
 
         sd_rtnl_message_get_type(m, &rtm_type);
 
@@ -629,9 +627,9 @@ int sd_rtnl_message_open_container(sd_rtnl_message *m, unsigned short type) {
 
 int sd_rtnl_message_close_container(sd_rtnl_message *m) {
         assert_return(m, -EINVAL);
-        assert_return(m->current_container, -EINVAL);
+        assert_return(CURRENT_CONTAINER(m), -EINVAL);
 
-        m->current_container = NULL;
+        m->container_offset = 0;
 
         return 0;
 }
-- 
1.8.4.2