[PATCH 0/2] netlink: Don't duplicate routes with mismatching interfaces

[PATCH 0/2] netlink: Don't duplicate routes with mismatching interfaces

[Stefano Brivio]

Stefano Brivio (2):
  netlink: Fix iterations over nexthop objects
  netlink: Don't duplicate routes referring to unrelated host interfaces

 netlink.c | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH 1/2] netlink: Fix iterations over nexthop objects

[Stefano Brivio]

Somewhat confusingly, RTNH_NEXT(), as defined by <linux/rtnetlink.h>,
doesn't take an attribute length parameter like RTA_NEXT() does, and
I just modelled loops over nexthops after RTA loops, forgetting to
decrease the remaining length we pass to RTNH_OK().

In practice, this didn't cause issue in any of the combinations I
checked, at least without the next patch.

We seem to be the only user of RTNH_OK(): even iproute2 has an
open-coded version of it in print_rta_multipath() (ip/iproute.c).

Introduce RTNH_NEXT_AND_DEC(), similar to RTA_NEXT(), and use it.

Fixes: 6c7623d07bbd ("netlink: Add support to fetch default gateway from multipath routes")
Fixes: f4e38b5cd232 ("netlink: Adjust interface index inside copied nexthop objects too")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 netlink.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/netlink.c b/netlink.c
index 89c0641..a5a4870 100644
--- a/netlink.c
+++ b/netlink.c
@@ -36,6 +36,10 @@
 #include "ip.h"
 #include "netlink.h"
 
