netdev - Re: [PATCH net-next] openvswitch: Only set mark and labels when commiting a connection.

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: <E5FA6787-E964-4878-B11C-E0461F555E37@ovn.org>
Date:	Mon, 20 Jun 2016 17:24:05 -0700
From:	Jarno Rajahalme <jarno@....org>
To:	Netdev <netdev@...r.kernel.org>
Cc:	dev@...nvswitch.org
Subject: Re: [PATCH net-next] openvswitch: Only set mark and labels when commiting a connection.

The title should have been:

openvswitch: Only set mark and labels with a commit flag.

This reflects the fact that modifying the mark and/or labels of an existing connection is allowed. The commit flag is still required to do that, though.

  Jarno

> On Jun 20, 2016, at 5:19 PM, Jarno Rajahalme <jarno@....org> wrote:
> 
> Only allow setting conntrack mark or labels when the commit flag is
> specified.  This makes sure we can not set them before the connection
> has been persisted, as in that case the mark and labels would be lost
> in an event of an userspace upcall.
> 
> OVS userspace already requires the commit flag to accept setting
> ct_mark and/or ct_labels.  Validate for this on the kernel API.
> 
> Finally, set conntrack mark and labels right before committing so that
> the initial conntrack NEW event has the mark and labels.
> 
> Signed-off-by: Jarno Rajahalme <jarno@....org>
> ---
> net/openvswitch/conntrack.c | 72 ++++++++++++++++++++++++++++++---------------
> 1 file changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 3d5feed..f1612f5 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -824,6 +824,17 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
> 	return 0;
> }
> 
> +static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < sizeof(*labels); i++)
> +		if (labels->ct_labels[i])
> +			return true;
> +
> +	return false;
> +}
> +
> /* Lookup connection and confirm if unconfirmed. */
> static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
> 			 const struct ovs_conntrack_info *info,
> @@ -834,24 +845,35 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
> 	err = __ovs_ct_lookup(net, key, info, skb);
> 	if (err)
> 		return err;
> -	/* This is a no-op if the connection has already been confirmed. */
> +
> +	/* As any changes to an unconfirmed connection may be lost due
> +	 * to an upcall, we require the presence of the 'commit' flag
> +	 * for setting mask and/or labels.  Perform the changes before
> +	 * confirming the connection so that the initial mark and label
> +	 * values are present in the initial CT NEW netlink event.
> +	 */
> +	if (info->mark.mask) {
> +		err = ovs_ct_set_mark(skb, key, info->mark.value,
> +				      info->mark.mask);
> +		if (err)
> +			return err;
> +	}
> +	if (labels_nonzero(&info->labels.mask)) {
> +		err = ovs_ct_set_labels(skb, key, &info->labels.value,
> +					&info->labels.mask);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* Will take care of sending queued events even if the connection is
> +	 * already confirmed.
> +	 */
> 	if (nf_conntrack_confirm(skb) != NF_ACCEPT)
> 		return -EINVAL;
> 
> 	return 0;
> }
> 
> -static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
> -{
> -	size_t i;
> -
> -	for (i = 0; i < sizeof(*labels); i++)
> -		if (labels->ct_labels[i])
> -			return true;
> -
> -	return false;
> -}
> -
> /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
>  * value if 'skb' is freed.
>  */
> @@ -876,19 +898,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> 		err = ovs_ct_commit(net, key, info, skb);
> 	else
> 		err = ovs_ct_lookup(net, key, info, skb);
> -	if (err)
> -		goto err;
> 
> -	if (info->mark.mask) {
> -		err = ovs_ct_set_mark(skb, key, info->mark.value,
> -				      info->mark.mask);
> -		if (err)
> -			goto err;
> -	}
> -	if (labels_nonzero(&info->labels.mask))
> -		err = ovs_ct_set_labels(skb, key, &info->labels.value,
> -					&info->labels.mask);
> -err:
> 	skb_push(skb, nh_ofs);
> 	if (err)
> 		kfree_skb(skb);
> @@ -1145,6 +1155,20 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
> 		}
> 	}
> 
> +#ifdef CONFIG_NF_CONNTRACK_MARK
> +	if (!info->commit && info->mark.mask) {
> +		OVS_NLERR(log,
> +			  "Setting conntrack mark requires 'commit' flag.");
> +		return -EINVAL;
> +	}
> +#endif
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	if (!info->commit && labels_nonzero(&info->labels.mask)) {
> +		OVS_NLERR(log,
> +			  "Setting conntrack labels requires 'commit' flag.");
> +		return -EINVAL;
> +	}
> +#endif
> 	if (rem > 0) {
> 		OVS_NLERR(log, "Conntrack attr has %d unknown bytes", rem);
> 		return -EINVAL;
> -- 
> 2.1.4
>