netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).