diff mbox series

[net,v5,1/2] net: netfilter: Limit the number of ftp helper port attempts

Message ID 20210920005905.9583-2-Cole.Dishington@alliedtelesis.co.nz (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 2 this patch: 3
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 56 lines checked
netdev/build_allmodconfig_warn fail Errors and warnings before: 2 this patch: 3
netdev/header_inline success Link

Commit Message

Cole Dishington Sept. 20, 2021, 12:59 a.m. UTC 
In preparation of fixing the port selection of ftp helper when using
NF_NAT_RANGE_PROTO_SPECIFIED, limit the number of ftp helper port
attempts to 128.

Looping a large port range takes too long. Instead select a random
offset within [ntohs(exp->saved_proto.tcp.port), 65535] and try 128
ports.

Co-developed-by: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
Signed-off-by: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
Co-developed-by: Scott Parlane <scott.parlane@alliedtelesis.co.nz>
Signed-off-by: Scott Parlane <scott.parlane@alliedtelesis.co.nz>
Co-developed-by: Blair Steven <blair.steven@alliedtelesis.co.nz>
Signed-off-by: Blair Steven <blair.steven@alliedtelesis.co.nz>
Signed-off-by: Cole Dishington <Cole.Dishington@alliedtelesis.co.nz>
---
 net/netfilter/nf_nat_ftp.c | 39 +++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

Comments

kernel test robot Sept. 20, 2021, 5:09 a.m. UTC | #1 
Hi Cole,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git e30cd812dffadc58241ae378e48728e6a161becd
config: i386-defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/b90b875dc5be3c59ec418ce403a8d749690a24ec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
        git checkout b90b875dc5be3c59ec418ce403a8d749690a24ec
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/netfilter/nf_nat_ftp.c: In function 'nf_nat_ftp':
>> net/netfilter/nf_nat_ftp.c:117:37: warning: format '%u' expects a matching 'unsigned int' argument [-Wformat=]
     117 |   nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
         |                                    ~^
         |                                     |
         |                                     unsigned int


vim +117 net/netfilter/nf_nat_ftp.c

    60	
    61	/* So, this packet has hit the connection tracking matching code.
    62	   Mangle it, and change the expectation to match the new version. */
    63	static unsigned int nf_nat_ftp(struct sk_buff *skb,
    64				       enum ip_conntrack_info ctinfo,
    65				       enum nf_ct_ftp_type type,
    66				       unsigned int protoff,
    67				       unsigned int matchoff,
    68				       unsigned int matchlen,
    69				       struct nf_conntrack_expect *exp)
    70	{
    71		union nf_inet_addr newaddr;
    72		u_int16_t port;
    73		int dir = CTINFO2DIR(ctinfo);
    74		struct nf_conn *ct = exp->master;
    75		unsigned int i, min, max, range_size;
    76		static const unsigned int max_attempts = 128;
    77		char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
    78		unsigned int buflen;
    79		int ret;
    80	
    81		pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
    82	
    83		/* Connection will come from wherever this packet goes, hence !dir */
    84		newaddr = ct->tuplehash[!dir].tuple.dst.u3;
    85		exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
    86		exp->dir = !dir;
    87	
    88		/* When you see the packet, we need to NAT it the same as the
    89		 * this one. */
    90		exp->expectfn = nf_nat_follow_master;
    91	
    92		min = ntohs(exp->saved_proto.tcp.port);
    93		max = 65535;
    94	
    95		/* Try to get same port */
    96		ret = nf_ct_expect_related(exp, 0);
    97	
    98		/* if same port is not in range or available, try to change it. */
    99		if (ret != 0) {
   100			range_size = max - min + 1;
   101			if (range_size > max_attempts)
   102				range_size = max_attempts;
   103	
   104			port = min + prandom_u32_max(max - min);
   105			for (i = 0; i < range_size; i++) {
   106				exp->tuple.dst.u.tcp.port = htons(port);
   107				ret = nf_ct_expect_related(exp, 0);
   108				if (ret != -EBUSY)
   109					break;
   110				port++;
   111				if (port > max)
   112					port = min;
   113			}
   114		}
   115	
   116		if (ret != 0) {
 > 117			nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
   118			return NF_DROP;
   119		}
   120	
   121		buflen = nf_nat_ftp_fmt_cmd(ct, type, buffer, sizeof(buffer),
   122					    &newaddr, port);
   123		if (!buflen)
   124			goto out;
   125	
   126		pr_debug("calling nf_nat_mangle_tcp_packet\n");
   127	
   128		if (!nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff, matchoff,
   129					      matchlen, buffer, buflen))
   130			goto out;
   131	
   132		return NF_ACCEPT;
   133	
   134	out:
   135		nf_ct_helper_log(skb, ct, "cannot mangle packet");
   136		nf_ct_unexpect_related(exp);
   137		return NF_DROP;
   138	}
   139	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Sept. 20, 2021, 6:05 a.m. UTC | #2 
Hi Cole,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git e30cd812dffadc58241ae378e48728e6a161becd
config: x86_64-randconfig-a002-20210920 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c8b3d7d6d6de37af68b2f379d0e37304f78e115f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b90b875dc5be3c59ec418ce403a8d749690a24ec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
        git checkout b90b875dc5be3c59ec418ce403a8d749690a24ec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_nat_ftp.c:117:37: warning: more '%' conversions than data arguments [-Wformat-insufficient-args]
                   nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
                                                    ~^
   1 warning generated.