+/* Same as RTA_NEXT() but for nexthops: RTNH_NEXT() doesn't take 'attrlen' */
+#define RTNH_NEXT_AND_DEC(rtnh, attrlen)				\
+	((attrlen) -= RTNH_ALIGN((rtnh)->rtnh_len), RTNH_NEXT(rtnh))
+
 /* Netlink expects a buffer of at least 8kiB or the system page size,
  * whichever is larger.  32kiB is recommended for more efficient.
  * Since the largest page size on any remotely common Linux setup is
@@ -349,12 +353,13 @@ unsigned int nl_get_ext_if(int s, sa_family_t af)
  */
 bool nl_route_get_def_multipath(struct rtattr *rta, void *gw)
 {
+	size_t nh_len = RTA_PAYLOAD(rta);
 	struct rtnexthop *rtnh;
 	bool found = false;
 	int hops = -1;
 
 	for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
-	     RTNH_OK(rtnh, RTA_PAYLOAD(rta)); rtnh = RTNH_NEXT(rtnh)) {
+	     RTNH_OK(rtnh, nh_len); rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
 		size_t len = rtnh->rtnh_len - sizeof(*rtnh);
 		struct rtattr *rta_inner;
 
@@ -566,11 +571,12 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
 			if (rta->rta_type == RTA_OIF) {
 				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
 			} else if (rta->rta_type == RTA_MULTIPATH) {
+				size_t nh_len = RTA_PAYLOAD(rta);
 				struct rtnexthop *rtnh;
 
 				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
-				     RTNH_OK(rtnh, RTA_PAYLOAD(rta));
-				     rtnh = RTNH_NEXT(rtnh))
+				     RTNH_OK(rtnh, nh_len);
+				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len))
 					rtnh->rtnh_ifindex = ifi_dst;
 			} else if (rta->rta_type == RTA_PREFSRC) {
 				/* Host routes might include a preferred source
-- 
@@ -36,6 +36,10 @@
 #include "ip.h"
 #include "netlink.h"
 
+/* Same as RTA_NEXT() but for nexthops: RTNH_NEXT() doesn't take 'attrlen' */
+#define RTNH_NEXT_AND_DEC(rtnh, attrlen)				\
+	((attrlen) -= RTNH_ALIGN((rtnh)->rtnh_len), RTNH_NEXT(rtnh))
+
 /* Netlink expects a buffer of at least 8kiB or the system page size,
  * whichever is larger.  32kiB is recommended for more efficient.
  * Since the largest page size on any remotely common Linux setup is
@@ -349,12 +353,13 @@ unsigned int nl_get_ext_if(int s, sa_family_t af)
  */
 bool nl_route_get_def_multipath(struct rtattr *rta, void *gw)
 {
+	size_t nh_len = RTA_PAYLOAD(rta);
 	struct rtnexthop *rtnh;
 	bool found = false;
 	int hops = -1;
 
 	for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
-	     RTNH_OK(rtnh, RTA_PAYLOAD(rta)); rtnh = RTNH_NEXT(rtnh)) {
+	     RTNH_OK(rtnh, nh_len); rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
 		size_t len = rtnh->rtnh_len - sizeof(*rtnh);
 		struct rtattr *rta_inner;
 
@@ -566,11 +571,12 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
 			if (rta->rta_type == RTA_OIF) {
 				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
 			} else if (rta->rta_type == RTA_MULTIPATH) {
+				size_t nh_len = RTA_PAYLOAD(rta);
 				struct rtnexthop *rtnh;
 
 				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
-				     RTNH_OK(rtnh, RTA_PAYLOAD(rta));
-				     rtnh = RTNH_NEXT(rtnh))
+				     RTNH_OK(rtnh, nh_len);
+				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len))
 					rtnh->rtnh_ifindex = ifi_dst;
 			} else if (rta->rta_type == RTA_PREFSRC) {
 				/* Host routes might include a preferred source
-- 
2.43.0

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

[Stefano Brivio]

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;
+		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)
 				continue;
 
 			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
-- 
@@ -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;
+		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)
 				continue;
 
 			rc = nl_do(s_dst, nh, RTM_NEWROUTE,
-- 
2.43.0

Re: [PATCH 1/2] netlink: Fix iterations over nexthop objects

[David Gibson]

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

On Tue, Apr 23, 2024 at 10:41:24PM +0200, Stefano Brivio wrote:
> Somewhat confusingly, RTNH_NEXT(), as defined by <linux/rtnetlink.h>,
> doesn't take an attribute length parameter like RTA_NEXT() does, and
> I just modelled loops over nexthops after RTA loops, forgetting to
> decrease the remaining length we pass to RTNH_OK().
> 
> In practice, this didn't cause issue in any of the combinations I
> checked, at least without the next patch.
> 
> We seem to be the only user of RTNH_OK(): even iproute2 has an
> open-coded version of it in print_rta_multipath() (ip/iproute.c).
> 
> Introduce RTNH_NEXT_AND_DEC(), similar to RTA_NEXT(), and use it.
> 
> Fixes: 6c7623d07bbd ("netlink: Add support to fetch default gateway from multipath routes")
> Fixes: f4e38b5cd232 ("netlink: Adjust interface index inside copied nexthop objects too")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
>  netlink.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index 89c0641..a5a4870 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -36,6 +36,10 @@
>  #include "ip.h"
>  #include "netlink.h"
>  
> +/* Same as RTA_NEXT() but for nexthops: RTNH_NEXT() doesn't take 'attrlen' */
> +#define RTNH_NEXT_AND_DEC(rtnh, attrlen)				\
> +	((attrlen) -= RTNH_ALIGN((rtnh)->rtnh_len), RTNH_NEXT(rtnh))
> +
>  /* Netlink expects a buffer of at least 8kiB or the system page size,
>   * whichever is larger.  32kiB is recommended for more efficient.
>   * Since the largest page size on any remotely common Linux setup is
> @@ -349,12 +353,13 @@ unsigned int nl_get_ext_if(int s, sa_family_t af)
>   */
>  bool nl_route_get_def_multipath(struct rtattr *rta, void *gw)
>  {
> +	size_t nh_len = RTA_PAYLOAD(rta);
>  	struct rtnexthop *rtnh;
>  	bool found = false;
>  	int hops = -1;
>  
>  	for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
> -	     RTNH_OK(rtnh, RTA_PAYLOAD(rta)); rtnh = RTNH_NEXT(rtnh)) {
> +	     RTNH_OK(rtnh, nh_len); rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len)) {
>  		size_t len = rtnh->rtnh_len - sizeof(*rtnh);
>  		struct rtattr *rta_inner;
>  
> @@ -566,11 +571,12 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  			if (rta->rta_type == RTA_OIF) {
>  				*(unsigned int *)RTA_DATA(rta) = ifi_dst;
>  			} else if (rta->rta_type == RTA_MULTIPATH) {
> +				size_t nh_len = RTA_PAYLOAD(rta);
>  				struct rtnexthop *rtnh;
>  
>  				for (rtnh = (struct rtnexthop *)RTA_DATA(rta);
> -				     RTNH_OK(rtnh, RTA_PAYLOAD(rta));
> -				     rtnh = RTNH_NEXT(rtnh))
> +				     RTNH_OK(rtnh, nh_len);
> +				     rtnh = RTNH_NEXT_AND_DEC(rtnh, nh_len))
>  					rtnh->rtnh_ifindex = ifi_dst;
>  			} else if (rta->rta_type == RTA_PREFSRC) {
>  				/* Host routes might include a preferred source

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

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

[David Gibson]

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