netdev - Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOrHB_D1mj_0O+_e3+GxxcTiuk0B5w5cc6qf90jM+fGiw8oAhw@mail.gmail.com>
Date:   Thu, 4 Jan 2018 19:36:38 -0800
From:   Pravin Shelar <pshelar@....org>
To:     Ed Swierk <eswierk@...portsystems.com>
Cc:     ovs-dev <ovs-dev@...nvswitch.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Benjamin Warren <ben@...portsystems.com>,
        Keith Holleman <holleman@...portsystems.com>
Subject: Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing

On Wed, Jan 3, 2018 at 7:49 PM, Ed Swierk <eswierk@...portsystems.com> wrote:
> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@....org> wrote:
>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@...portsystems.com> 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. In the normal IPv4 receive path, ip_rcv() trims the
>>> packet to ip_hdr->tot_len before invoking netfilter hooks (including
>>> conntrack and nat).
>>>
>>> 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 NF_INET_PRE_ROUTING hooks.
>>>
>>> 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_checksum() fails and
>>> logs "nf_ct_tcp: bad TCP checksum". 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.
>>>
>>> The assumption throughout nf_conntrack and nf_nat is that skb->len
>>> reflects the length of the L3 header and payload, so there is no need
>>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len.
>>>
>>> This change brings OVS into line with other netfilter users, trimming
>>> IPv4 and IPv6 packets prior to L3+ netfilter processing.
>>>
>>> Signed-off-by: Ed Swierk <eswierk@...portsystems.com>
>>> ---
>>> v2:
>>> - Trim packet in nat receive path as well as conntrack
>>> - Free skb on error
>>> ---
>>>  net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 34 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index b27c5c6..1bdc78f 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net,
>>>         return ct_executed;
>>>  }
>>>
>>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to
>>> + * the L3 header. The skb is freed on error.
>>> + */
>>> +static int skb_trim_l3(struct sk_buff *skb)
>>> +{
>>> +       unsigned int nh_len;
>>> +       int err;
>>> +
>>> +       switch (skb->protocol) {
>>> +       case htons(ETH_P_IP):
>>> +               nh_len = ntohs(ip_hdr(skb)->tot_len);
>>> +               break;
>>> +       case htons(ETH_P_IPV6):
>>> +               nh_len = ntohs(ipv6_hdr(skb)->payload_len)
>>> +                       + sizeof(struct ipv6hdr);
>>> +               break;
>>> +       default:
>>> +               nh_len = skb->len;
>>> +       }
>>> +
>>> +       err = pskb_trim_rcsum(skb, nh_len);
>>> +       if (err)
>> This should is unlikely.
>>> +               kfree_skb(skb);
>>> +
>>> +       return err;
>>> +}
>>> +
>> This looks like a generic function, it probably does not belong to OVS
>> code base.
>
> It occurs to me that skb_trim_l3() can't just reach into ip_hdr(skb)
> before calling pskb_may_pull(skb, sizeof(struct iphdr)) to make sure
> the IP header is actually there; and for IPv4 it should validate the
> IP header checksum, including options. Once we add all these steps,
> skb_trim_l3() starts to look an awful lot like br_validate_ipv4() and
> br_validate_ipv6(). And those in turn are eerily similar to ip_rcv()
> and ip6_rcv(). It would be nice to avoid duplicating this logic yet
> again.
>
OVS already pull all required headers in skb linear data, so no need
to redo all of it. only check required is the ip-checksum validation.
I think we could avoid it in most of cases by checking skb length to
ipheader length before verifying the ip header-checksum.