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: <f92d898f-f612-f17a-e3d7-ece8a9a19db3@skyportsystems.com>
Date:   Mon, 8 Jan 2018 19:02:56 -0800
From:   Ed Swierk <eswierk@...portsystems.com>
To:     Pravin Shelar <pshelar@....org>
Cc:     ovs-dev <ovs-dev@...nvswitch.org>, netdev <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 1/6/18 10:57, Pravin Shelar wrote:
> On Fri, Jan 5, 2018 at 10:59 PM, Ed Swierk <eswierk@...portsystems.com> wrote:
>>
>>
>> On Jan 5, 2018 22:17, "Pravin Shelar" <pshelar@....org> wrote:
>>
>> On Fri, Jan 5, 2018 at 3:20 PM, Ed Swierk <eswierk@...portsystems.com>
>> wrote:
>>> On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk@...portsystems.com>
>>> wrote:
>>>> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar@....org> wrote:
>>>>> 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?
>>>
>>> Something like this for verifying the IP header checksum (not tested):
>>>
>> AFAIU openflow does not need this verification, so it is not required
>> in flow extract.
>>
>>
>> Okay. How about my proposed trimming implementation, caching the pad length
>> in the ovs cb?
>>
> Caching the length is not that simple, OVS actions can change the
> length. Keeping it consistent with packet would be more work, so lets
> calculate it in ovs-ct function.
> 

Something like this?

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a38c80e..282325d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4084,6 +4084,8 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
 				     unsigned int transport_len,
 				     __sum16(*skb_chkf)(struct sk_buff *skb));
 
+int skb_network_trim(struct sk_buff *skb);
+
 /**
  * skb_head_is_locked - Determine if the skb->head is locked down
  * @skb: skb to check
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 08f5740..c68e927 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4740,6 +4740,41 @@ struct sk_buff *skb_checksum_trimmed(struct sk_buff *skb,
 }
 EXPORT_SYMBOL(skb_checksum_trimmed);
 
+/**
+ * skb_network_trim - trim skb to length specified by the network header
+ * @skb: the skb to trim
+ *
+ * Trims the skb to the length specified by the network header,
+ * removing any trailing padding. Leaves the skb alone if the protocol
+ * is not IP or IPv6. Frees the skb on error.
+ * 
+ * Caller needs to pull the skb to the network header.
+ */
+int 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 (unlikely(err))
+		kfree_skb(skb);
+
+	return err;
+}
+EXPORT_SYMBOL(skb_network_trim);
+
 void __skb_warn_lro_forwarding(const struct sk_buff *skb)
 {
 	net_warn_ratelimited("%s: received packets cannot be forwarded while LRO is enabled\n",
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index b27c5c6..73418d3 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1112,6 +1112,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 = 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)