[PATCH net 1/3] openvswitch: Reject ct_state masks for unknown bits

Currently, 0-bits are generated in ct_state where the bit position is
undefined, and matches are accepted on these bit-positions. If userspace
requests to match the 0-value for this bit then it may expect only a
subset of traffic to match this value, whereas currently all packets
will have this bit set to 0. Fix this by rejecting such masks.

Signed-off-by: Joe Stringer <redacted>
---
 net/openvswitch/conntrack.h    | 11 +++++------
 net/openvswitch/flow_netlink.c |  5 ++++-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
index da8714942c95..2d42b3640117 100644
--- a/net/openvswitch/conntrack.h
+++ b/net/openvswitch/conntrack.h

@@ -35,12 +35,9 @@ void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key);
 int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb);
 void ovs_ct_free_action(const struct nlattr *a);
 
-static inline bool ovs_ct_state_supported(u32 state)
-{
-	return !(state & ~(OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED |
-			 OVS_CS_F_RELATED | OVS_CS_F_REPLY_DIR |
-			 OVS_CS_F_INVALID | OVS_CS_F_TRACKED));
-}
+#define CT_SUPPORTED_MASK (OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED | \
+			   OVS_CS_F_RELATED | OVS_CS_F_REPLY_DIR | \
+			   OVS_CS_F_INVALID | OVS_CS_F_TRACKED)
 #else
 #include <linux/errno.h>
 

@@ -94,5 +91,7 @@ static inline int ovs_ct_put_key(const struct sw_flow_key *key,
 }
 
 static inline void ovs_ct_free_action(const struct nlattr *a) { }
+
+#define CT_SUPPORTED_MASK 0
 #endif /* CONFIG_NF_CONNTRACK */
 #endif /* ovs_conntrack.h */

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 171a691f1c32..bd710bc37469 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c

