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

[Laine Stump]

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 at 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);
>> +        }
>> +    }
>> +