[PATCH v2] netfilter: xt_multiport: reject trailing range markers

[PATCH v2] netfilter: xt_multiport: reject trailing range markers

[Ren Wei]

ports_match_v1() treats any non-zero pflags entry as the start of a
port range and unconditionally consumes the next ports[] element as
the range end.

The checkentry path currently validates protocol, flags and count, but
it does not reject a malformed rule whose final slot is marked as a
range start. That leaves ports_match_v1() to step past the last valid
ports[] element when such a rule is installed.

Fix this by rejecting multiport v1 rules with a trailing range marker
in checkentry. This keeps malformed rules out of the runtime matching
path and preserves the existing exact-match and valid-range behavior.

Fixes: a89ecb6a2ef7 ("[NETFILTER]: x_tables: unify IPv4/IPv6 multiport match")
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Yuhang Zheng <z1652074432@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
Changes in v2:
- drop the selftest patch
- send the fix publicly to netfilter-devel
 net/netfilter/xt_multiport.c | 50 +++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
index 44a00f5acde8..38aa5b90d38e 100644
--- a/net/netfilter/xt_multiport.c
+++ b/net/netfilter/xt_multiport.c
@@ -26,10 +26,10 @@ MODULE_ALIAS("ip6t_multiport");
 /* Returns 1 if the port is matched by the test, 0 otherwise. */
 static inline bool
 ports_match_v1(const struct xt_multiport_v1 *minfo,
-	       u_int16_t src, u_int16_t dst)
+	       u16 src, u16 dst)
 {
 	unsigned int i;
-	u_int16_t s, e;
+	u16 s, e;
 
 	for (i = 0; i < minfo->count; i++) {
 		s = minfo->ports[i];
@@ -106,20 +106,36 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
 }
 
 static inline bool
-check(u_int16_t proto,
-      u_int8_t ip_invflags,
-      u_int8_t match_flags,
-      u_int8_t count)
+multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
+{
+	unsigned int i;
+
+	for (i = 0; i < multiinfo->count; i++) {
+		if (!multiinfo->pflags[i])
+			continue;
+
+		if (i == multiinfo->count - 1)
+			return false;
+
+		i++;
+	}
+
+	return true;
+}
+
+static inline bool
+check(u16 proto, u8 ip_invflags, const struct xt_multiport_v1 *multiinfo)
 {
 	/* Must specify supported protocol, no unknown flags or bad count */
-	return (proto == IPPROTO_TCP || proto == IPPROTO_UDP
-		|| proto == IPPROTO_UDPLITE
-		|| proto == IPPROTO_SCTP || proto == IPPROTO_DCCP)
-		&& !(ip_invflags & XT_INV_PROTO)
-		&& (match_flags == XT_MULTIPORT_SOURCE
-		    || match_flags == XT_MULTIPORT_DESTINATION
-		    || match_flags == XT_MULTIPORT_EITHER)
-		&& count <= XT_MULTI_PORTS;
+	return (proto == IPPROTO_TCP || proto == IPPROTO_UDP ||
+		proto == IPPROTO_UDPLITE ||
+		proto == IPPROTO_SCTP || proto == IPPROTO_DCCP) &&
+	       !(ip_invflags & XT_INV_PROTO) &&
+	       (multiinfo->flags == XT_MULTIPORT_SOURCE ||
+		multiinfo->flags == XT_MULTIPORT_DESTINATION ||
+		multiinfo->flags == XT_MULTIPORT_EITHER) &&
+	       multiinfo->count <= XT_MULTI_PORTS &&
+	       multiport_valid_ranges(multiinfo);
 }
 
 static int multiport_mt_check(const struct xt_mtchk_param *par)
@@ -127,8 +143,7 @@ static int multiport_mt_check(const struct xt_mtchk_param *par)
 	const struct ipt_ip *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	return check(ip->proto, ip->invflags, multiinfo) ? 0 : -EINVAL;
 }
 
 static int multiport_mt6_check(const struct xt_mtchk_param *par)
