[PATCHv2 net] openvswitch: Scrub skb between namespaces

[PATCHv2 net] openvswitch: Scrub skb between namespaces

[Joe Stringer]

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 <joestringer@nicira.com>
---
v2: Add unlikely(), shift all within the netns check block.
---
 net/openvswitch/vport.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index fc5c0b9ccfe9..12a36ac21eda 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -444,6 +444,15 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
 
 	OVS_CB(skb)->input_vport = vport;
 	OVS_CB(skb)->mru = 0;
+	if (unlikely(dev_net(skb->dev) != ovs_dp_get_net(vport->dp))) {
+		u32 mark;
+
+		mark = skb->mark;
+		skb_scrub_packet(skb, true);
+		skb->mark = mark;
+		tun_info = NULL;
+	}
+
 	/* Extract flow from 'skb' into 'key'. */
 	error = ovs_flow_key_extract(tun_info, skb, &key);
 	if (unlikely(error)) {
-- 
2.1.4

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

[Joe Stringer]

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 <joestringer@nicira.com>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
v2: Remove extraneous ovs_ct_supported() function declaration.
    Acked.
---
 net/openvswitch/conntrack.h    | 16 +++++-----------
 net/openvswitch/flow_netlink.c |  5 ++++-
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
index da8714942c95..82e0dfc66028 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>
 
@@ -53,11 +50,6 @@ static inline bool ovs_ct_verify(struct net *net, int attr)
 	return false;
 }
 
-static inline bool ovs_ct_state_supported(u32 state)
-{
-	return false;
-}
-
 static inline int ovs_ct_copy_action(struct net *net, const struct nlattr *nla,
 				     const struct sw_flow_key *key,
 				     struct sw_flow_actions **acts, bool log)
@@ -94,5 +86,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

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

[Thomas Graf]

On 10/16/15 at 11:08am, Joe Stringer wrote:
> 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 <joestringer@nicira.com>
> Acked-by: Pravin B Shelar <pshelar@nicira.com>

Acked-by: Thomas Graf <tgraf@suug.ch>

[PATCHv2 net 2/3] openvswitch: Treat IP_CT_RELATED as new

[Joe Stringer]

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 <jrajahalme@nicira.com>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
v2: Acked.
---
 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

Re: [PATCHv2 net 2/3] openvswitch: Treat IP_CT_RELATED as new

[Thomas Graf]

On 10/16/15 at 11:08am, Joe Stringer 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 <jrajahalme@nicira.com>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> Acked-by: Pravin B Shelar <pshelar@nicira.com>
> ---
> v2: Acked.
> ---
>  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;

I'm probably missing something obvious. Why is the reply direction
not considered NEW? Wouldn't this consider an ICMPv6 as related+new
depending on simply the direction?

Re: [PATCHv2 net 2/3] openvswitch: Treat IP_CT_RELATED as new

[Joe Stringer]

On 17 October 2015 at 00:52, Thomas Graf <tgraf@suug.ch> wrote:
> On 10/16/15 at 11:08am, Joe Stringer 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 <jrajahalme@nicira.com>
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>> Acked-by: Pravin B Shelar <pshelar@nicira.com>
>> ---
>> v2: Acked.
>> ---
>>  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;
>
> I'm probably missing something obvious. Why is the reply direction
> not considered NEW? Wouldn't this consider an ICMPv6 as related+new
> depending on simply the direction?

My thoughts were along the lines "If something is a reply, that
implies that state is held, and therefore it cannot be NEW (where NEW
means no state is available)". However, if you consider that the
'related' connection is an independent connection with its own state,
but the 'reply' bit refers to the original connection, my original
premise breaks. Furthermore, looking at how it's used in netfilter
core and the ICMP proto handler, it looks like both of these cases
should be considered NEW. I can respin.

Do you have a specific case in mind here? It would be useful for
extending the OVS testsuite.

Re: [PATCHv2 net 2/3] openvswitch: Treat IP_CT_RELATED as new

[Thomas Graf]

On 10/19/15 at 12:07am, Joe Stringer wrote:
> > I'm probably missing something obvious. Why is the reply direction
> > not considered NEW? Wouldn't this consider an ICMPv6 as related+new
> > depending on simply the direction?
> 
> My thoughts were along the lines "If something is a reply, that
> implies that state is held, and therefore it cannot be NEW (where NEW
> means no state is available)". However, if you consider that the
> 'related' connection is an independent connection with its own state,
> but the 'reply' bit refers to the original connection, my original
> premise breaks. Furthermore, looking at how it's used in netfilter
> core and the ICMP proto handler, it looks like both of these cases
> should be considered NEW. I can respin.
> 
> Do you have a specific case in mind here? It would be useful for
> extending the OVS testsuite.

It's tricky. A typical use case would be an active FTP connection
where the data connection is established in the reply direction
and marked related if I'm not mistaken.

OTOH, an ICMP sent in response should not be considered NEW. It
really depends on our definition of NEW towards the user.

Re: [PATCHv2 net 2/3] openvswitch: Treat IP_CT_RELATED as new

[Joe Stringer]

