Re: [PATCH v4] openvswitch: Remove padding from packet before L3+ conntrack processing

[David Miller]

From: Ed Swierk 
Date: Wed, 31 Jan 2018 18:48:02 -0800

> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
> included in the L3 length. For example, a short IPv4 packet may have
> up to 6 bytes of padding following the IP payload when received on an
> Ethernet device with a minimum packet length of 64 bytes.
> 
> Higher-layer processing functions in netfilter (e.g. nf_ip_checksum(),
> and help() in nf_conntrack_ftp) assume skb->len reflects the length of
> the L3 header and payload, rather than referring back to
> ip_hdr->tot_len or ipv6_hdr->payload_len, and get confused by
> lower-layer padding.
> 
> In the normal IPv4 receive path, ip_rcv() trims the packet to
> ip_hdr->tot_len before invoking netfilter hooks. In the IPv6 receive
> path, ip6_rcv() does the same using ipv6_hdr->payload_len. Similarly
> in the br_netfilter receive path, br_validate_ipv4() and
> br_validate_ipv6() trim the packet to the L3 length before invoking
> netfilter hooks.
> 
> Currently in the OVS conntrack receive path, ovs_ct_execute() pulls
> the skb to the L3 header but does not trim it to the L3 length before
> calling nf_conntrack_in(NF_INET_PRE_ROUTING). When
> nf_conntrack_proto_tcp encounters a packet with lower-layer padding,
> nf_ip_checksum() fails causing a "nf_ct_tcp: bad TCP checksum" log
> message. While extra zero bytes don't affect the checksum, the length
> in the IP pseudoheader does. That length is based on skb->len, and
> without trimming, it doesn't match the length the sender used when
> computing the checksum.
> 
> In ovs_ct_execute(), trim the skb to the L3 length before higher-layer
> processing.
> 
> Signed-off-by: Ed Swierk 

Applied, thank you Ed.

Re: [PATCH v4] openvswitch: Remove padding from packet before L3+ conntrack processing

[Pravin Shelar]

On Wed, Jan 31, 2018 at 6:48 PM, Ed Swierk  wrote:
> IPv4 and IPv6 packets may arrive with lower-layer padding that is not
> included in the L3 length. For example, a short IPv4 packet may have
> up to 6 bytes of padding following the IP payload when received on an
> Ethernet device with a minimum packet length of 64 bytes.
>
> Higher-layer processing functions in netfilter (e.g. nf_ip_checksum(),
> and help() in nf_conntrack_ftp) assume skb->len reflects the length of
> the L3 header and payload, rather than referring back to
> ip_hdr->tot_len or ipv6_hdr->payload_len, and get confused by
> lower-layer padding.
>
> In the normal IPv4 receive path, ip_rcv() trims the packet to
> ip_hdr->tot_len before invoking netfilter hooks. In the IPv6 receive
> path, ip6_rcv() does the same using ipv6_hdr->payload_len. Similarly
> in the br_netfilter receive path, br_validate_ipv4() and
> br_validate_ipv6() trim the packet to the L3 length before invoking
> netfilter hooks.
>
> Currently in the OVS conntrack receive path, ovs_ct_execute() pulls
> the skb to the L3 header but does not trim it to the L3 length before
> calling nf_conntrack_in(NF_INET_PRE_ROUTING). When
> nf_conntrack_proto_tcp encounters a packet with lower-layer padding,
> nf_ip_checksum() fails causing a "nf_ct_tcp: bad TCP checksum" log
> message. While extra zero bytes don't affect the checksum, the length
> in the IP pseudoheader does. That length is based on skb->len, and
> without trimming, it doesn't match the length the sender used when
> computing the checksum.
>
> In ovs_ct_execute(), trim the skb to the L3 length before higher-layer
> processing.
>
> Signed-off-by: Ed Swierk 

Acked-by: Pravin B Shelar 

[PATCH v4] openvswitch: Remove padding from packet before L3+ conntrack processing

[Ed Swierk]

IPv4 and IPv6 packets may arrive with lower-layer padding that is not
included in the L3 length. For example, a short IPv4 packet may have
up to 6 bytes of padding following the IP payload when received on an
Ethernet device with a minimum packet length of 64 bytes.

Higher-layer processing functions in netfilter (e.g. nf_ip_checksum(),
and help() in nf_conntrack_ftp) assume skb->len reflects the length of
the L3 header and payload, rather than referring back to
ip_hdr->tot_len or ipv6_hdr->payload_len, and get confused by
lower-layer padding.

In the normal IPv4 receive path, ip_rcv() trims the packet to
ip_hdr->tot_len before invoking netfilter hooks. In the IPv6 receive
path, ip6_rcv() does the same using ipv6_hdr->payload_len. Similarly
in the br_netfilter receive path, br_validate_ipv4() and
br_validate_ipv6() trim the packet to the L3 length before invoking
netfilter hooks.

Currently in the OVS conntrack receive path, ovs_ct_execute() pulls
the skb to the L3 header but does not trim it to the L3 length before
calling nf_conntrack_in(NF_INET_PRE_ROUTING). When
nf_conntrack_proto_tcp encounters a packet with lower-layer padding,
nf_ip_checksum() fails causing a "nf_ct_tcp: bad TCP checksum" log
message. While extra zero bytes don't affect the checksum, the length
in the IP pseudoheader does. That length is based on skb->len, and
without trimming, it doesn't match the length the sender used when
computing the checksum.

In ovs_ct_execute(), trim the skb to the L3 length before higher-layer
processing.

Signed-off-by: Ed Swierk 
---
 net/openvswitch/conntrack.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index d558e88..285f879 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1097,6 +1097,36 @@ static int ovs_ct_commit(struct net *net, struct 
sw_flow_key *key,
return 0;
 }
 
+/* Trim the skb to the length specified by the IP/IPv6 header,
+ * removing any trailing lower-layer padding. This prepares the skb
+ * for higher-layer processing that assumes skb->len excludes padding
+ * (such as nf_ip_checksum). The caller needs to pull the skb to the
+ * network header, and ensure ip_hdr/ipv6_hdr points to valid data.
+ */
+static int ovs_skb_network_trim(struct sk_buff *skb)
+{
+   unsigned int len;
+   int err;
+
+   switch (skb->protocol) {
+   case htons(ETH_P_IP):
+   len = ntohs(ip_hdr(skb)->tot_len);
+   break;
+   case htons(ETH_P_IPV6):
+   len = sizeof(struct ipv6hdr)
+   + ntohs(ipv6_hdr(skb)->payload_len);
+   break;
+   default:
+   len = skb->len;
+   }
+
+   err = pskb_trim_rcsum(skb, len);
+   if (err)
+   kfree_skb(skb);
+
+   return err;
+}
+
 /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
  * value if 'skb' is freed.
  */
@@ -,6 +1141,10 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
nh_ofs = skb_network_offset(skb);
skb_pull_rcsum(skb, nh_ofs);
 
+   err = ovs_skb_network_trim(skb);
+   if (err)
+   return err;
+
if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
err = handle_fragments(net, key, info->zone.id, skb);
if (err)
-- 
1.9.1