[ovs-dev] [PATCH] datapath: Use masked flow when validating actions.
Jesse Gross
jesse at nicira.com
Tue Jul 16 22:40:34 UTC 2013
Right, I think it should work because in order to have a non-zero TCP
port the flow setup code will require that we have an exact match on
the IP protocol.
I added a log message and applied this patch.
On Tue, Jul 16, 2013 at 3:09 PM, Andy Zhou <azhou at nicira.com> wrote:
> Never mind. It is also covered.
>
>
> On Tue, Jul 16, 2013 at 3:07 PM, Andy Zhou <azhou at nicira.com> wrote:
>>
>> What if the IP protocol field is partially masked?
>>
>>
>> On Tue, Jul 16, 2013 at 1:54 PM, Jesse Gross <jesse at nicira.com> wrote:
>>>
>>> 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