[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] util: check accept_ra for all nexthop interfaces of multipath routes



On 1/10/19 10:44 AM, Erik Skultety wrote:
On Wed, Jan 09, 2019 at 12:43:15PM -0500, Laine Stump wrote:
When checking the setting of accept_ra, we have assumed that all
routes have a single nexthop, so the interface of the route would be
in the RTA_OIF attribute of the netlink RTM_NEWROUTE message. But
multipath routes don't have an RTA_OIF; instead, they have an
RTA_MULTIPATH attribute, which is an array of rtnexthop, with each
rtnexthop having an interface. This patch adds a loop to look at the
setting of accept_ra of the interface for every rtnexthop in the
array.

Signed-off-by: Laine Stump <laine laine org>
---
  src/util/virnetdevip.c | 37 +++++++++++++++++++++++++++++++++++++
  1 file changed, 37 insertions(+)

diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index c032ecacfc..e0965bcc1d 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -566,6 +566,43 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
          return 0;
      }

+    /* if no RTA_OIF was found, see if this is a multipath route (one
+     * which has an array of nexthops, each with its own interface)
+     */
+
+    rta_attr = (struct rtattr *)nlmsg_find_attr(resp, sizeof(struct rtmsg), RTA_MULTIPATH);
+    if (rta_attr) {
+        /* The data of the attribute is an array of rtnexthop */
+        struct rtnexthop *nh = RTA_DATA(rta_attr);
+        size_t len = RTA_PAYLOAD(rta_attr);
+
+        /* validate the attribute array length */
+        len = MIN(len, ((char *)resp + NLMSG_PAYLOAD(resp, 0) - (char *)rta_attr));
I was fine until                      ...                              ^here,
you lost me there, can you elaborate on that witchcraft?


The original "len" is the length of the RTA_MULTIPATH as given in the attribute object. If the message has been truncated somehow, using this value without validating it could lead to us trying to interpret garbage as if it were actual date.


The left side of the MIN is giving us the length from the start of the attribute (rta_attr) to the end of the actual message ("resp" is the start of the message, and "NLMSG_PAYLOAD(resp, 0)" is the length of that message).


So, it's just limiting len to be within the bounds of what we actually read from netlink, rather than trusting the length that was advertised by the attribute itself.



+
+        while (len >= sizeof(*nh) && len >= nh->rtnh_len) {
+            /* check accept_ra for the interface of each nexthop */
+
+            ifname = virNetDevGetName(nh->rtnh_ifindex);
+
+            if (ifname)
+                accept_ra = virNetDevIPGetAcceptRA(ifname);
+
+            VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra: %d",
+                      ifname, nh->rtnh_ifindex, accept_ra);
+
+            if (!ifname ||
+                (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)) {
+                return -1;
+            }
+
+            VIR_FREE(ifname); /* in case it wasn't added to the array */
                                 I'd drop ^this commentary, otherwise it just
leaves the reader wondering what happens with free() in the other case, IOW
VIR_APPEND_ELEMENT happens :)


Yeah, I suppose it should either say more (so that it's more readily obvious that virNetDevIPCheckIPv6ForwardingAddIF could consume the string) or say less (so you don't spend time wondering about it). I can remove the comment.



Erik

+            data->hasRARoutes = true;
+
+            len -= NLMSG_ALIGN(nh->rtnh_len);
+            nh = RTNH_NEXT(nh);
+        }
+    }
+



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]