@@ -136,8 +151,7 @@ static int multiport_mt6_check(const struct xt_mtchk_param *par)
 	const struct ip6t_ip6 *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	return check(ip->proto, ip->invflags, multiinfo) ? 0 : -EINVAL;
 }
 
 static struct xt_match multiport_mt_reg[] __read_mostly = {
-- 
2.51.0

Re: [PATCH v2] netfilter: xt_multiport: reject trailing range markers

[Pablo Neira Ayuso]

Hi,

On Sat, Mar 28, 2026 at 10:51:23PM +0800, Ren Wei wrote:
> diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
> index 44a00f5acde8..38aa5b90d38e 100644
> --- a/net/netfilter/xt_multiport.c
> +++ b/net/netfilter/xt_multiport.c
> @@ -26,10 +26,10 @@ MODULE_ALIAS("ip6t_multiport");
>  /* Returns 1 if the port is matched by the test, 0 otherwise. */
>  static inline bool
>  ports_match_v1(const struct xt_multiport_v1 *minfo,
> -	       u_int16_t src, u_int16_t dst)
> +	       u16 src, u16 dst)
>  {
>  	unsigned int i;
> -	u_int16_t s, e;
> +	u16 s, e;
>  
>  	for (i = 0; i < minfo->count; i++) {
>  		s = minfo->ports[i];

I see, this is a cleanup to use preferred datatypes.

> @@ -106,20 +106,36 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  }
>  
>  static inline bool
> -check(u_int16_t proto,
> -      u_int8_t ip_invflags,
> -      u_int8_t match_flags,
> -      u_int8_t count)
> +multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < multiinfo->count; i++) {
> +		if (!multiinfo->pflags[i])
> +			continue;
> +
> +		if (i == multiinfo->count - 1)
> +			return false;
> +
> +		i++;
> +	}

Why so convoluted? This should just fix this bug:

static bool multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
{
        return multiinfo->pflags[multiinfo->count - 1] == 0;
}

This is an off-by-one read bug from packet path that is possible
because the last slot in the multiport array _must_ be zero.

pflags[i] non-zero means beginning of a range, and pflags[i+1] zero
means end of range (or singleton port).

Actually multiport_valid_ranges() can just go away too given the more
simple fix.

