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

[Laine Stump]

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 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.
> 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 at redhat.com>


...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.