Re: [PATCH 2/2] netlink: Don't duplicate routes referring to unrelated host interfaces

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4537 bytes --]

On Tue, Apr 23, 2024 at 10:41:25PM +0200, Stefano Brivio wrote:
> We take care of this in nl_addr_dup(): if the interface index
> associated to an address doesn't match the selected host interface
> (ifa->ifa_index != ifi_src), we don't copy that address.
> 
> But for routes, we just unconditionally update the interface index to
> match the index in the target namespace, even if the source interface
> didn't match.
> 
> This might happen in two cases: with a pre-4.20 kernel without support
> for NETLINK_GET_STRICT_CHK, which won't filter routes based on the
> interface we pass in the request, as reported by runsisi, and any
> kernel with support for multipath routes where any of the nexthops
> refers to an unrelated host interface.
> 
> In both cases, check the index of the source interface, and avoid
> copying unrelated routes.
> 
> Reported-by: runsisi <runsisi@hust.edu.cn>
> Link: https://bugs.passt.top/show_bug.cgi?id=86
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  netlink.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index a5a4870..e8325c7 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -554,21 +554,32 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  	     NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0;
>  	     nh = NLMSG_NEXT(nh, left)) {
>  		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
> +		bool discard = false;
>  		struct rtattr *rta;
>  		size_t na;
>  
>  		if (nh->nlmsg_type != RTM_NEWROUTE)
>  			continue;
>  
> -		dup_routes++;
> -
>  		for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
>  		     rta = RTA_NEXT(rta, na)) {
>  			/* RTA_OIF and RTA_MULTIPATH attributes carry the
> -			 * identifier of a host interface. Change them to match
> -			 * the corresponding identifier in the target namespace.
> -			 */
> +			 * identifier of a host interface. If they match the
> +			 * host interface we're copying from, change them to
> +			 * match the corresponding identifier in the target
> +			 * namespace.
> +			 *
> +			 * If RTA_OIF doesn't match (NETLINK_GET_STRICT_CHK not
> +			 * available), or if any interface index in nexthop
> +			 * objects differ from the host interface, discard the
> +			 * route altogether.
> +			*/
>  			if (rta->rta_type == RTA_OIF) {
> +				if (*(unsigned int *)RTA_DATA(rta) != ifi_src) {
> +					discard = true;
> +					break;
> +				}
> +
>  				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
>  			} else if (rta->rta_type == RTA_MULTIPATH) {
>  				size_t nh_len = RTA_PAYLOAD(rta);
> @@ -576,8 +587,19 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  
>  				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
>  				     RTNH_OK(rtnh, nh_len);
> -				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len))
> +				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
> +					int src = (int)ifi_src;
> +
> +					if (rtnh->rtnh_ifindex != src) {
> +						discard = true;
> +						break;
> +					}
> +
>  					rtnh->rtnh_ifindex = ifi_dst;
> +				}
> +
> +				if (discard)
> +					break;
>  			} else if (rta->rta_type == RTA_PREFSRC) {
>  				/* Host routes might include a preferred source
>  				 * address, which must be one of the host's
> @@ -588,6 +610,11 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  				rta->rta_type = RTA_UNSPEC;
>  			}
>  		}
> +
> +		if (discard)
> +			rtm->rtm_family = AF_UNSPEC;

Sorry, I misremembered by constants.  Rather than using AF_UNSPEC, I
was thinking you could change nh->nlmsg_type to NLMSG_NOOP, that way..


> +		else
> +			dup_routes++;
>  	}
>  
>  	if (!NLMSG_OK(nh, left)) {
> @@ -619,10 +646,12 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  		for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size;
>  		     NLMSG_OK(nh, left);
>  		     nh = NLMSG_NEXT(nh, left)) {
> +			struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
>  			uint16_t flags = nh->nlmsg_flags;
>  			int rc;
>  
> -			if (nh->nlmsg_type != RTM_NEWROUTE)
> +			if (nh->nlmsg_type != RTM_NEWROUTE ||
> +			    rtm->rtm_family == AF_UNSPEC)

.. you don't need to update the condition here.

>  				continue;
>  
>  			rc = nl_do(s_dst, nh, RTM_NEWROUTE,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]