diff mbox

[PATCH/RFC,net-next,v2,1/4] flow dissector: return error on port dissection under-run

Message ID 1493988426-22854-2-git-send-email-simon.horman@netronome.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Simon Horman May 5, 2017, 12:47 p.m. UTC 
Return an error from __skb_flow_dissect() if insufficient packet data is
present when dissecting layer 4 ports.

Without this patch the absence of ports in truncated - e.g. UDP - packets
is treated the same way by the flow dissector as the presence of ports with
a value of zero. And without this patch the flower classifier is unable to
differentiate between these two cases which may lead to unexpected matching
of truncated packets.

With this patch the flow dissector and in turn the flower classifier can
differentiate between packets with zero L4 ports and truncated packets.

The approach taken here is to only return an error if the offset of ports
for the previously dissected IP protocol is known - a non error return from
proto_ports_offset() - but port data is not present in the packet - an
error return value from __skb_header_pointer().

The behaviour for callers of __skb_flow_get_ports() is changed but the only
callers are skb_flow_get_ports() and the flow dissector.  The former has
been updated so that its behaviour is unchanged.  Behavioural change of the
latter is the intended purpose of this patch but will only take effect with
a separate patch to have it refuse to match if dissection fails.

This change will lead to behavioural changes of the users of the dissector
with FLOW_DISSECTOR_KEY_PORTS - flower, and users of
flow_keys_dissector_keys[] and flow_keys_dissector_symmetric_keys[].  The
behavioural change for *_keys[] changes seem reasonable as the change will
should only be for truncated packets.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
---
rfc2
* Update changelog
---
 include/linux/skbuff.h    | 11 ++++++++---
 net/core/flow_dissector.c | 40 ++++++++++++++++++++++++++--------------
 2 files changed, 34 insertions(+), 17 deletions(-)

Comments

Jamal Hadi Salim May 8, 2017, 11:21 a.m. UTC | #1 
On 17-05-05 08:47 AM, Simon Horman wrote:
> Return an error from __skb_flow_dissect() if insufficient packet data is
> present when dissecting layer 4 ports.
>
> Without this patch the absence of ports in truncated - e.g. UDP - packets
> is treated the same way by the flow dissector as the presence of ports with
> a value of zero. And without this patch the flower classifier is unable to
> differentiate between these two cases which may lead to unexpected matching
> of truncated packets.
>
> With this patch the flow dissector and in turn the flower classifier can
> differentiate between packets with zero L4 ports and truncated packets.
>
> The approach taken here is to only return an error if the offset of ports
> for the previously dissected IP protocol is known - a non error return from
> proto_ports_offset() - but port data is not present in the packet - an
> error return value from __skb_header_pointer().
>
> The behaviour for callers of __skb_flow_get_ports() is changed but the only
> callers are skb_flow_get_ports() and the flow dissector.  The former has
> been updated so that its behaviour is unchanged.  Behavioural change of the
> latter is the intended purpose of this patch but will only take effect with
> a separate patch to have it refuse to match if dissection fails.
>
> This change will lead to behavioural changes of the users of the dissector
> with FLOW_DISSECTOR_KEY_PORTS - flower, and users of
> flow_keys_dissector_keys[] and flow_keys_dissector_symmetric_keys[].  The
> behavioural change for *_keys[] changes seem reasonable as the change will
> should only be for truncated packets.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
diff mbox

Patch

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a098d95b3d84..eb1b06dd0c8f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1108,13 +1108,18 @@  u32 __skb_get_hash_symmetric(const struct sk_buff *skb);
 u32 skb_get_poff(const struct sk_buff *skb);
 u32 __skb_get_poff(const struct sk_buff *skb, void *data,
 		   const struct flow_keys *keys, int hlen);
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen_proto);
+bool __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
+			  void *data, int hlen_proto, __be32 *ports);
 
 static inline __be32 skb_flow_get_ports(const struct sk_buff *skb,
 					int thoff, u8 ip_proto)
 {
-	return __skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0);
+	__be32 ports;
+
+	if (__skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0, &ports))
+		return ports;
+	else
+		return 0;
 }
 
 void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 28d94bce4df8..b3bf4886f71f 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -86,30 +86,41 @@  static __be16 skb_flow_get_be16(const struct sk_buff *skb, int poff,
  * @ip_proto: protocol for which to get port offset
  * @data: raw buffer pointer to the packet, if NULL use skb->data
  * @hlen: packet header length, if @data is NULL use skb_headlen(skb)
+ * @ports: pointer to return ports in
  *
  * The function will try to retrieve the ports at offset thoff + poff where poff
- * is the protocol port offset returned from proto_ports_offset
+ * is the protocol port offset returned from proto_ports_offset.
+ *
+ * Returns false on error, true otherwise.
  */
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen)
+bool __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
+			  void *data, int hlen, __be32 *ports)
 {
 	int poff = proto_ports_offset(ip_proto);
+	__be32 *p, _p;
+
+	/* proto_ports_offset returning an error indicates that ip_proto is
+	 * not known to have ports. This is not considered an error here.
+	 * Rather it is considered that the flow key of the caller may use
+	 * the default value of port fields: 0.
+	 */
+	if (poff < 0) {
+		*ports = 0;
+		return true;
+	}
 
 	if (!data) {
 		data = skb->data;
 		hlen = skb_headlen(skb);
 	}
 
-	if (poff >= 0) {
-		__be32 *ports, _ports;
+	p = __skb_header_pointer(skb, thoff + poff, sizeof(_p),
+				 data, hlen, &_p);
+	if (!p)
+		return false;
+	*ports = *p;
 
-		ports = __skb_header_pointer(skb, thoff + poff,
-					     sizeof(_ports), data, hlen, &_ports);
-		if (ports)
-			return *ports;
-	}
-
-	return 0;
+	return true;
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
@@ -692,8 +703,9 @@  bool __skb_flow_dissect(const struct sk_buff *skb,
 		key_ports = skb_flow_dissector_target(flow_dissector,
 						      FLOW_DISSECTOR_KEY_PORTS,
 						      target_container);
-		key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
-							data, hlen);
+		if (!__skb_flow_get_ports(skb, nhoff, ip_proto, data, hlen,
+					  &key_ports->ports))
+			goto out_bad;
 	}
 
 	if (dissector_uses_key(flow_dissector,