netdev - Re: [PATCH net] openvswitch: Fix mask generation for nested attributes.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 11 Sep 2015 18:37:40 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Pravin Shelar <pshelar@...ira.com>
Cc:	David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] openvswitch: Fix mask generation for nested attributes.

On Fri, Sep 11, 2015 at 9:04 AM, Pravin Shelar <pshelar@...ira.com> wrote:
> On Thu, Sep 10, 2015 at 6:36 PM, Jesse Gross <jesse@...ira.com> wrote:
>> Masks were added to OVS flows in a way that was backwards compatible
>> with userspace programs that did not generate masks. As a result, it is
>> possible that we may receive flows that do not have a mask and we need
>> to synthesize one.
>>
>> Generating a mask requires iterating over attributes and descending into
>> nested attributes. For each level we need to know the size to generate the
>> correct mask. We do this with a linked table of attribute types.
>>
>> Although the logic to handle these nested attributes was there in concept,
>> there are a number of bugs in practice. Examples include incomplete links
>> between tables, variable length attributes being treated as nested and
>> missing sanity checks.
>>
> Missing Fixes tag.

This isn't really fixing one commit, it's more something that has
degraded over time. I'm also not looking for this to go to stable
since I haven't actually heard reports of this causing a problem, it's
just something I saw by looking at the code.

> This patch adds white space space errors.
>
> ./scripts/checkpatch.pl
> 0001-openvswitch-Fix-mask-generation-for-nested-attribute.patch
>
> total: 15 errors, 19 warnings, 0 checks, 130 lines checked

Fixed, thanks.

>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index c92d6a2..83a6847 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -57,6 +57,7 @@ struct ovs_len_tbl {
>>  };
>>
>
> ...
>>         unsigned long opt_key_offset;
>>         struct vxlan_metadata opts;
>> -       int err;
>>
>>         BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts));
>>
>> -       err = nla_parse_nested(tb, OVS_VXLAN_EXT_MAX, a, vxlan_opt_policy);
>> -       if (err < 0)
>> -               return err;
>> -
>>         memset(&opts, 0, sizeof(opts));
>> +       nla_for_each_nested(a, attr, rem) {
>> +               int type = nla_type(a);
>>
>> -       if (tb[OVS_VXLAN_EXT_GBP])
>> -               opts.gbp = nla_get_u32(tb[OVS_VXLAN_EXT_GBP]);
>> +               if (type > OVS_VXLAN_EXT_MAX) {
>> +                        OVS_NLERR(log, "VXLAN extension %d out of range max %d",
>> +                                  type, OVS_VXLAN_EXT_MAX);
>> +                        return -EINVAL;
>> +                }
>> +
>> +                if (ovs_vxlan_ext_key_lens[type].len != nla_len(a) &&
>> +                    !(ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_NESTED ||
>> +                      ovs_vxlan_ext_key_lens[type].len == OVS_ATTR_VARIABLE)) {
>
> ovs_vxlan_ext_key_lens types is never set to nested or variable len.
> so this would not true.

At the moment, it's true that it can't happen. However, I'd like to
avoid making assumptions like that in the code since it could make it
more likely that this type of problem is reintroduced in the future.
It's somewhat moot anyways after the introduction of a common helper
function.

>> +                        OVS_NLERR(log, "VXLAN extension %d has unexpected len %d expected %d",
>> +                                  type, nla_len(a), ovs_vxlan_ext_key_lens[type].len);
>> +                        return -EINVAL;
>> +                }
>> +
>> +                switch (type) {
>> +               case OVS_VXLAN_EXT_GBP:
>> +                       opts.gbp = nla_get_u32(a);
>> +                       break;
>> +               default:
>> +                        OVS_NLERR(log, "Unknown VXLAN extension attribute %d",
>> +                                  type);
>> +                        return -EINVAL;
>> +               }
>> +       }
>
> Need to check for remaining bytes in this attribute.

Added.

>> @@ -1923,7 +1951,8 @@ static int validate_set(const struct nlattr *a,
>>
>>         if (key_type > OVS_KEY_ATTR_MAX ||
>>             (ovs_key_lens[key_type].len != key_len &&
>> -            ovs_key_lens[key_type].len != OVS_ATTR_NESTED))
>> +            !(ovs_key_lens[key_type].len == OVS_ATTR_NESTED ||
>> +              ovs_key_lens[key_type].len == OVS_ATTR_VARIABLE)))
>
> Same check are done multiple times, It would be nice if we add helper
> function for this.

I agree it's better. I factored these out.

I'll send out a v2 shortly.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html