> +
> +	return true;
> +}
> +
> +static inline bool
> +check(u16 proto, u8 ip_invflags, const struct xt_multiport_v1 *multiinfo)
>  {
>  	/* Must specify supported protocol, no unknown flags or bad count */
> -	return (proto == IPPROTO_TCP || proto == IPPROTO_UDP
> -		|| proto == IPPROTO_UDPLITE
> -		|| proto == IPPROTO_SCTP || proto == IPPROTO_DCCP)
> -		&& !(ip_invflags & XT_INV_PROTO)
> -		&& (match_flags == XT_MULTIPORT_SOURCE
> -		    || match_flags == XT_MULTIPORT_DESTINATION
> -		    || match_flags == XT_MULTIPORT_EITHER)
> -		&& count <= XT_MULTI_PORTS;
> +	return (proto == IPPROTO_TCP || proto == IPPROTO_UDP ||
> +		proto == IPPROTO_UDPLITE ||
> +		proto == IPPROTO_SCTP || proto == IPPROTO_DCCP) &&
> +	       !(ip_invflags & XT_INV_PROTO) &&
> +	       (multiinfo->flags == XT_MULTIPORT_SOURCE ||
> +		multiinfo->flags == XT_MULTIPORT_DESTINATION ||
> +		multiinfo->flags == XT_MULTIPORT_EITHER) &&
> +	       multiinfo->count <= XT_MULTI_PORTS &&
> +	       multiport_valid_ranges(multiinfo);

It took me a while to review this cleanup-in-fix is doing what it is
intended. While this coding style is preferred these days, I'm unsure
I want this cleanup in this fix.

I think this fix can be turned into a oneliner...

>  }
>  
>  static int multiport_mt_check(const struct xt_mtchk_param *par)
> @@ -127,8 +143,7 @@ static int multiport_mt_check(const struct xt_mtchk_param *par)
>  	const struct ipt_ip *ip = par->entryinfo;
>  	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
>  
> -	return check(ip->proto, ip->invflags, multiinfo->flags,
> -		     multiinfo->count) ? 0 : -EINVAL;
> +	return check(ip->proto, ip->invflags, multiinfo) ? 0 : -EINVAL;
>  }
>  
>  static int multiport_mt6_check(const struct xt_mtchk_param *par)
> @@ -136,8 +151,7 @@ static int multiport_mt6_check(const struct xt_mtchk_param *par)
>  	const struct ip6t_ip6 *ip = par->entryinfo;
>  	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
>  
> -	return check(ip->proto, ip->invflags, multiinfo->flags,
> -		     multiinfo->count) ? 0 : -EINVAL;
> +	return check(ip->proto, ip->invflags, multiinfo) ? 0 : -EINVAL;
>  }
>  
>  static struct xt_match multiport_mt_reg[] __read_mostly = {
> -- 
> 2.51.0
> 

Re: [PATCH v2] netfilter: xt_multiport: reject trailing range markers

[Pablo Neira Ayuso]

On Wed, Apr 01, 2026 at 01:29:38AM +0200, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Sat, Mar 28, 2026 at 10:51:23PM +0800, Ren Wei wrote:
> > diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
> > index 44a00f5acde8..38aa5b90d38e 100644
> > --- a/net/netfilter/xt_multiport.c
> > +++ b/net/netfilter/xt_multiport.c
> > @@ -26,10 +26,10 @@ MODULE_ALIAS("ip6t_multiport");
> >  /* Returns 1 if the port is matched by the test, 0 otherwise. */
> >  static inline bool
> >  ports_match_v1(const struct xt_multiport_v1 *minfo,
> > -	       u_int16_t src, u_int16_t dst)
> > +	       u16 src, u16 dst)
> >  {
> >  	unsigned int i;
> > -	u_int16_t s, e;
> > +	u16 s, e;
> >  
> >  	for (i = 0; i < minfo->count; i++) {
> >  		s = minfo->ports[i];
> 
> I see, this is a cleanup to use preferred datatypes.
> 
> > @@ -106,20 +106,36 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  }
> >  
> >  static inline bool
> > -check(u_int16_t proto,
> > -      u_int8_t ip_invflags,
> > -      u_int8_t match_flags,
> > -      u_int8_t count)
> > +multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < multiinfo->count; i++) {
> > +		if (!multiinfo->pflags[i])
> > +			continue;
> > +
> > +		if (i == multiinfo->count - 1)
> > +			return false;
> > +
> > +		i++;
> > +	}
> 
> Why so convoluted? This should just fix this bug:
> 
> static bool multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
> {
>         return multiinfo->pflags[multiinfo->count - 1] == 0;
> }

Another possibility is full-blown range validation, ie.

if pflags[i] is one, then pflags[i+1] must be zero, checking that i+1
do not go over XT_MULTI_PORTS beforehand.

[PATCH v2] netfilter: xt_multiport: validate range encoding in checkentry

[Ren Wei]

ports_match_v1() treats any non-zero pflags entry as the start of a
port range and unconditionally consumes the next ports[] element as
the range end.

The checkentry path currently validates protocol, flags and count, but
it does not validate the range encoding itself. As a result, malformed
rules can mark the last slot as a range start or place two range starts
back to back, leaving ports_match_v1() to step past the last valid
ports[] element while interpreting the rule.

Reject malformed multiport v1 rules in checkentry by validating that
each range start has a following element and that the following element
is not itself marked as another range start.

Fixes: a89ecb6a2ef7 ("[NETFILTER]: x_tables: unify IPv4/IPv6 multiport match")
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Yuhang Zheng <z1652074432@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/netfilter/xt_multiport.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
index 44a00f5acde8..07a0f2a3fc75 100644
--- a/net/netfilter/xt_multiport.c
+++ b/net/netfilter/xt_multiport.c
@@ -105,6 +105,24 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	return ports_match_v1(multiinfo, ntohs(pptr[0]), ntohs(pptr[1]));
 }
 
+static inline bool
+multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
+{
+	unsigned int i;
+
+	for (i = 0; i < multiinfo->count; i++) {
+		if (!multiinfo->pflags[i])
+			continue;
+
+		if (i + 1 >= multiinfo->count || multiinfo->pflags[i + 1])
+			return false;
+
+		i++;
+	}
+
+	return true;
+}
+
 static inline bool
 check(u_int16_t proto,
       u_int8_t ip_invflags,
@@ -127,8 +145,10 @@ static int multiport_mt_check(const struct xt_mtchk_param *par)
 	const struct ipt_ip *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	if (!check(ip->proto, ip->invflags, multiinfo->flags, multiinfo->count))
+		return -EINVAL;
+
+	return multiport_valid_ranges(multiinfo) ? 0 : -EINVAL;
 }
 
 static int multiport_mt6_check(const struct xt_mtchk_param *par)
