| Message ID | 1561346258-272481-1-git-send-email-zhe.he@windriver.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: Fix remainder of pseudo-header protocol 0 | expand |
On Mon, Jun 24, 2019 at 11:17:38AM +0800, zhe.he@windriver.com wrote: > From: He Zhe <zhe.he@windriver.com> > > Since v5.1-rc1, some types of packets do not get unreachable reply with the > following iptables setting. Fox example, > > $ iptables -A INPUT -p icmp --icmp-type 8 -j REJECT > $ ping 127.0.0.1 -c 1 > PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data. > — 127.0.0.1 ping statistics — > 1 packets transmitted, 0 received, 100% packet loss, time 0ms > > We should have got the following reply from command line, but we did not. > From 127.0.0.1 icmp_seq=1 Destination Port Unreachable > > Yi Zhao reported it and narrowed it down to: > 7fc38225363d ("netfilter: reject: skip csum verification for protocols that don't support it"), > > This is because nf_ip_checksum still expects pseudo-header protocol type 0 for > packets that are of neither TCP or UDP, and thus ICMP packets are mistakenly > treated as TCP/UDP. > > This patch corrects the conditions in nf_ip_checksum and all other places that > still call it with protocol 0. Looking at 7fc38225363dd8f19e667ad7c77b63bc4a5c065d, I wonder this can be fixed while simplifying it... I think nf_reject_verify_csum() is useless? In your patch, now you explicitly check for IPPROTO_TCP and IPPROTO_UDP to validate the checksum.
On 6/28/19 2:49 AM, Pablo Neira Ayuso wrote: > On Mon, Jun 24, 2019 at 11:17:38AM +0800, zhe.he@windriver.com wrote: >> From: He Zhe <zhe.he@windriver.com> >> >> Since v5.1-rc1, some types of packets do not get unreachable reply with the >> following iptables setting. Fox example, >> >> $ iptables -A INPUT -p icmp --icmp-type 8 -j REJECT >> $ ping 127.0.0.1 -c 1 >> PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data. >> — 127.0.0.1 ping statistics — >> 1 packets transmitted, 0 received, 100% packet loss, time 0ms >> >> We should have got the following reply from command line, but we did not. >> From 127.0.0.1 icmp_seq=1 Destination Port Unreachable >> >> Yi Zhao reported it and narrowed it down to: >> 7fc38225363d ("netfilter: reject: skip csum verification for protocols that don't support it"), >> >> This is because nf_ip_checksum still expects pseudo-header protocol type 0 for >> packets that are of neither TCP or UDP, and thus ICMP packets are mistakenly >> treated as TCP/UDP. >> >> This patch corrects the conditions in nf_ip_checksum and all other places that >> still call it with protocol 0. > Looking at 7fc38225363dd8f19e667ad7c77b63bc4a5c065d, I wonder this can > be fixed while simplifying it... > > I think nf_reject_verify_csum() is useless? > > In your patch, now you explicitly check for IPPROTO_TCP and > IPPROTO_UDP to validate the checksum. Thanks for your review. I suppose the two main points of 7fc38225363d are valid and I was trying to align with them and fix them: 1) Skip csum verification for protocols that don't support it. 2) Remove the protocol 0 used to indicate non-TCP/UDP packets, and use actual types instead to be clear. 1) uses nf_reject_verify_csum to skip those that should be skipped and leaves the protocols that support csum to the rest of the logic including nf_ip_checksum. But 2) removes the "0" transition from the rest of the logic and thus causes this issue. So I add the explicit check against TCP/UDP to nf_ip_checksum. And nf_reject_verify_csum is still useful. Zhe >
On Mon, Jun 24, 2019 at 11:17:38AM +0800, zhe.he@windriver.com wrote: > From: He Zhe <zhe.he@windriver.com> > > Since v5.1-rc1, some types of packets do not get unreachable reply with the > following iptables setting. Fox example, > > $ iptables -A INPUT -p icmp --icmp-type 8 -j REJECT > $ ping 127.0.0.1 -c 1 > PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data. > — 127.0.0.1 ping statistics — > 1 packets transmitted, 0 received, 100% packet loss, time 0ms > > We should have got the following reply from command line, but we did not. > From 127.0.0.1 icmp_seq=1 Destination Port Unreachable > > Yi Zhao reported it and narrowed it down to: > 7fc38225363d ("netfilter: reject: skip csum verification for protocols that don't support it"), > > This is because nf_ip_checksum still expects pseudo-header protocol type 0 for > packets that are of neither TCP or UDP, and thus ICMP packets are mistakenly > treated as TCP/UDP. > > This patch corrects the conditions in nf_ip_checksum and all other places that > still call it with protocol 0. Applied, thanks.
diff --git a/net/netfilter/nf_conntrack_proto_icmp.c b/net/netfilter/nf_conntrack_proto_icmp.c index a824367..dd53e2b 100644 --- a/net/netfilter/nf_conntrack_proto_icmp.c +++ b/net/netfilter/nf_conntrack_proto_icmp.c @@ -218,7 +218,7 @@ int nf_conntrack_icmpv4_error(struct nf_conn *tmpl, /* See ip_conntrack_proto_tcp.c */ if (state->net->ct.sysctl_checksum && state->hook == NF_INET_PRE_ROUTING && - nf_ip_checksum(skb, state->hook, dataoff, 0)) { + nf_ip_checksum(skb, state->hook, dataoff, IPPROTO_ICMP)) { icmp_error_log(skb, state, "bad hw icmp checksum"); return -NF_ACCEPT; } diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c index 07da077..83a24cc 100644 --- a/net/netfilter/nf_nat_proto.c +++ b/net/netfilter/nf_nat_proto.c @@ -564,7 +564,7 @@ int nf_nat_icmp_reply_translation(struct sk_buff *skb, if (!skb_make_writable(skb, hdrlen + sizeof(*inside))) return 0; - if (nf_ip_checksum(skb, hooknum, hdrlen, 0)) + if (nf_ip_checksum(skb, hooknum, hdrlen, IPPROTO_ICMP)) return 0; inside = (void *)skb->data + hdrlen; diff --git a/net/netfilter/utils.c b/net/netfilter/utils.c index 06dc555..51b454d 100644 --- a/net/netfilter/utils.c +++ b/net/netfilter/utils.c @@ -17,7 +17,8 @@ __sum16 nf_ip_checksum(struct sk_buff *skb, unsigned int hook, case CHECKSUM_COMPLETE: if (hook != NF_INET_PRE_ROUTING && hook != NF_INET_LOCAL_IN) break; - if ((protocol == 0 && !csum_fold(skb->csum)) || + if ((protocol != IPPROTO_TCP && protocol != IPPROTO_UDP && + !csum_fold(skb->csum)) || !csum_tcpudp_magic(iph->saddr, iph->daddr, skb->len - dataoff, protocol, skb->csum)) { @@ -26,7 +27,7 @@ __sum16 nf_ip_checksum(struct sk_buff *skb, unsigned int hook, } /* fall through */ case CHECKSUM_NONE: - if (protocol == 0) + if (protocol != IPPROTO_TCP && protocol != IPPROTO_UDP) skb->csum = 0; else skb->csum = csum_tcpudp_nofold(iph->saddr, iph->daddr,