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
