Using masked key for validate_and_copy_actions() won't be safe against
partially masked fields.
It may not be a problem in practice, but it is technically possible with
the netlink messages.
Would you please add a error log before returning EINVAL.
I like the API clean ups. It makes the code easier to follow.
Thanks,
--andy
On Mon, Jul 15, 2013 at 10:51 PM, Jesse Gross <je...@nicira.com> wrote:
> It is important to validate flow actions to ensure that they do
> not try to write off the end of a packet. The mechanism to do this
> is to ensure that a flow is precise enough to describe valid vs.
> invalid packets and only allowing actions on valid flows.
>
> The introduction of megaflows broke this by using a narrow base
> flow but a potentially wide match. This meant that while the
> original flow was properly validated, later packets might not
> conform to that flow and could be truncated. This switches to
> using the masked flow instead, effectively requiring that all
> possible matching packets be valid in order for a flow's actions
> to be accepted.
>
> This change only affects the flow setup path - executed packets
> have always used the flow extracted from the packet and therefore
> were properly validated.
>
> Signed-off-by: Jesse Gross <je...@nicira.com>
> ---
> datapath/datapath.c | 11 ++++++++---
> datapath/flow.c | 12 ++++--------
> datapath/flow.h | 5 +++--
> 3 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index eb07a89..8c9b7f7 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1251,7 +1251,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff
> *skb, struct genl_info *info)
> {
> struct nlattr **a = info->attrs;
> struct ovs_header *ovs_header = info->userhdr;
> - struct sw_flow_key key;
> + struct sw_flow_key key, masked_key;
> struct sw_flow *flow = NULL;
> struct sw_flow_mask mask;
> struct sk_buff *reply;
> @@ -1279,7 +1279,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff
> *skb, struct genl_info *info)
> if (IS_ERR(acts))
> goto error;
>
> - error =
> validate_and_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &key, 0, &acts);
> + ovs_flow_key_mask(&masked_key, &key, &mask);
> + error = validate_and_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
> + &masked_key, 0, &acts);
> if (error)
> goto err_kfree;
> } else if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW) {
> @@ -1324,6 +1326,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff
> *skb, struct genl_info *info)
> }
> clear_stats(flow);
>
> + flow->key = masked_key;
> + flow->unmasked_key = key;
> +
> /* Make sure mask is unique in the system */
> mask_p = ovs_sw_flow_mask_find(table, &mask);
> if (!mask_p) {
> @@ -1341,7 +1346,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff
> *skb, struct genl_info *info)
> rcu_assign_pointer(flow->sf_acts, acts);
>
> /* Put flow in bucket. */
> - ovs_flow_insert(table, flow, &key, match.range.end);
> + ovs_flow_insert(table, flow);
>
> reply = ovs_flow_cmd_build_info(flow, dp, info->snd_portid,
> info->snd_seq,
> OVS_FLOW_CMD_NEW);
> diff --git a/datapath/flow.c b/datapath/flow.c
> index be69186..752c8d6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -349,9 +349,8 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
> sizeof(struct icmp6hdr));
> }
>
> -static void flow_key_mask(struct sw_flow_key *dst,
> - const struct sw_flow_key *src,
> - const struct sw_flow_mask *mask)
> +void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key
> *src,
> + const struct sw_flow_mask *mask)
> {
> u8 *m = (u8 *)&mask->key + mask->range.start;
> u8 *s = (u8 *)src + mask->range.start;
> @@ -1045,7 +1044,7 @@ static struct sw_flow *ovs_masked_flow_lookup(struct
> flow_table *table,
> u32 hash;
> struct sw_flow_key masked_key;
>
> - flow_key_mask(&masked_key, flow_key, mask);
> + ovs_flow_key_mask(&masked_key, flow_key, mask);
> hash = ovs_flow_hash(&masked_key, key_start, key_len);
> head = find_bucket(table, hash);
> hlist_for_each_entry_rcu(flow, head, hash_node[table->node_ver]) {
> @@ -1071,11 +1070,8 @@ struct sw_flow *ovs_flow_lookup(struct flow_table
> *tbl,
> }
>
>
> -void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow,
> - const struct sw_flow_key *key, int key_len)
> +void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow)
> {
> - flow->unmasked_key = *key;
> - flow_key_mask(&flow->key, &flow->unmasked_key,
> ovsl_dereference(flow->mask));
> flow->hash = ovs_flow_hash(&flow->key,
> ovsl_dereference(flow->mask)->range.start,
> ovsl_dereference(flow->mask)->range.end);
> diff --git a/datapath/flow.h b/datapath/flow.h
> index a31dab0..1a3764e 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -216,9 +216,8 @@ void ovs_flow_tbl_destroy(struct flow_table *table,
> bool deferred);
> struct flow_table *ovs_flow_tbl_alloc(int new_size);
> struct flow_table *ovs_flow_tbl_expand(struct flow_table *table);
> struct flow_table *ovs_flow_tbl_rehash(struct flow_table *table);
> -void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow,
> - const struct sw_flow_key *key, int key_len);
>
> +void ovs_flow_insert(struct flow_table *table, struct sw_flow *flow);
> void ovs_flow_remove(struct flow_table *table, struct sw_flow *flow);
>
> struct sw_flow *ovs_flow_dump_next(struct flow_table *table, u32 *bucket,
> u32 *idx);
> @@ -258,4 +257,6 @@ void ovs_sw_flow_mask_del_ref(struct sw_flow_mask *,
> bool deferred);
> void ovs_sw_flow_mask_insert(struct flow_table *, struct sw_flow_mask *);
> struct sw_flow_mask *ovs_sw_flow_mask_find(const struct flow_table *,
> const struct sw_flow_mask *);
> +void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key
> *src,
> + const struct sw_flow_mask *mask);
> #endif /* flow.h */
> --
> 1.8.1.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