@@ -136,8 +156,10 @@ static int multiport_mt6_check(const struct xt_mtchk_param *par)
 	const struct ip6t_ip6 *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	if (!check(ip->proto, ip->invflags, multiinfo->flags, multiinfo->count))
+		return -EINVAL;
+
+	return multiport_valid_ranges(multiinfo) ? 0 : -EINVAL;
 }
 
 static struct xt_match multiport_mt_reg[] __read_mostly = {
-- 
2.51.0

[PATCH v3] netfilter: xt_multiport: validate range encoding in checkentry

[Ren Wei]

ports_match_v1() treats any non-zero pflags entry as the start of a
port range and unconditionally consumes the next ports[] element as
the range end.

The checkentry path currently validates protocol, flags and count, but
it does not validate the range encoding itself. As a result, malformed
rules can mark the last slot as a range start or place two range starts
back to back, leaving ports_match_v1() to step past the last valid
ports[] element while interpreting the rule.

Reject malformed multiport v1 rules in checkentry by validating that
each range start has a following element and that the following element
is not itself marked as another range start.

Fixes: a89ecb6a2ef7 ("[NETFILTER]: x_tables: unify IPv4/IPv6 multiport match")
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Yuhang Zheng <z1652074432@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
Changes in v2:
- drop the selftest patch
- send the fix publicly to netfilter-devel

Changes in v3:
- drop datatype cleanup from the fix
- keep the original check() interface unchanged
- validate malformed range encoding in checkentry

 net/netfilter/xt_multiport.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
index 44a00f5acde8..07a0f2a3fc75 100644
--- a/net/netfilter/xt_multiport.c
+++ b/net/netfilter/xt_multiport.c
@@ -105,6 +105,24 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	return ports_match_v1(multiinfo, ntohs(pptr[0]), ntohs(pptr[1]));
 }
 
+static inline bool
+multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
+{
+	unsigned int i;
+
+	for (i = 0; i < multiinfo->count; i++) {
+		if (!multiinfo->pflags[i])
+			continue;
+
+		if (i + 1 >= multiinfo->count || multiinfo->pflags[i + 1])
+			return false;
+
+		i++;
+	}
+
+	return true;
+}
+
 static inline bool
 check(u_int16_t proto,
       u_int8_t ip_invflags,
@@ -127,8 +145,10 @@ static int multiport_mt_check(const struct xt_mtchk_param *par)
 	const struct ipt_ip *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	if (!check(ip->proto, ip->invflags, multiinfo->flags, multiinfo->count))
+		return -EINVAL;
+
+	return multiport_valid_ranges(multiinfo) ? 0 : -EINVAL;
 }
 
 static int multiport_mt6_check(const struct xt_mtchk_param *par)
@@ -136,8 +156,10 @@ static int multiport_mt6_check(const struct xt_mtchk_param *par)
 	const struct ip6t_ip6 *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	if (!check(ip->proto, ip->invflags, multiinfo->flags, multiinfo->count))