@@ -816,7 +816,7 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
 	    ovs_ct_verify(net, OVS_KEY_ATTR_CT_STATE)) {
 		u32 ct_state = nla_get_u32(a[OVS_KEY_ATTR_CT_STATE]);
 
-		if (!is_mask && !ovs_ct_state_supported(ct_state)) {
+		if (ct_state & ~CT_SUPPORTED_MASK) {
 			OVS_NLERR(log, "ct_state flags %08x unsupported",
 				  ct_state);
 			return -EINVAL;

@@ -1099,6 +1099,9 @@ static void nlattr_set(struct nlattr *attr, u8 val,
 		} else {
 			memset(nla_data(nla), val, nla_len(nla));
 		}
+
+		if (nla_type(nla) == OVS_KEY_ATTR_CT_STATE)
+			*(u32 *)nla_data(nla) &= CT_SUPPORTED_MASK;
 	}
 }
 

-- 
2.1.4

If OVS receives a packet from another namespace, then the packet should
be scrubbed. However, people have already begun to rely on the behaviour
that skb->mark is preserved across namespaces, so retain this one field.

This is mainly to address information leakage between namespaces when
using OVS internal ports, but by placing it in ovs_vport_receive() it is
more generally applicable, meaning it should not be overlooked if other
port types are allowed to be moved into namespaces in future.

Signed-off-by: Joe Stringer <redacted>
---
I originally proposed this patch as part of the conntrack changes to OVS,
and there was some discussion on that thread, culminating here:
http://www.spinics.net/lists/netdev/msg338626.html

We also discussed this a bit in Seattle, however I didn't follow up
immediately so I don't exactly recall what the consensus was. Following
Jesse's direction in the above thread, I'm proposing that we preserve the
mark, but scrub the rest. Also fixed the use-after-free bug present in the
previous version.

I think this is relevant for 'net', because this is the first time that
the metadata_dst and nfct are exposed (albeit indirectly) through OVS so it
would be nice to get agreement on the expected behaviour.
---
 net/openvswitch/vport.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index fc5c0b9ccfe9..70f19ea99b92 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c

@@ -440,10 +440,17 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
 		      const struct ip_tunnel_info *tun_info)
 {
 	struct sw_flow_key key;
+	u32 mark = skb->mark;
 	int error;
 
 	OVS_CB(skb)->input_vport = vport;
 	OVS_CB(skb)->mru = 0;
+	if (dev_net(skb->dev) != ovs_dp_get_net(vport->dp)) {
+		skb_scrub_packet(skb, true);
+		tun_info = NULL;
+	}
+	skb->mark = mark;
+
 	/* Extract flow from 'skb' into 'key'. */
 	error = ovs_flow_key_extract(tun_info, skb, &key);
 	if (unlikely(error)) {

-- 
2.1.4

New, related connections are marked as such as part of ovs_ct_lookup(),
but they are not marked as "new" if the commit flag is used. Make this
consistent by treating IP_CT_RELATED as new as well.

Reported-by: Jarno Rajahalme <redacted>
Signed-off-by: Joe Stringer <redacted>
---
 net/openvswitch/conntrack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 80bf702715bb..480dbb9095b7 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c

@@ -86,6 +86,8 @@ static u8 ovs_ct_get_state(enum ip_conntrack_info ctinfo)
 		ct_state |= OVS_CS_F_ESTABLISHED;
 		break;
 	case IP_CT_RELATED:
+		ct_state |= OVS_CS_F_NEW;
+		/* Fall through */
 	case IP_CT_RELATED_REPLY:
 		ct_state |= OVS_CS_F_RELATED;
 		break;

-- 
2.1.4

If userspace provides a ct action with no nested mark or label, then the
storage for these fields is zeroed. Later when actions are requested,
such zeroed fields are serialized even though userspace didn't
originally specify them. Fix the behaviour by ensuring that no action is
serialized in this case, and reject actions where userspace attempts to
set these fields with mask=0. This should make netlink marshalling
consistent across deserialization/reserialization.

Reported-by: Jarno Rajahalme <redacted>
Signed-off-by: Joe Stringer <redacted>
---
 net/openvswitch/conntrack.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 480dbb9095b7..ba29e6c2e0d4 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c

@@ -540,6 +540,16 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
 	return 0;
 }
 
+static bool label_zero(const struct ovs_key_ct_labels *labels)
+{
+	int i;
+
+	for (i = 0; i < sizeof(*labels); i++)
+		if (labels->ct_labels[i])
+			return false;
+	return true;
+}
+
 static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
 	[OVS_CT_ATTR_COMMIT]	= { .minlen = 0, .maxlen = 0 },
 	[OVS_CT_ATTR_ZONE]	= { .minlen = sizeof(u16),

@@ -589,6 +599,10 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
 		case OVS_CT_ATTR_MARK: {
 			struct md_mark *mark = nla_data(a);
 
+			if (!mark->mask) {
+				OVS_NLERR(log, "ct_mark mask cannot be 0");
+				return -EINVAL;
+			}
 			info->mark = *mark;
 			break;
 		}

@@ -597,6 +611,10 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
 		case OVS_CT_ATTR_LABELS: {
 			struct md_labels *labels = nla_data(a);
 
+			if (label_zero(&labels->mask)) {
+				OVS_NLERR(log, "ct_labels mask cannot be 0");
+				return -EINVAL;
+			}
 			info->labels = *labels;
 			break;
 		}

@@ -707,11 +725,12 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
 	if (IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES) &&
 	    nla_put_u16(skb, OVS_CT_ATTR_ZONE, ct_info->zone.id))
 		return -EMSGSIZE;
-	if (IS_ENABLED(CONFIG_NF_CONNTRACK_MARK) &&
+	if (IS_ENABLED(CONFIG_NF_CONNTRACK_MARK) && ct_info->mark.mask &&
 	    nla_put(skb, OVS_CT_ATTR_MARK, sizeof(ct_info->mark),
 		    &ct_info->mark))
 		return -EMSGSIZE;
 	if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
+	    !label_zero(&ct_info->labels.mask) &&
 	    nla_put(skb, OVS_CT_ATTR_LABELS, sizeof(ct_info->labels),
 		    &ct_info->labels))
 		return -EMSGSIZE;

-- 
2.1.4

On Wed, Oct 14, 2015 at 11:10 AM, Joe Stringer [off-list ref] wrote:

This should be marked as unlikely.

+               skb_scrub_packet(skb, true);
+               tun_info = NULL;
+       }
+       skb->mark = mark;

Lets move this to skb scrub block. in other cases this not required.

