Never mind. It is also covered.
On Tue, Jul 16, 2013 at 3:07 PM, Andy Zhou <az...@nicira.com> wrote:
> What if the IP protocol field is partially masked?
>
>
> On Tue, Jul 16, 2013 at 1:54 PM, Jesse Gross <je...@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 <az...@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 <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