+		return -EINVAL;
+
+	return multiport_valid_ranges(multiinfo) ? 0 : -EINVAL;
 }
 
 static struct xt_match multiport_mt_reg[] __read_mostly = {
-- 
2.51.0

Re: [PATCH v3] netfilter: xt_multiport: validate range encoding in checkentry

[Pablo Neira Ayuso]

On Fri, Apr 03, 2026 at 02:21:17AM +0800, Ren Wei wrote:
> ports_match_v1() treats any non-zero pflags entry as the start of a
> port range and unconditionally consumes the next ports[] element as
> the range end.
> 
> The checkentry path currently validates protocol, flags and count, but
> it does not validate the range encoding itself. As a result, malformed
> rules can mark the last slot as a range start or place two range starts
> back to back, leaving ports_match_v1() to step past the last valid
> ports[] element while interpreting the rule.
> 
> Reject malformed multiport v1 rules in checkentry by validating that
> each range start has a following element and that the following element
> is not itself marked as another range start.
> 
> Fixes: a89ecb6a2ef7 ("[NETFILTER]: x_tables: unify IPv4/IPv6 multiport match")
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Co-developed-by: Yuan Tan <yuantan098@gmail.com>
> Signed-off-by: Yuan Tan <yuantan098@gmail.com>
> Suggested-by: Xin Liu <bird@lzu.edu.cn>
> Tested-by: Yuhang Zheng <z1652074432@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
> Changes in v2:
> - drop the selftest patch
> - send the fix publicly to netfilter-devel
> 
> Changes in v3:
> - drop datatype cleanup from the fix
> - keep the original check() interface unchanged
> - validate malformed range encoding in checkentry
> 
>  net/netfilter/xt_multiport.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
> index 44a00f5acde8..07a0f2a3fc75 100644
> --- a/net/netfilter/xt_multiport.c
> +++ b/net/netfilter/xt_multiport.c
> @@ -105,6 +105,24 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  	return ports_match_v1(multiinfo, ntohs(pptr[0]), ntohs(pptr[1]));
>  }
>  
> +static inline bool
> +multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < multiinfo->count; i++) {
> +		if (!multiinfo->pflags[i])
> +			continue;
> +
> +		if (i + 1 >= multiinfo->count || multiinfo->pflags[i + 1])
> +			return false;
> +
> +		i++;
> +	}
> +
> +	return true;
> +}

I'd suggest:

static inline bool
multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
{
        unsigned int i;
 
        for (i = 0; i < multiinfo->count; i++) {
                if (!multiinfo->pflags[i])
                        continue;

                if (++i >= multiinfo->count)
                        return false;
         
                if (multiinfo->pflags[i])
                        return false;
         
                if (multiinfo->ports[i - 1] > multiinfo->ports[i])
                        return false;
        }
 
        return true;
}

Then, this validate non-sense ports array too.

Re: [PATCH v3] netfilter: xt_multiport: validate range encoding in checkentry

[Pablo Neira Ayuso]

On Fri, Apr 03, 2026 at 01:31:41PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 03, 2026 at 02:21:17AM +0800, Ren Wei wrote:
> > ports_match_v1() treats any non-zero pflags entry as the start of a
> > port range and unconditionally consumes the next ports[] element as
> > the range end.
> > 
> > The checkentry path currently validates protocol, flags and count, but
> > it does not validate the range encoding itself. As a result, malformed
> > rules can mark the last slot as a range start or place two range starts
> > back to back, leaving ports_match_v1() to step past the last valid
> > ports[] element while interpreting the rule.
> > 
> > Reject malformed multiport v1 rules in checkentry by validating that
> > each range start has a following element and that the following element
> > is not itself marked as another range start.
> > 
> > Fixes: a89ecb6a2ef7 ("[NETFILTER]: x_tables: unify IPv4/IPv6 multiport match")
> > Reported-by: Yifan Wu <yifanwucs@gmail.com>
> > Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> > Co-developed-by: Yuan Tan <yuantan098@gmail.com>
> > Signed-off-by: Yuan Tan <yuantan098@gmail.com>
> > Suggested-by: Xin Liu <bird@lzu.edu.cn>
> > Tested-by: Yuhang Zheng <z1652074432@gmail.com>
> > Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> > ---
> > Changes in v2:
> > - drop the selftest patch
> > - send the fix publicly to netfilter-devel
> > 
> > Changes in v3:
> > - drop datatype cleanup from the fix
> > - keep the original check() interface unchanged
> > - validate malformed range encoding in checkentry
> > 
> >  net/netfilter/xt_multiport.c | 30 ++++++++++++++++++++++++++----
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
> > index 44a00f5acde8..07a0f2a3fc75 100644
> > --- a/net/netfilter/xt_multiport.c
> > +++ b/net/netfilter/xt_multiport.c
> > @@ -105,6 +105,24 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
> >  	return ports_match_v1(multiinfo, ntohs(pptr[0]), ntohs(pptr[1]));
> >  }
> >  
> > +static inline bool
> > +multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < multiinfo->count; i++) {
> > +		if (!multiinfo->pflags[i])
> > +			continue;
> > +
> > +		if (i + 1 >= multiinfo->count || multiinfo->pflags[i + 1])
> > +			return false;
> > +
> > +		i++;
> > +	}
> > +
> > +	return true;
> > +}
> 
> I'd suggest:
> 
> static inline bool

