Skip to content

Commit

Permalink
datapath: Remove padding from packet before L3+ conntrack processing
Browse files Browse the repository at this point in the history
Upstream commit:
    commit 9382fe71c0058465e942a633869629929102843d
    Author: Ed Swierk <[email protected]>
    Date:   Wed Jan 31 18:48:02 2018 -0800

    openvswitch: Remove padding from packet before L3+ conntrack processing

    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 <[email protected]>
    Acked-by: Pravin B Shelar <[email protected]>
    Signed-off-by: David S. Miller <[email protected]>

Cc: Ed Swierk <[email protected]>
Signed-off-by: Greg Rose <[email protected]>
Acked-by: Pravin B Shelar <[email protected]>
  • Loading branch information
Ed Swierk authored and pshelar committed Feb 16, 2018
1 parent 5830593 commit c21adac
Showing 1 changed file with 34 additions and 0 deletions.
34 changes: 34 additions & 0 deletions datapath/conntrack.c
Original file line number Diff line number Diff line change
Expand Up @@ -1138,6 +1138,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.
*/
Expand All @@ -1152,6 +1182,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)
Expand Down

0 comments on commit c21adac

Please sign in to comment.