On 1/11/19 2:11 AM, Erik Skultety wrote:
On Thu, Jan 10, 2019 at 11:48:42AM -0500, Laine Stump wrote:
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@xxxxxxxxx>
---
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.
So, do we really want to continue processing the data, if the message might
have been truncated?
Yes. In the very low chance case that the message was truncated (because
there were tons and tons of routes, and the entire list was too long for
some internal buffer limit somewhere), I don't think we want to just
throw our hands up and say "Okay, anything goes!" (or "%*(#( it,
*nothing* goes!). We should just check for all the interfaces we can
verifiably say do have RA routes associated with them. It's all really
just a best effort check anyway (actually at least one person has asked
that there be a way to disable it, since there is at least one case when
it can give false-positives that make it impossible to start an IPv6
network, but that's a separate issue).
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).
Understood, thanks.
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.
The reason why it caught my eye was very simple, I don't remember any places in
libvirt where we'd put such a comment with an identical scenario, so that's why
I stopped and thought about it for a second.
You have my RB if it's safe and okay for us to continue processing the data of
an attribute if the message might be incomplete.
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
...anndddd I pushed it without making the same change you suggested in
patch 3/4 (returning immediately if virNetDevGetName() failed) (I forgot
I had the same logic in this patch until I noticed it while backporting
to an earlier branch :-(). I'll send a patch to correct that now.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
