On Sep 5, 2014, at 2:12 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>>>
>>> case OVS_KEY_ATTR_IPV4:
>> [...]
>>> - if (ipv4_key->ipv4_frag != flow_key->ip.frag)
>>> - return -EINVAL;
>>> + /* Non-writeable fields. */
>>> + if (mask->ipv4_proto || mask->ipv4_frag)
>>> + return -EINVAL;
>>> + } else {
>>> + if (!flow_key->ip.proto)
>>> + return -EINVAL;
>>
>> I believe that this check on ip.proto is being used to verify that the
>> IP header is actually present, so this would mean that we can write
>> off the end of the packet in the masked case.
>
> But this is at flow set-up time, and the mask could still wildcard the
> ip.proto field,
I checked this and flow_key is the masked flow key, so it can not be
wildcarded, sorry for the confusion. However, it is conceivable that a
userspace app wants to set a flow to match on all IP packets with
eth_type(0x0800), and then e.g. set(ipv4(tos=0)). So, I’d like to get rid of
the ip.proto check anyway!
Is the make_writeable() check sufficient?
Jarno
> so it could be zero or missing in an actual packet. Doesn’t the
> make_writeable take care of the verification?:
>
> static int set_ipv4(struct sk_buff *skb, const struct ovs_key_ipv4 *key,
> const struct ovs_key_ipv4 *mask)
> {
> struct iphdr *nh;
> __be32 new_addr;
> int err;
>
> err = make_writable(skb, skb_network_offset(skb) +
> sizeof(struct iphdr));
> if (unlikely(err))
> return err;
>
> nh = ip_hdr(skb);
>
>
>>
>>> case OVS_KEY_ATTR_IPV6:
>> [...]
>>> - if (ipv6_key->ipv6_frag != flow_key->ip.frag)
>>> - return -EINVAL;
>>> + /* Non-writeable fields. */
>>> + if (mask->ipv6_proto || mask->ipv6_frag)
>>> + return -EINVAL;
>>> +
>>> + /* Invalid bits in the flow label mask? */
>>> + if (ntohl(mask->ipv6_label) & 0xFFF00000)
>>> + return -EINVAL;
>>> + } else {
>>> + if (!flow_key->ip.proto)
>>> + return -EINVAL;
>>
>> Same issue with header validation here.
>>
>>> @@ -1605,12 +1649,43 @@ static int validate_set(const struct nlattr *a,
>>> + /* Convert non-masked set actions to masked set actions.
>>> + * Tunnel set action returns before getting here. */
>>> + if (!masked) {
>>> + int start, len = key_len * 2;
>>> + struct nlattr *at;
>>> +
>>> + *skip_copy = true;
>>> +
>>> + start = add_nested_action_start(sfa,
>>> + OVS_ACTION_ATTR_SET_MASKED);
>>> + if (start < 0)
>>> + return start;
>>> +
>>> + at = reserve_sfa_size(sfa, nla_attr_size(len));
>>> + if (IS_ERR(at))
>>> + return PTR_ERR(at);
>>> +
>>> + at->nla_type = key_type;
>>> + at->nla_len = nla_attr_size(len);
>>
>> I think this can be simplified by using add_action() or __add_action().
>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
