* [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum @ 2019-03-03 8:17 Xin Long 2019-03-08 15:50 ` Pablo Neira Ayuso 0 siblings, 1 reply; 8+ messages in thread From: Xin Long @ 2019-03-03 8:17 UTC (permalink / raw) To: network dev, netfilter-devel; +Cc: Marcelo Ricardo Leitner, Neil Horman, pablo sctp_hdr(skb) only works when skb->transport_header is set properly. But in the path of nf_conntrack_in: sctp_packet() -> sctp_error() -> sctp_compute_cksum(). skb->transport_header is not guaranteed to be right value for sctp. It will cause to fail to check the checksum for sctp packets. So fix it by setting skb transport_header before calling sctp_compute_cksum(). Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code") Reported-by: Li Shuang <shuali@redhat.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> Acked-by: Neil Horman <nhorman@tuxdriver.com> --- net/netfilter/nf_conntrack_proto_sctp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c index d53e3e7..6b53cd2 100644 --- a/net/netfilter/nf_conntrack_proto_sctp.c +++ b/net/netfilter/nf_conntrack_proto_sctp.c @@ -343,7 +343,9 @@ static bool sctp_error(struct sk_buff *skb, logmsg = "nf_ct_sctp: failed to read header "; goto out_invalid; } - sh = (const struct sctphdr *)(skb->data + dataoff); + /* sctp_compute_cksum() depends on correct transport header */ + skb_set_transport_header(skb, dataoff); + sh = sctp_hdr(skb); if (sh->checksum != sctp_compute_cksum(skb, dataoff)) { logmsg = "nf_ct_sctp: bad CRC "; goto out_invalid; -- 2.1.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum 2019-03-03 8:17 [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum Xin Long @ 2019-03-08 15:50 ` Pablo Neira Ayuso 2019-03-09 9:07 ` Xin Long 0 siblings, 1 reply; 8+ messages in thread From: Pablo Neira Ayuso @ 2019-03-08 15:50 UTC (permalink / raw) To: Xin Long Cc: network dev, netfilter-devel, Marcelo Ricardo Leitner, Neil Horman Hi, On Sun, Mar 03, 2019 at 04:17:21PM +0800, Xin Long wrote: > sctp_hdr(skb) only works when skb->transport_header is set > properly. > > But in the path of nf_conntrack_in: > > sctp_packet() -> sctp_error() -> sctp_compute_cksum(). > > skb->transport_header is not guaranteed to be right value > for sctp. It will cause to fail to check the checksum for > sctp packets. > > So fix it by setting skb transport_header before calling > sctp_compute_cksum(). I see a few more calls to sctp_compute_cksum() in the netfilter tree. I guess they are broken too. In netfilter, skb->transport_header is never set from the input path, I think this introduces an assymmetry with other transport protocols. May we have a variant of sctp_compute_cksum() which does not rely on sctp_hdr() instead? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum 2019-03-08 15:50 ` Pablo Neira Ayuso @ 2019-03-09 9:07 ` Xin Long 2019-03-09 9:24 ` Florian Westphal 0 siblings, 1 reply; 8+ messages in thread From: Xin Long @ 2019-03-09 9:07 UTC (permalink / raw) To: Pablo Neira Ayuso, Neil Horman Cc: network dev, netfilter-devel, Marcelo Ricardo Leitner On Fri, Mar 8, 2019 at 11:50 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Hi, > > On Sun, Mar 03, 2019 at 04:17:21PM +0800, Xin Long wrote: > > sctp_hdr(skb) only works when skb->transport_header is set > > properly. > > > > But in the path of nf_conntrack_in: > > > > sctp_packet() -> sctp_error() -> sctp_compute_cksum(). > > > > skb->transport_header is not guaranteed to be right value > > for sctp. It will cause to fail to check the checksum for > > sctp packets. > > > > So fix it by setting skb transport_header before calling > > sctp_compute_cksum(). > > I see a few more calls to sctp_compute_cksum() in the netfilter tree. > I guess they are broken too. > > In netfilter, skb->transport_header is never set from the input path, > I think this introduces an assymmetry with other transport protocols. > > May we have a variant of sctp_compute_cksum() which does not rely on > sctp_hdr() instead? I posted one before this: https://marc.info/?l=linux-netdev&m=155109395226858&w=2 But from sctp side, Neil preferred sctp_hdr(). We need to either add skb_set_transport_header() in sctp_s/dnat_handler() and sctp_manip_pkt(), or bring that patch back? Now it seems not good to set skb->transport_header in netfilter code. Hi Neil, what's your point now? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum 2019-03-09 9:07 ` Xin Long @ 2019-03-09 9:24 ` Florian Westphal 2019-03-11 11:07 ` Neil Horman 0 siblings, 1 reply; 8+ messages in thread From: Florian Westphal @ 2019-03-09 9:24 UTC (permalink / raw) To: Xin Long Cc: Pablo Neira Ayuso, Neil Horman, network dev, netfilter-devel, Marcelo Ricardo Leitner Xin Long <lucien.xin@gmail.com> wrote: > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 > But from sctp side, Neil preferred sctp_hdr(). > > We need to either add skb_set_transport_header() in sctp_s/dnat_handler() > and sctp_manip_pkt(), or bring that patch back? > > Now it seems not good to set skb->transport_header in netfilter code. I think its fine, but I wonder why we need to do it. Since 21d1196a35f5686c4323e42a62fdb4b23b0ab4a3 ipv4 input path sets transport header before netfilter. The only problem is that linear access is illegal without may_pull checks, but in this case the make_writable call takes care of this already. So, why was this patch needed? If we need it, do we also need to add it in other locations that deal with sctp csum (e.g. in ipvs?). Thanks, Florian ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum 2019-03-09 9:24 ` Florian Westphal @ 2019-03-11 11:07 ` Neil Horman 2019-03-12 8:39 ` Xin Long 0 siblings, 1 reply; 8+ messages in thread From: Neil Horman @ 2019-03-11 11:07 UTC (permalink / raw) To: Florian Westphal Cc: Xin Long, Pablo Neira Ayuso, network dev, netfilter-devel, Marcelo Ricardo Leitner On Sat, Mar 09, 2019 at 10:24:34AM +0100, Florian Westphal wrote: > Xin Long <lucien.xin@gmail.com> wrote: > > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 > > But from sctp side, Neil preferred sctp_hdr(). > > > > We need to either add skb_set_transport_header() in sctp_s/dnat_handler() > > and sctp_manip_pkt(), or bring that patch back? > > > > Now it seems not good to set skb->transport_header in netfilter code. > > I think its fine, but I wonder why we need to do it. > > Since 21d1196a35f5686c4323e42a62fdb4b23b0ab4a3 ipv4 input path sets > transport header before netfilter. The only problem is that linear > access is illegal without may_pull checks, but in this case the > make_writable call takes care of this already. > Yes, this. It seems to me we should be setting the transport header prior to ever getting into the netfilter code, which does imply that we need the may_pull check to linearize enough of the packet to do so, just like tcp and udp do. > So, why was this patch needed? > If we need it, do we also need to add it in other locations that > deal with sctp csum (e.g. in ipvs?). > This is a fair question, and I'm not yet sure of the answer. > Thanks, > Florian > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum 2019-03-11 11:07 ` Neil Horman @ 2019-03-12 8:39 ` Xin Long 2019-03-12 9:48 ` Pablo Neira Ayuso 0 siblings, 1 reply; 8+ messages in thread From: Xin Long @ 2019-03-12 8:39 UTC (permalink / raw) To: Neil Horman Cc: Florian Westphal, Pablo Neira Ayuso, network dev, netfilter-devel, Marcelo Ricardo Leitner On Mon, Mar 11, 2019 at 7:08 PM Neil Horman <nhorman@tuxdriver.com> wrote: > > On Sat, Mar 09, 2019 at 10:24:34AM +0100, Florian Westphal wrote: > > Xin Long <lucien.xin@gmail.com> wrote: > > > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 > > > But from sctp side, Neil preferred sctp_hdr(). > > > > > > We need to either add skb_set_transport_header() in sctp_s/dnat_handler() > > > and sctp_manip_pkt(), or bring that patch back? > > > > > > Now it seems not good to set skb->transport_header in netfilter code. > > > > I think its fine, but I wonder why we need to do it. > > > > Since 21d1196a35f5686c4323e42a62fdb4b23b0ab4a3 ipv4 input path sets > > transport header before netfilter. The only problem is that linear > > access is illegal without may_pull checks, but in this case the > > make_writable call takes care of this already. > > > Yes, this. It seems to me we should be setting the transport header prior to > ever getting into the netfilter code, which does imply that we need the may_pull > check to linearize enough of the packet to do so, just like tcp and udp do. > > > So, why was this patch needed? The issue was reported when going to nf_conntrack by br_netfilter's bridge-nf-call-iptables, which could be: br_prerouting->inet_prerouting-> br_forward->inet_forward-> br_postrouting->inet_postrouting > > If we need it, do we also need to add it in other locations that > > deal with sctp csum (e.g. in ipvs?). So ipvs hooks are in inet_local_in/out, sctp_s/dnat_handler() won't be affected. But the one in sctp_manip_pkt() that may be in inet_pre/postrouting will need to set it. I will post v2 with skb_set_transport_header(skb, dataoff) added in sctp_manip_pkt(). agreed? > > > This is a fair question, and I'm not yet sure of the answer. > > > Thanks, > > Florian > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum 2019-03-12 8:39 ` Xin Long @ 2019-03-12 9:48 ` Pablo Neira Ayuso 2019-03-12 11:01 ` Xin Long 0 siblings, 1 reply; 8+ messages in thread From: Pablo Neira Ayuso @ 2019-03-12 9:48 UTC (permalink / raw) To: Xin Long Cc: Neil Horman, Florian Westphal, network dev, netfilter-devel, Marcelo Ricardo Leitner On Tue, Mar 12, 2019 at 04:39:46PM +0800, Xin Long wrote: > On Mon, Mar 11, 2019 at 7:08 PM Neil Horman <nhorman@tuxdriver.com> wrote: > > > > On Sat, Mar 09, 2019 at 10:24:34AM +0100, Florian Westphal wrote: > > > Xin Long <lucien.xin@gmail.com> wrote: > > > > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 > > > > But from sctp side, Neil preferred sctp_hdr(). > > > > > > > > We need to either add skb_set_transport_header() in sctp_s/dnat_handler() > > > > and sctp_manip_pkt(), or bring that patch back? > > > > > > > > Now it seems not good to set skb->transport_header in netfilter code. > > > > > > I think its fine, but I wonder why we need to do it. > > > > > > Since 21d1196a35f5686c4323e42a62fdb4b23b0ab4a3 ipv4 input path sets > > > transport header before netfilter. The only problem is that linear > > > access is illegal without may_pull checks, but in this case the > > > make_writable call takes care of this already. > > > > > Yes, this. It seems to me we should be setting the transport header prior to > > ever getting into the netfilter code, which does imply that we need the may_pull > > check to linearize enough of the packet to do so, just like tcp and udp do. > > > > > So, why was this patch needed? > > The issue was reported when going to nf_conntrack by br_netfilter's > bridge-nf-call-iptables, which could be: > > br_prerouting->inet_prerouting-> > br_forward->inet_forward-> > br_postrouting->inet_postrouting Can you fix this from br_netfilter then? ie. set the transport header before prerouting to emulate the IP stack behaviour. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum 2019-03-12 9:48 ` Pablo Neira Ayuso @ 2019-03-12 11:01 ` Xin Long 0 siblings, 0 replies; 8+ messages in thread From: Xin Long @ 2019-03-12 11:01 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: Neil Horman, Florian Westphal, network dev, netfilter-devel, Marcelo Ricardo Leitner On Tue, Mar 12, 2019 at 5:49 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Mar 12, 2019 at 04:39:46PM +0800, Xin Long wrote: > > On Mon, Mar 11, 2019 at 7:08 PM Neil Horman <nhorman@tuxdriver.com> wrote: > > > > > > On Sat, Mar 09, 2019 at 10:24:34AM +0100, Florian Westphal wrote: > > > > Xin Long <lucien.xin@gmail.com> wrote: > > > > > https://marc.info/?l=linux-netdev&m=155109395226858&w=2 > > > > > But from sctp side, Neil preferred sctp_hdr(). > > > > > > > > > > We need to either add skb_set_transport_header() in sctp_s/dnat_handler() > > > > > and sctp_manip_pkt(), or bring that patch back? > > > > > > > > > > Now it seems not good to set skb->transport_header in netfilter code. > > > > > > > > I think its fine, but I wonder why we need to do it. > > > > > > > > Since 21d1196a35f5686c4323e42a62fdb4b23b0ab4a3 ipv4 input path sets > > > > transport header before netfilter. The only problem is that linear > > > > access is illegal without may_pull checks, but in this case the > > > > make_writable call takes care of this already. > > > > > > > Yes, this. It seems to me we should be setting the transport header prior to > > > ever getting into the netfilter code, which does imply that we need the may_pull > > > check to linearize enough of the packet to do so, just like tcp and udp do. > > > > > > > So, why was this patch needed? > > > > The issue was reported when going to nf_conntrack by br_netfilter's > > bridge-nf-call-iptables, which could be: > > > > br_prerouting->inet_prerouting-> > > br_forward->inet_forward-> > > br_postrouting->inet_postrouting > > Can you fix this from br_netfilter then? ie. set the transport header > before prerouting to emulate the IP stack behaviour. diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c index 9d34de6..22afa56 100644 --- a/net/bridge/br_netfilter_hooks.c +++ b/net/bridge/br_netfilter_hooks.c @@ -502,6 +502,7 @@ static unsigned int br_nf_pre_routing(void *priv, nf_bridge->ipv4_daddr = ip_hdr(skb)->daddr; skb->protocol = htons(ETH_P_IP); + skb->transport_header = skb->network_header + ip_hdr(skb)->ihl * 4; NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING, state->net, state->sk, skb, skb->dev, NULL, diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c index 564710f..e88d664 100644 --- a/net/bridge/br_netfilter_ipv6.c +++ b/net/bridge/br_netfilter_ipv6.c @@ -235,6 +235,8 @@ unsigned int br_nf_pre_routing_ipv6(void *priv, nf_bridge->ipv6_daddr = ipv6_hdr(skb)->daddr; skb->protocol = htons(ETH_P_IPV6); + skb->transport_header = skb->network_header + sizeof(struct ipv6hdr); + NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING, state->net, state->sk, skb, skb->dev, NULL, br_nf_pre_routing_finish_ipv6); Looks more reasonable, it's also safe after br_validate_ipv4/6(). Thanks. ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-03-12 11:01 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-03 8:17 [PATCH net] netfilter: set skb transport_header before calling sctp_compute_cksum Xin Long 2019-03-08 15:50 ` Pablo Neira Ayuso 2019-03-09 9:07 ` Xin Long 2019-03-09 9:24 ` Florian Westphal 2019-03-11 11:07 ` Neil Horman 2019-03-12 8:39 ` Xin Long 2019-03-12 9:48 ` Pablo Neira Ayuso 2019-03-12 11:01 ` Xin Long
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).