| Message ID | 740683890e28e93c1b2e940a28089ca10f006b7f.1551601041.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net] netfilter: set skb transport_header before calling sctp_compute_cksum | expand |
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?
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?
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
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 >
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 > >
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.
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.
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;
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(-)