[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 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 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?

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


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