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