public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH v2] netlink: Don't duplicate routes referring to unrelated host interfaces
@ 2024-05-02  6:17 Stefano Brivio
  2024-05-06  7:22 ` David Gibson
  0 siblings, 1 reply; 2+ messages in thread
From: Stefano Brivio @ 2024-05-02  6:17 UTC (permalink / raw)
  To: passt-dev; +Cc: runsisi, David Gibson

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>
---
v2: Set NLMSG_NOOP on the message instead of AF_UNSPEC on the route
    to discard it, to keep the check on insertion simple.

 netlink.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/netlink.c b/netlink.c
index cdfe68c..b1c0cce 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)
+			nh->nlmsg_type = NLMSG_NOOP;
+		else
+			dup_routes++;
 	}
 
 	if (!NLMSG_OK(nh, left)) {
-- 
@@ -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)
+			nh->nlmsg_type = NLMSG_NOOP;
+		else
+			dup_routes++;
 	}
 
 	if (!NLMSG_OK(nh, left)) {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] netlink: Don't duplicate routes referring to unrelated host interfaces
  2024-05-02  6:17 [PATCH v2] netlink: Don't duplicate routes referring to unrelated host interfaces Stefano Brivio
@ 2024-05-06  7:22 ` David Gibson
  0 siblings, 0 replies; 2+ messages in thread
From: David Gibson @ 2024-05-06  7:22 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev, runsisi

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

On Thu, May 02, 2024 at 08:17:16AM +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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v2: Set NLMSG_NOOP on the message instead of AF_UNSPEC on the route
>     to discard it, to keep the check on insertion simple.
> 
>  netlink.c | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index cdfe68c..b1c0cce 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)
> +			nh->nlmsg_type = NLMSG_NOOP;
> +		else
> +			dup_routes++;
>  	}
>  
>  	if (!NLMSG_OK(nh, left)) {

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2024-05-06  7:22 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-02  6:17 [PATCH v2] netlink: Don't duplicate routes referring to unrelated host interfaces Stefano Brivio
2024-05-06  7:22 ` David Gibson

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).