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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.