On 19 October 2015 at 02:03, Thomas Graf <tgraf@suug.ch> wrote:
> On 10/19/15 at 12:07am, Joe Stringer wrote:
>> > I'm probably missing something obvious. Why is the reply direction
>> > not considered NEW? Wouldn't this consider an ICMPv6 as related+new
>> > depending on simply the direction?
>>
>> My thoughts were along the lines "If something is a reply, that
>> implies that state is held, and therefore it cannot be NEW (where NEW
>> means no state is available)". However, if you consider that the
>> 'related' connection is an independent connection with its own state,
>> but the 'reply' bit refers to the original connection, my original
>> premise breaks. Furthermore, looking at how it's used in netfilter
>> core and the ICMP proto handler, it looks like both of these cases
>> should be considered NEW. I can respin.
>>
>> Do you have a specific case in mind here? It would be useful for
>> extending the OVS testsuite.
>
> It's tricky. A typical use case would be an active FTP connection
> where the data connection is established in the reply direction
> and marked related if I'm not mistaken.
>
> OTOH, an ICMP sent in response should not be considered NEW. It
> really depends on our definition of NEW towards the user.

ICMP and FTP are handled a bit differently: ICMP protocol handling is
based on the original connection; however for expected connections
like FTP data, there is a separate conntrack entry. I agree that for
ICMP errors, they should not be NEW; but for active FTP connections,
I'd expect the data connection to be NEW (if it wasn't already
seen&established). Along these lines, my earlier assertion that the
'reply' flag is based on the original connection is only applicable
for ICMP responses, but not for FTP data connections.

I think that the proper solution instead of this patch is to set NEW
if !nf_ct_is_confirmed(ct). This is more accurately the meaning for
'NEW' that we are actually trying to expose. As long as this is done
before confirming the connection during a "commit", this will make the
behaviour consistent with the lack of commit flag. I'm sending a
patch. Thanks for the feedback!

Re: [PATCHv2 net 2/3] openvswitch: Treat IP_CT_RELATED as new

[Thomas Graf]

On 10/19/15 at 04:13pm, Joe Stringer wrote:
> I think that the proper solution instead of this patch is to set NEW
> if !nf_ct_is_confirmed(ct). This is more accurately the meaning for
> 'NEW' that we are actually trying to expose. As long as this is done
> before confirming the connection during a "commit", this will make the
> behaviour consistent with the lack of commit flag. I'm sending a
> patch. Thanks for the feedback!

Agreed. This sounds perfect.

[PATCHv2 net 3/3] openvswitch: Serialize nested ct actions if provided

[Joe Stringer]

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 <jrajahalme@nicira.com>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
v2: Reuse labels_nonzero().
    Don't check for labels support during execution.
---
 net/openvswitch/conntrack.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 480dbb9095b7..b0287f13491a 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -224,9 +224,6 @@ static int ovs_ct_set_labels(struct sk_buff *skb, struct sw_flow_key *key,
 	struct nf_conn *ct;
 	int err;
 
-	if (!IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS))
-		return -ENOTSUPP;
-
 	/* The connection could be invalid, in which case set_label is no-op.*/
 	ct = nf_ct_get(skb, &ctinfo);
 	if (!ct)
@@ -589,6 +586,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 +598,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 (!labels_nonzero(&labels->mask)) {
+				OVS_NLERR(log, "ct_labels mask cannot be 0");
+				return -EINVAL;
+			}
 			info->labels = *labels;
 			break;
 		}
@@ -707,11 +712,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) &&
+	    labels_nonzero(&ct_info->labels.mask) &&
 	    nla_put(skb, OVS_CT_ATTR_LABELS, sizeof(ct_info->labels),
 		    &ct_info->labels))
 		return -EMSGSIZE;
-- 
2.1.4

Re: [PATCHv2 net 3/3] openvswitch: Serialize nested ct actions if provided

[Pravin Shelar]

On Fri, Oct 16, 2015 at 11:08 AM, Joe Stringer <joestringer@nicira.com> wrote:
> 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 <jrajahalme@nicira.com>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCHv2 net 3/3] openvswitch: Serialize nested ct actions if provided

[Thomas Graf]

On 10/16/15 at 11:08am, Joe Stringer wrote:
> 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 <jrajahalme@nicira.com>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Acked-by: Thomas Graf <tgraf@suug.ch>

Re: [PATCHv2 net] openvswitch: Scrub skb between namespaces

[Pravin Shelar]

On Fri, Oct 16, 2015 at 11:08 AM, Joe Stringer <joestringer@nicira.com> wrote:
> 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 <joestringer@nicira.com>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCHv2 net] openvswitch: Scrub skb between namespaces

[Thomas Graf]

On 10/16/15 at 11:08am, Joe Stringer wrote:
> 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 <joestringer@nicira.com>

Perfect, thanks!

Acked-by: Thomas Graf <tgraf@suug.ch>

Re: [PATCHv2 net] openvswitch: Scrub skb between namespaces

[David Miller]

From: Joe Stringer <joestringer@nicira.com>
Date: Fri, 16 Oct 2015 11:08:18 -0700

> 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 <joestringer@nicira.com>

Applied, thanks.