netfilter: nft_exthdr: fix endianness of tcp option cast

PATCH In review Sergey Marinkevich 3 participants 3 messages Mar 29, 2020 – Apr 2, 2020

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Sergey Marinkevich

author

2020-03-29 12:29 UTC #0 lore raw

PATCH 1/1 netfilter: nft_exthdr: fix endianness of tcp option cast

I got a problem on MIPS with Big-Endian is turned on: every time when NF trying to change TCP MSS it returns because of new.v16 was greater than old.v16. But real MSS was 1460 and my rule was like this:

add rule table chain tcp option maxseg size set 1400

And 1400 is lesser that 1460, not greater.

Later I founded that main causer is cast from u32 to __be16.

Debugging:

In example MSS = 1400(HEX: 0x578). Here is representation of each byte like it is in memory by addresses from left to right(e.g. [0x0 0x1 0x2 0x3]). LE — Little-Endian system, BE — Big-Endian, left column is type.

	     LE               BE
	u32: [78 05 00 00]    [00 00 05 78]

As you can see, u32 representation will be casted to u16 from different half of 4-byte address range. But actually nf_tables uses registers and store data of various size. Actually TCP MSS stored in 2 bytes. But registers are still u32 in definition:

	struct nft_regs {
		union {
			u32			data[20];
			struct nft_verdict	verdict;
		};
	};

So, access like regs->data[priv->sreg] exactly u32. So, according to table presents above, per-byte representation of stored TCP MSS in register will be:

	                     LE               BE
	(u32)regs->data[]:   [78 05 00 00]    [05 78 00 00]
	                                       ^^ ^^

We see that register uses just half of u32 and other 2 bytes may be used for some another data. But in nft_exthdr_tcp_set_eval() it casted just like u32 -> __be16:

	new.v16 = src

But u32 overfill __be16, so it get 2 low bytes. For clarity draw one more table(<xx xx> means that bytes will be used for cast).

	                     LE                 BE
	u32:                 [<78 05> 00 00]    [00 00 <05 78>]
	(u32)regs->data[]:   [<78 05> 00 00]    [05 78 <00 00>]

As you can see, for Little-Endian nothing changes, but for Big-endian we take the wrong half. In my case there is some other data instead of zeros, so new MSS was wrongly greater.

For shooting this bug I used solution for ports ranges. Applying of this patch does not affect Little-Endian systems.

Signed-off-by Sergey Marinkevich <[email protected]>

---
 net/netfilter/nft_exthdr.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

net/netfilter/nft_exthdr.c +3 -5

index a5e8469859e3..07782836fad6 100644
@@ -228,7 +228,6 @@ static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
 	unsigned int i, optl, tcphdr_len, offset;
 	struct tcphdr *tcph;
 	u8 *opt;
-	u32 src;
 
 	tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff, &tcphdr_len);
 	if (!tcph)