Actually, inline is silly here, no inline here.

> multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
> {
>         unsigned int i;
>  
>         for (i = 0; i < multiinfo->count; i++) {
>                 if (!multiinfo->pflags[i])
>                         continue;
> 
>                 if (++i >= multiinfo->count)
>                         return false;
>          
>                 if (multiinfo->pflags[i])
>                         return false;
>          
>                 if (multiinfo->ports[i - 1] > multiinfo->ports[i])
>                         return false;
>         }
>  
>         return true;
> }
> 
> Then, this validate non-sense ports array too.

You can also mention in the patch description that this leads to
off-by-one read after the array that is reported via UBSAN.

Thanks.

[PATCH v4] netfilter: xt_multiport: validate range encoding in checkentry

[Ren Wei]

ports_match_v1() treats any non-zero pflags entry as the start of a
port range and unconditionally consumes the next ports[] element as
the range end.

The checkentry path currently validates protocol, flags and count, but
it does not validate the range encoding itself. As a result, malformed
rules can mark the last slot as a range start or place two range starts
back to back, leaving ports_match_v1() to step past the last valid
ports[] element while interpreting the rule.

Reject malformed multiport v1 rules in checkentry by validating that
each range start has a following element and that the following element
is not itself marked as another range start.

Fixes: a89ecb6a2ef7 ("[NETFILTER]: x_tables: unify IPv4/IPv6 multiport match")
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Yuhang Zheng <z1652074432@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
Changes in v2:
- drop the selftest patch
- send the fix publicly to netfilter-devel

Changes in v3:
- drop datatype cleanup from the fix
- keep the original check() interface unchanged
- validate malformed range encoding in checkentry

Changes in v4:
- follow Pablo's suggested validation form
- drop inline from multiport_valid_ranges
- reject reversed port ranges

 net/netfilter/xt_multiport.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/xt_multiport.c b/net/netfilter/xt_multiport.c
index 44a00f5acde8..a1691ff405d3 100644
--- a/net/netfilter/xt_multiport.c
+++ b/net/netfilter/xt_multiport.c
@@ -105,6 +105,28 @@ multiport_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	return ports_match_v1(multiinfo, ntohs(pptr[0]), ntohs(pptr[1]));
 }
 
+static bool
+multiport_valid_ranges(const struct xt_multiport_v1 *multiinfo)
+{
+	unsigned int i;
+
+	for (i = 0; i < multiinfo->count; i++) {
+		if (!multiinfo->pflags[i])
+			continue;
+
+		if (++i >= multiinfo->count)
+			return false;
+
+		if (multiinfo->pflags[i])
+			return false;
+
+		if (multiinfo->ports[i - 1] > multiinfo->ports[i])
+			return false;
+	}
+
+	return true;
+}
+
 static inline bool
 check(u_int16_t proto,
       u_int8_t ip_invflags,
@@ -127,8 +149,10 @@ static int multiport_mt_check(const struct xt_mtchk_param *par)
 	const struct ipt_ip *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	if (!check(ip->proto, ip->invflags, multiinfo->flags, multiinfo->count))
+		return -EINVAL;
+
+	return multiport_valid_ranges(multiinfo) ? 0 : -EINVAL;
 }
 
 static int multiport_mt6_check(const struct xt_mtchk_param *par)
@@ -136,8 +160,10 @@ static int multiport_mt6_check(const struct xt_mtchk_param *par)
 	const struct ip6t_ip6 *ip = par->entryinfo;
 	const struct xt_multiport_v1 *multiinfo = par->matchinfo;
 
-	return check(ip->proto, ip->invflags, multiinfo->flags,
-		     multiinfo->count) ? 0 : -EINVAL;
+	if (!check(ip->proto, ip->invflags, multiinfo->flags, multiinfo->count))
+		return -EINVAL;
+
+	return multiport_valid_ranges(multiinfo) ? 0 : -EINVAL;
 }
 
 static struct xt_match multiport_mt_reg[] __read_mostly = {
-- 
2.51.0