+
        /* Extract flow from 'skb' into 'key'. */
        error = ovs_flow_key_extract(tun_info, skb, &key);
        if (unlikely(error)) {
--
2.1.4

On Wed, Oct 14, 2015 at 11:10 AM, Joe Stringer [off-list ref] wrote:

Can you also remove definition of ovs_ct_state_supported() in case
where conntrack is not enabled.

Otherwise looks good.

Acked-by: Pravin B Shelar <redacted>

On Wed, Oct 14, 2015 at 11:10 AM, Joe Stringer [off-list ref] wrote:

New, related connections are marked as such as part of ovs_ct_lookup(),
but they are not marked as "new" if the commit flag is used. Make this
consistent by treating IP_CT_RELATED as new as well.

Reported-by: Jarno Rajahalme <redacted>
Signed-off-by: Joe Stringer <redacted>

Looks good.

Acked-by: Pravin B Shelar <redacted>

On Wed, Oct 14, 2015 at 11:10 AM, Joe Stringer [off-list ref] wrote:

There is already function called labels_nonzero(), This can be reused
for labels check.

After this flow install stage sanity check for labels, there is no
need for check in CONFIG_NF_CONNTRACK_LABELS action execution.

On 14 October 2015 at 17:34, Pravin Shelar [off-list ref] wrote:

On Wed, Oct 14, 2015 at 11:10 AM, Joe Stringer [off-list ref] wrote:

quoted

If OVS receives a packet from another namespace, then the packet should
be scrubbed. However, people have already begun to rely on the behaviour
that skb->mark is preserved across namespaces, so retain this one field.

This is mainly to address information leakage between namespaces when
using OVS internal ports, but by placing it in ovs_vport_receive() it is
more generally applicable, meaning it should not be overlooked if other
port types are allowed to be moved into namespaces in future.

Signed-off-by: Joe Stringer <redacted>
---
I originally proposed this patch as part of the conntrack changes to OVS,
and there was some discussion on that thread, culminating here:
http://www.spinics.net/lists/netdev/msg338626.html

We also discussed this a bit in Seattle, however I didn't follow up
immediately so I don't exactly recall what the consensus was. Following
Jesse's direction in the above thread, I'm proposing that we preserve the
mark, but scrub the rest. Also fixed the use-after-free bug present in the
previous version.

I think this is relevant for 'net', because this is the first time that
the metadata_dst and nfct are exposed (albeit indirectly) through OVS so it
would be nice to get agreement on the expected behaviour.
---
 net/openvswitch/vport.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index fc5c0b9ccfe9..70f19ea99b92 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c

@@ -440,10 +440,17 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
                      const struct ip_tunnel_info *tun_info)
 {
        struct sw_flow_key key;
+       u32 mark = skb->mark;
        int error;

        OVS_CB(skb)->input_vport = vport;
        OVS_CB(skb)->mru = 0;
+       if (dev_net(skb->dev) != ovs_dp_get_net(vport->dp)) {

This should be marked as unlikely.

OK.

quoted

+               skb_scrub_packet(skb, true);
+               tun_info = NULL;
+       }
+       skb->mark = mark;

Lets move this to skb scrub block. in other cases this not required.

OK, I'll send a v2.

On 15 October 2015 at 11:13, Pravin Shelar [off-list ref] wrote:

On Wed, Oct 14, 2015 at 11:10 AM, Joe Stringer [off-list ref] wrote:

quoted

If userspace provides a ct action with no nested mark or label, then the
storage for these fields is zeroed. Later when actions are requested,
such zeroed fields are serialized even though userspace didn't
originally specify them. Fix the behaviour by ensuring that no action is
serialized in this case, and reject actions where userspace attempts to
set these fields with mask=0. This should make netlink marshalling
consistent across deserialization/reserialization.

Reported-by: Jarno Rajahalme <redacted>
Signed-off-by: Joe Stringer <redacted>
---
 net/openvswitch/conntrack.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 480dbb9095b7..ba29e6c2e0d4 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c

@@ -540,6 +540,16 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
        return 0;
 }

+static bool label_zero(const struct ovs_key_ct_labels *labels)
+{
+       int i;
+
+       for (i = 0; i < sizeof(*labels); i++)
+               if (labels->ct_labels[i])
+                       return false;
+       return true;
+}
+

There is already function called labels_nonzero(), This can be reused
for labels check.

quoted

 static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
        [OVS_CT_ATTR_COMMIT]    = { .minlen = 0, .maxlen = 0 },
        [OVS_CT_ATTR_ZONE]      = { .minlen = sizeof(u16),

@@ -589,6 +599,10 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
                case OVS_CT_ATTR_MARK: {
                        struct md_mark *mark = nla_data(a);

+                       if (!mark->mask) {
+                               OVS_NLERR(log, "ct_mark mask cannot be 0");
+                               return -EINVAL;
+                       }
                        info->mark = *mark;
                        break;
                }

@@ -597,6 +611,10 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
                case OVS_CT_ATTR_LABELS: {
                        struct md_labels *labels = nla_data(a);

+                       if (label_zero(&labels->mask)) {
+                               OVS_NLERR(log, "ct_labels mask cannot be 0");
+                               return -EINVAL;
+                       }

After this flow install stage sanity check for labels, there is no
need for check in CONFIG_NF_CONNTRACK_LABELS action execution.

OK, thanks for the review. I'll fix these up.