@@ -237,7 +236,6 @@ static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
 	opt = (u8 *)tcph;
 	for (i = sizeof(*tcph); i < tcphdr_len - 1; i += optl) {
 		union {
-			u8 octet;
 			__be16 v16;
 			__be32 v32;
 		} old, new;

@@ -259,13 +257,13 @@ static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
 		if (!tcph)
 			return;
 
-		src = regs->data[priv->sreg];
 		offset = i + priv->offset;
 
 		switch (priv->len) {
 		case 2:
 			old.v16 = get_unaligned((u16 *)(opt + offset));
-			new.v16 = src;
+			new.v16 = (__force __be16)nft_reg_load16(
+				&regs->data[priv->sreg]);
 
 			switch (priv->type) {
 			case TCPOPT_MSS:

@@ -283,7 +281,7 @@ static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
 						 old.v16, new.v16, false);
 			break;
 		case 4:
-			new.v32 = src;
+			new.v32 = regs->data[priv->sreg];
 			old.v32 = get_unaligned((u32 *)(opt + offset));
 
 			if (old.v32 == new.v32)

Signature · 1 line

2.21.0

Florian Westphal

maintainer Acked-by on v1 1/1

2020-03-29 13:44 UTC #1 lore raw

Quoted 9 lines · Sergey Marinkevich

I got a problem on MIPS with Big-Endian is turned on: every time when NF trying to change TCP MSS it returns because of new.v16 was greater than old.v16. But real MSS was 1460 and my rule was like this:
add rule table chain tcp option maxseg size set 1400
And 1400 is lesser that 1460, not greater.
Later I founded that main causer is cast from u32 to __be16.

LGTM.

Acked-by Florian Westphal <[email protected]>

kbuild test robot

build bot

2020-04-02 01:12 UTC #2 lore raw

In reply to Sergey Marinkevich

Thank you for the patch! Perhaps something to improve:

Hi Sergey,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on nf/master]
[also build test WARNING on nf-next/master ipvs/master v5.6 next-20200327] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Sergey-Marinkevich/netfilter-nft_exthdr-fix-endianness-of-tcp-option-cast/20200329-215834
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git master
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-187-gbff9b106-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
:::::: branch date: 11 hours ago
:::::: commit date: 11 hours ago

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

net/netfilter/nft_exthdr.c:264:33: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 [usertype] v16 @@ got 16 [usertype] v16 @@
net/netfilter/nft_exthdr.c:264:33: sparse: expected restricted __be16 [usertype] v16
net/netfilter/nft_exthdr.c:264:33: sparse: got unsigned short

>> net/netfilter/nft_exthdr.c:284:33: sparse: sparse: incorrect type in assignment (different base types) @@    expected restricted __be32 [assigned] [usertype] v32 @@    got  [usertype] v32 @@
>> net/netfilter/nft_exthdr.c:284:33: sparse:    expected restricted __be32 [assigned] [usertype] v32

net/netfilter/nft_exthdr.c:284:33: sparse: got unsigned int
net/netfilter/nft_exthdr.c:285:33: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __be32 [usertype] v32 @@ got [usertype] v32 @@
net/netfilter/nft_exthdr.c:285:33: sparse: expected restricted __be32 [usertype] v32
net/netfilter/nft_exthdr.c:285:33: sparse: got unsigned int

# https://github.com/0day-ci/linux/commit/d9b9ce8e0720a559cb92e044b3721f9acc51990b
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout d9b9ce8e0720a559cb92e044b3721f9acc51990b
vim +284 net/netfilter/nft_exthdr.c

935b7f64301887 Manuel Messner     2017-02-07  221  
99d1712bc41c7c Florian Westphal   2017-08-08  222  static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
99d1712bc41c7c Florian Westphal   2017-08-08  223  				    struct nft_regs *regs,
99d1712bc41c7c Florian Westphal   2017-08-08  224  				    const struct nft_pktinfo *pkt)
99d1712bc41c7c Florian Westphal   2017-08-08  225  {
99d1712bc41c7c Florian Westphal   2017-08-08  226  	u8 buff[sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE];
99d1712bc41c7c Florian Westphal   2017-08-08  227  	struct nft_exthdr *priv = nft_expr_priv(expr);
99d1712bc41c7c Florian Westphal   2017-08-08  228  	unsigned int i, optl, tcphdr_len, offset;
99d1712bc41c7c Florian Westphal   2017-08-08  229  	struct tcphdr *tcph;
99d1712bc41c7c Florian Westphal   2017-08-08  230  	u8 *opt;
99d1712bc41c7c Florian Westphal   2017-08-08  231  
99d1712bc41c7c Florian Westphal   2017-08-08  232  	tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff, &tcphdr_len);
99d1712bc41c7c Florian Westphal   2017-08-08  233  	if (!tcph)
99d1712bc41c7c Florian Westphal   2017-08-08  234  		return;
99d1712bc41c7c Florian Westphal   2017-08-08  235  
99d1712bc41c7c Florian Westphal   2017-08-08  236  	opt = (u8 *)tcph;
99d1712bc41c7c Florian Westphal   2017-08-08  237  	for (i = sizeof(*tcph); i < tcphdr_len - 1; i += optl) {
99d1712bc41c7c Florian Westphal   2017-08-08  238  		union {
99d1712bc41c7c Florian Westphal   2017-08-08  239  			__be16 v16;
99d1712bc41c7c Florian Westphal   2017-08-08  240  			__be32 v32;
99d1712bc41c7c Florian Westphal   2017-08-08  241  		} old, new;
99d1712bc41c7c Florian Westphal   2017-08-08  242  
99d1712bc41c7c Florian Westphal   2017-08-08  243  		optl = optlen(opt, i);
99d1712bc41c7c Florian Westphal   2017-08-08  244  
99d1712bc41c7c Florian Westphal   2017-08-08  245  		if (priv->type != opt[i])
99d1712bc41c7c Florian Westphal   2017-08-08  246  			continue;
99d1712bc41c7c Florian Westphal   2017-08-08  247  
99d1712bc41c7c Florian Westphal   2017-08-08  248  		if (i + optl > tcphdr_len || priv->len + priv->offset > optl)
99d1712bc41c7c Florian Westphal   2017-08-08  249  			return;
99d1712bc41c7c Florian Westphal   2017-08-08  250  
7418ee4c8810e4 Florian Westphal   2019-05-23  251  		if (skb_ensure_writable(pkt->skb,
7418ee4c8810e4 Florian Westphal   2019-05-23  252  					pkt->xt.thoff + i + priv->len))
99d1712bc41c7c Florian Westphal   2017-08-08  253  			return;
99d1712bc41c7c Florian Westphal   2017-08-08  254  
99d1712bc41c7c Florian Westphal   2017-08-08  255  		tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff,
99d1712bc41c7c Florian Westphal   2017-08-08  256  					      &tcphdr_len);
99d1712bc41c7c Florian Westphal   2017-08-08  257  		if (!tcph)
99d1712bc41c7c Florian Westphal   2017-08-08  258  			return;
99d1712bc41c7c Florian Westphal   2017-08-08  259  
99d1712bc41c7c Florian Westphal   2017-08-08  260  		offset = i + priv->offset;
99d1712bc41c7c Florian Westphal   2017-08-08  261  
99d1712bc41c7c Florian Westphal   2017-08-08  262  		switch (priv->len) {
99d1712bc41c7c Florian Westphal   2017-08-08  263  		case 2:
99d1712bc41c7c Florian Westphal   2017-08-08  264  			old.v16 = get_unaligned((u16 *)(opt + offset));
d9b9ce8e0720a5 Sergey Marinkevich 2020-03-29  265  			new.v16 = (__force __be16)nft_reg_load16(
d9b9ce8e0720a5 Sergey Marinkevich 2020-03-29  266  				&regs->data[priv->sreg]);
99d1712bc41c7c Florian Westphal   2017-08-08  267  
99d1712bc41c7c Florian Westphal   2017-08-08  268  			switch (priv->type) {
99d1712bc41c7c Florian Westphal   2017-08-08  269  			case TCPOPT_MSS:
99d1712bc41c7c Florian Westphal   2017-08-08  270  				/* increase can cause connection to stall */
99d1712bc41c7c Florian Westphal   2017-08-08  271  				if (ntohs(old.v16) <= ntohs(new.v16))
99d1712bc41c7c Florian Westphal   2017-08-08  272  					return;
99d1712bc41c7c Florian Westphal   2017-08-08  273  			break;
99d1712bc41c7c Florian Westphal   2017-08-08  274  			}
99d1712bc41c7c Florian Westphal   2017-08-08  275  
99d1712bc41c7c Florian Westphal   2017-08-08  276  			if (old.v16 == new.v16)
99d1712bc41c7c Florian Westphal   2017-08-08  277  				return;
99d1712bc41c7c Florian Westphal   2017-08-08  278  
99d1712bc41c7c Florian Westphal   2017-08-08  279  			put_unaligned(new.v16, (u16*)(opt + offset));
99d1712bc41c7c Florian Westphal   2017-08-08  280  			inet_proto_csum_replace2(&tcph->check, pkt->skb,
99d1712bc41c7c Florian Westphal   2017-08-08  281  						 old.v16, new.v16, false);
99d1712bc41c7c Florian Westphal   2017-08-08  282  			break;
99d1712bc41c7c Florian Westphal   2017-08-08  283  		case 4:
d9b9ce8e0720a5 Sergey Marinkevich 2020-03-29 @284  			new.v32 = regs->data[priv->sreg];
99d1712bc41c7c Florian Westphal   2017-08-08  285  			old.v32 = get_unaligned((u32 *)(opt + offset));
99d1712bc41c7c Florian Westphal   2017-08-08  286  
99d1712bc41c7c Florian Westphal   2017-08-08  287  			if (old.v32 == new.v32)
99d1712bc41c7c Florian Westphal   2017-08-08  288  				return;
99d1712bc41c7c Florian Westphal   2017-08-08  289  
99d1712bc41c7c Florian Westphal   2017-08-08  290  			put_unaligned(new.v32, (u32*)(opt + offset));
99d1712bc41c7c Florian Westphal   2017-08-08  291  			inet_proto_csum_replace4(&tcph->check, pkt->skb,
99d1712bc41c7c Florian Westphal   2017-08-08  292  						 old.v32, new.v32, false);
99d1712bc41c7c Florian Westphal   2017-08-08  293  			break;
99d1712bc41c7c Florian Westphal   2017-08-08  294  		default:
99d1712bc41c7c Florian Westphal   2017-08-08  295  			WARN_ON_ONCE(1);
99d1712bc41c7c Florian Westphal   2017-08-08  296  			break;
99d1712bc41c7c Florian Westphal   2017-08-08  297  		}
99d1712bc41c7c Florian Westphal   2017-08-08  298  
99d1712bc41c7c Florian Westphal   2017-08-08  299  		return;
99d1712bc41c7c Florian Westphal   2017-08-08  300  	}
99d1712bc41c7c Florian Westphal   2017-08-08  301  }
99d1712bc41c7c Florian Westphal   2017-08-08  302

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org