| Message ID | 1549631724-20326-1-git-send-email-alin.nastac@technicolor.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: reject: skip csum verification for protocols that don't support it | expand |
Alin Nastac <alin.nastac@gmail.com> wrote: > From: Alin Nastac <alin.nastac@gmail.com> > > Some protocols have other means to verify the payload integrity > (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum > implementation because checksum is either optional or might be > partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used > to validate the packets, ip(6)tables REJECT rules were not capable > to generate ICMP(v6) errors for the protocols mentioned above. > > This commit also fixes the incorrect pseudo-header protocol used > for IPv4 packets that carry other transport protocols than TCP or > UDP (pseudo-header used protocol 0 iso the proper value). > > Signed-off-by: Alin Nastac <alin.nastac@gmail.com> Acked-by: Florian Westphal <fw@strlen.de>
On Fri, Feb 08, 2019 at 02:15:24PM +0100, Alin Nastac wrote: > From: Alin Nastac <alin.nastac@gmail.com> > > Some protocols have other means to verify the payload integrity > (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum > implementation because checksum is either optional or might be > partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used > to validate the packets, ip(6)tables REJECT rules were not capable > to generate ICMP(v6) errors for the protocols mentioned above. > > This commit also fixes the incorrect pseudo-header protocol used > for IPv4 packets that carry other transport protocols than TCP or > UDP (pseudo-header used protocol 0 iso the proper value). Applied, thanks Alin.
Hi Alin, On Tue, Feb 12, 2019 at 12:45:51AM +0100, Pablo Neira Ayuso wrote: > On Fri, Feb 08, 2019 at 02:15:24PM +0100, Alin Nastac wrote: > > From: Alin Nastac <alin.nastac@gmail.com> > > > > Some protocols have other means to verify the payload integrity > > (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum > > implementation because checksum is either optional or might be > > partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used > > to validate the packets, ip(6)tables REJECT rules were not capable > > to generate ICMP(v6) errors for the protocols mentioned above. > > > > This commit also fixes the incorrect pseudo-header protocol used > > for IPv4 packets that carry other transport protocols than TCP or > > UDP (pseudo-header used protocol 0 iso the proper value). Sorry, I just realized that we are not updating: net/bridge/netfilter/nft_reject_bridge.c Probably we can place this: + proto = iph->protocol; + switch (proto) { + /* Protocols with other integrity checks. */ + case IPPROTO_AH: + case IPPROTO_ESP: + case IPPROTO_SCTP: + + /* Protocols with partial checksums. */ + case IPPROTO_UDPLITE: + case IPPROTO_DCCP: + + /* Protocols with optional checksums. */ + case IPPROTO_GRE: + goto send_unreach; } into an inline function in include/net/netfilter/nf_reject.h and use it from these three spots? Thanks!
Hi Pablo, On Tue, Feb 12, 2019 at 12:50 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Hi Alin, > > On Tue, Feb 12, 2019 at 12:45:51AM +0100, Pablo Neira Ayuso wrote: > > On Fri, Feb 08, 2019 at 02:15:24PM +0100, Alin Nastac wrote: > > > From: Alin Nastac <alin.nastac@gmail.com> > > > > > > Some protocols have other means to verify the payload integrity > > > (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum > > > implementation because checksum is either optional or might be > > > partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used > > > to validate the packets, ip(6)tables REJECT rules were not capable > > > to generate ICMP(v6) errors for the protocols mentioned above. > > > > > > This commit also fixes the incorrect pseudo-header protocol used > > > for IPv4 packets that carry other transport protocols than TCP or > > > UDP (pseudo-header used protocol 0 iso the proper value). > > Sorry, I just realized that we are not updating: > > net/bridge/netfilter/nft_reject_bridge.c > > Probably we can place this: > > + proto = iph->protocol; > + switch (proto) { > + /* Protocols with other integrity checks. */ > + case IPPROTO_AH: > + case IPPROTO_ESP: > + case IPPROTO_SCTP: > + > + /* Protocols with partial checksums. */ > + case IPPROTO_UDPLITE: > + case IPPROTO_DCCP: > + > + /* Protocols with optional checksums. */ > + case IPPROTO_GRE: > + goto send_unreach; > } > > into an inline function in include/net/netfilter/nf_reject.h and use > it from these three spots? The pseudo-header proto=0 issue must also be addressed in net/bridge/netfilter/nft_reject_bridge.c. I see you haven't pushed yet my commit. Do you want me to issue the 2nd version of this patch?
Hi Alin, On Tue, Feb 12, 2019 at 07:25:29AM +0100, Alin Năstac wrote: > The pseudo-header proto=0 issue must also be addressed in > net/bridge/netfilter/nft_reject_bridge.c. > > I see you haven't pushed yet my commit. Do you want me to issue the > 2nd version of this patch? You choose: 1) I push out your original patch and I wait for a follow up patch to adjust nft_reject_bridge.c 2) I wait for a v2 that fixes things in one go. Let me know your preference. Thanks.
Hi Pablo, On Tue, Feb 12, 2019 at 11:20 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Hi Alin, > > On Tue, Feb 12, 2019 at 07:25:29AM +0100, Alin Năstac wrote: > > The pseudo-header proto=0 issue must also be addressed in > > net/bridge/netfilter/nft_reject_bridge.c. > > > > I see you haven't pushed yet my commit. Do you want me to issue the > > 2nd version of this patch? > > You choose: > > 1) I push out your original patch and I wait for a follow up patch to > adjust nft_reject_bridge.c > > 2) I wait for a v2 that fixes things in one go. > > Let me know your preference. I will send you later today the v2 version of this patch.
Hi Pablo,
On Tue, Feb 12, 2019 at 11:21 AM Alin Năstac <alin.nastac@gmail.com> wrote:
> I will send you later today the v2 version of this patch.
I have problems with the inline function defined in
include/net/netfilter/nft_reject.h. Mu CONFIG_NF_TABLES is disabled
and I get the following error when I try to include it in
nf_reject_ipv{4,6}.c:
include/net/netfilter/nf_tables.h:898:12: error: 'const struct net'
has no member named 'nft'
return net->nft.gencursor + 1 == 1 ? 1 : 0;
Should I create a new include/net/netfilter/nf_reject.h file and
include it in both include/net/netfilter/ipv{4,6}/nf_reject.h?
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c index aa8304c..9225b7f 100644 --- a/net/ipv4/netfilter/nf_reject_ipv4.c +++ b/net/ipv4/netfilter/nf_reject_ipv4.c @@ -178,18 +178,33 @@ void nf_send_unreach(struct sk_buff *skb_in, int code, int hook) if (iph->frag_off & htons(IP_OFFSET)) return; - if (skb_csum_unnecessary(skb_in)) { - icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0); - return; + if (skb_csum_unnecessary(skb_in)) + goto send_unreach; + + /* Skip checksum verification for protocols that don't use + * 16-bit one's complement checksum of the entire payload. + */ + proto = iph->protocol; + switch (proto) { + /* Protocols with other integrity checks. */ + case IPPROTO_AH: + case IPPROTO_ESP: + case IPPROTO_SCTP: + + /* Protocols with partial checksums. */ + case IPPROTO_UDPLITE: + case IPPROTO_DCCP: + + /* Protocols with optional checksums. */ + case IPPROTO_GRE: + goto send_unreach; } - if (iph->protocol == IPPROTO_TCP || iph->protocol == IPPROTO_UDP) - proto = iph->protocol; - else - proto = 0; + if (nf_ip_checksum(skb_in, hook, ip_hdrlen(skb_in), proto)) + return; - if (nf_ip_checksum(skb_in, hook, ip_hdrlen(skb_in), proto) == 0) - icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0); + send_unreach: + icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0); } EXPORT_SYMBOL_GPL(nf_send_unreach); diff --git a/net/ipv6/netfilter/nf_reject_ipv6.c b/net/ipv6/netfilter/nf_reject_ipv6.c index b9c8a76..2af014e 100644 --- a/net/ipv6/netfilter/nf_reject_ipv6.c +++ b/net/ipv6/netfilter/nf_reject_ipv6.c @@ -233,6 +233,24 @@ static bool reject6_csum_ok(struct sk_buff *skb, int hook) if (thoff < 0 || thoff >= skb->len || (fo & htons(~0x7)) != 0) return false; + /* Skip protocols that don't use 16-bit one's complement checksum + * of the entire payload. + */ + switch (proto) { + /* Protocols with other integrity checks. */ + case IPPROTO_AH: + case IPPROTO_ESP: + case IPPROTO_SCTP: + + /* Protocols with partial checksums. */ + case IPPROTO_UDPLITE: + case IPPROTO_DCCP: + + /* Protocols with optional checksums. */ + case IPPROTO_GRE: + return true; + } + return nf_ip6_checksum(skb, hook, thoff, proto) == 0; }