> -----Original Message----- > From: 高峰 [mailto:fgao@xxxxxxxxxx] > Sent: Tuesday, April 11, 2017 9:00 AM > To: 'Pablo Neira Ayuso' <pablo@xxxxxxxxxxxxx>; 'gfree.wind@xxxxxxxxxxx' > <gfree.wind@xxxxxxxxxxx> > Cc: 'netfilter-devel@xxxxxxxxxxxxxxx' <netfilter-devel@xxxxxxxxxxxxxxx> > Subject: RE: [PATCH nf 1/1] netfilter: seqadj: Fix possible non-linear data access > for TCP header > > Hi Pablo, > > > -----Original Message----- > > From: Pablo Neira Ayuso [mailto:pablo@xxxxxxxxxxxxx] > > Sent: Monday, April 10, 2017 8:07 PM > > To: gfree.wind@xxxxxxxxxxx > > Cc: netfilter-devel@xxxxxxxxxxxxxxx; Gao Feng <fgao@xxxxxxxxxx> > > Subject: Re: [PATCH nf 1/1] netfilter: seqadj: Fix possible non-linear > > data access for TCP header > > > > On Mon, Apr 10, 2017 at 06:36:03PM +0800, gfree.wind@xxxxxxxxxxx > wrote: > > > From: Gao Feng <fgao@xxxxxxxxxx> > > > > > > The current call path of nf_ct_tcp_seqadj_set is the following. > > > > > > nfqnl_recv_verdict->ctnetlink_glue_hook->ctnetlink_glue_seqadj > > > ->nf_ct_tcp_seqadj_set. > > > > > > It couldn't make sure the TCP header is in the linear data part. > > > So use the skb_header_pointer instead of the current codes. > > > > > > BTW, the nf_ct_tcp_seqadj_set is one external function of netfilter > > > which works in the network layer, it should not assume the transport > > > header is in the linear data. > > > > > > Signed-off-by: Gao Feng <fgao@xxxxxxxxxx> > > > --- > > > net/netfilter/nf_conntrack_seqadj.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/netfilter/nf_conntrack_seqadj.c > > > b/net/netfilter/nf_conntrack_seqadj.c > > > index ef7063e..80394ab 100644 > > > --- a/net/netfilter/nf_conntrack_seqadj.c > > > +++ b/net/netfilter/nf_conntrack_seqadj.c > > > @@ -61,11 +61,14 @@ void nf_ct_tcp_seqadj_set(struct sk_buff *skb, > > > s32 off) > > > { > > > const struct tcphdr *th; > > > + struct tcphdr tcph; > > > > > > if (nf_ct_protonum(ct) != IPPROTO_TCP) > > > return; > > > > > > - th = (struct tcphdr *)(skb_network_header(skb) + ip_hdrlen(skb)); > > > + th = skb_header_pointer(skb, ip_hdrlen(skb), sizeof(tcph), &tcph); > > > + if (!th) > > > + return; > > > > This fix is required since your recent upgrade to check for 4 bytes > > instead of 8 bytes from conntrack. Please confirm this. > > Do you mean the commit e5e693ab49a95e1994979972eea224eefa81eba9 > "netfilter: conntrack: Only need first 4 bytes to get l4proto ports"? > Sorry, I could not figure out why it would break the rule, and cause issue. > > Let's look at one change for TCP of that commit as following > > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c > b/net/netfilter/nf_conntrack_proto_tcp.c > index 70c8381..4abe9e1 100644 > --- a/net/netfilter/nf_conntrack_proto_tcp.c > +++ b/net/netfilter/nf_conntrack_proto_tcp.c > @@ -282,8 +282,8 @@ static bool tcp_pkt_to_tuple(const struct sk_buff *skb, > unsigned int dataoff, > const struct tcphdr *hp; > struct tcphdr _hdr; > > - /* Actually only need first 8 bytes. */ > - hp = skb_header_pointer(skb, dataoff, 8, &_hdr); > + /* Actually only need first 4 bytes to get ports. */ > + hp = skb_header_pointer(skb, dataoff, 4, &_hdr); > if (hp == NULL) > return false; > > The skb_header_pointer only copies the data, not make the data linear. > So I think it is ok after 4 bytes instead of 8 bytes. > It only affect the local variable, not the skb. > > > > > If so, it would be good to review the NAT code to see if there are > > more spots that are not broken since that change... > > I also check the other codes of Netfilter. > > grep -in skb_network_header * in nf/net/netfilter and nf/net/ipv4(6)/netfilter/ > nf_nat_helper.c:41: data = skb_network_header(skb) + dataoff; > nf_tables_core.c:99: ptr = skb_network_header(skb) + > pkt->xt.thoff; > xt_TCPMSS.c:104: tcph = (struct tcphdr *)(skb_network_header(skb) + > tcphoff); > xt_TCPMSS.c:163: tcph = (struct tcphdr > *)(skb_network_header(skb) + tcphoff); > xt_TCPOPTSTRIP.c:54: tcph = (struct tcphdr *)(skb_network_header(skb) + > tcphoff); > arpt_mangle.c:23: arpptr = skb_network_header(skb) + sizeof(*arp); > > all of them make sure the skb is linear, except the nf_tables_core.c which use > the skb_tail_pointer to exclude the non-linear data. > > So there is only nf_ct_tcp_seqadj_set which need fixed. > > Best Regards > Feng Sorry, please ignore this email. I reply the conversation with wrong email account which Outlook select it by default. I have replied again with the right email account. Best Regards Feng -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
