[ovs-dev] [PATCH] datapath: Use masked flow when validating actions.
Jesse Gross
jesse at nicira.com
Tue Jul 16 20:54:40 UTC 2013
I think it actually is safe even in the presence of partial masks
because of the way that we are enforcing the checks. For example, if
we have a set TCP action then it isn't safe to just look at the IP
protocol because that could correspond to a packet that is truncated
after the IP header. Instead, we need to verify that the TCP header
actually exists, which we do by looking at some fields in the
appropriate header (in the case of TCP one of the two port numbers
must be non-zero). Given this, I think our existing prerequisite
checks in the flow setup can handle the rest.
On Tue, Jul 16, 2013 at 12:11 PM, Andy Zhou <azhou at nicira.com> wrote:
> 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 <jesse at 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 <jesse at 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 at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>
X-CudaMail-Whitelist-To: dev at openvswitch.org
More information about the dev
mailing list