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

[Ed Swierk <eswierk@skyportsystems.com>]

On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Wed, Jan 3, 2018 at 7:49 PM, Ed Swierk <eswierk@skyportsystems.com> wrote:
>> On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>>> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk@skyportsystems.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@skyportsystems.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.

Shouldn't the IP header checksum be verified even earlier, like in
key_extract(), before actually using any of the fields in the IP
header?

And since key_extract() is already inspecting the IP/IPv6 header, it
would be a convenient spot to check whether the skb->len matches. If
there's a difference, it could record the number of bytes to trim in
an ovs_skb_cb field. Then ovs_ct_execute() would look at this field
and trim the skb only if necessary.