vim +117 net/netfilter/nf_nat_ftp.c

    60	
    61	/* So, this packet has hit the connection tracking matching code.
    62	   Mangle it, and change the expectation to match the new version. */
    63	static unsigned int nf_nat_ftp(struct sk_buff *skb,
    64				       enum ip_conntrack_info ctinfo,
    65				       enum nf_ct_ftp_type type,
    66				       unsigned int protoff,
    67				       unsigned int matchoff,
    68				       unsigned int matchlen,
    69				       struct nf_conntrack_expect *exp)
    70	{
    71		union nf_inet_addr newaddr;
    72		u_int16_t port;
    73		int dir = CTINFO2DIR(ctinfo);
    74		struct nf_conn *ct = exp->master;
    75		unsigned int i, min, max, range_size;
    76		static const unsigned int max_attempts = 128;
    77		char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
    78		unsigned int buflen;
    79		int ret;
    80	
    81		pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
    82	
    83		/* Connection will come from wherever this packet goes, hence !dir */
    84		newaddr = ct->tuplehash[!dir].tuple.dst.u3;
    85		exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
    86		exp->dir = !dir;
    87	
    88		/* When you see the packet, we need to NAT it the same as the
    89		 * this one. */
    90		exp->expectfn = nf_nat_follow_master;
    91	
    92		min = ntohs(exp->saved_proto.tcp.port);
    93		max = 65535;
    94	
    95		/* Try to get same port */
    96		ret = nf_ct_expect_related(exp, 0);
    97	
    98		/* if same port is not in range or available, try to change it. */
    99		if (ret != 0) {
   100			range_size = max - min + 1;
   101			if (range_size > max_attempts)
   102				range_size = max_attempts;
   103	
   104			port = min + prandom_u32_max(max - min);
   105			for (i = 0; i < range_size; i++) {
   106				exp->tuple.dst.u.tcp.port = htons(port);
   107				ret = nf_ct_expect_related(exp, 0);
   108				if (ret != -EBUSY)
   109					break;
   110				port++;
   111				if (port > max)
   112					port = min;
   113			}
   114		}
   115	
   116		if (ret != 0) {
 > 117			nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
   118			return NF_DROP;
   119		}
   120	
   121		buflen = nf_nat_ftp_fmt_cmd(ct, type, buffer, sizeof(buffer),
   122					    &newaddr, port);
   123		if (!buflen)
   124			goto out;
   125	
   126		pr_debug("calling nf_nat_mangle_tcp_packet\n");
   127	
   128		if (!nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff, matchoff,
   129					      matchlen, buffer, buflen))
   130			goto out;
   131	
   132		return NF_ACCEPT;
   133	
   134	out:
   135		nf_ct_helper_log(skb, ct, "cannot mangle packet");
   136		nf_ct_unexpect_related(exp);
   137		return NF_DROP;
   138	}
   139	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Florian Westphal Sept. 20, 2021, 7:22 a.m. UTC | #3 
Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> In preparation of fixing the port selection of ftp helper when using
> NF_NAT_RANGE_PROTO_SPECIFIED, limit the number of ftp helper port
> attempts to 128.
> 
> Looping a large port range takes too long. Instead select a random
> offset within [ntohs(exp->saved_proto.tcp.port), 65535] and try 128
> ports.

LGTM, please fix the format argument error the kbuild robot reported
and resend.  You may add

Acked-by: Florian Westphal <fw@strlen.de>

when doing so.
diff mbox series

Patch

diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index aace6768a64e..7dcb4f179ac9 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -72,8 +72,11 @@  static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	u_int16_t port;
 	int dir = CTINFO2DIR(ctinfo);
 	struct nf_conn *ct = exp->master;
+	unsigned int i, min, max, range_size;
+	static const unsigned int max_attempts = 128;
 	char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
 	unsigned int buflen;
+	int ret;
 
 	pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
 
@@ -86,22 +89,32 @@  static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	 * this one. */
 	exp->expectfn = nf_nat_follow_master;
 
-	/* Try to get same port: if not, try to change it. */
-	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
-		int ret;
-
-		exp->tuple.dst.u.tcp.port = htons(port);
-		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			port = 0;
-			break;
+	min = ntohs(exp->saved_proto.tcp.port);
+	max = 65535;
+
+	/* Try to get same port */
+	ret = nf_ct_expect_related(exp, 0);
+
+	/* if same port is not in range or available, try to change it. */
+	if (ret != 0) {
+		range_size = max - min + 1;
+		if (range_size > max_attempts)
+			range_size = max_attempts;
+
+		port = min + prandom_u32_max(max - min);
+		for (i = 0; i < range_size; i++) {
+			exp->tuple.dst.u.tcp.port = htons(port);
+			ret = nf_ct_expect_related(exp, 0);
+			if (ret != -EBUSY)
+				break;
+			port++;
+			if (port > max)
+				port = min;
 		}
 	}
 
-	if (port == 0) {
-		nf_ct_helper_log(skb, ct, "all ports in use");
+	if (ret != 0) {
+		nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
 		return NF_DROP;
 	}