netdev - Re: [Patch net v3] cls_flower: call nla_ok() before nla

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20210114145656.65044a4b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 14 Jan 2021 14:56:56 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Cong Wang <cong.wang@...edance.com>,
        syzbot <syzbot+2624e3778b18fc497c92@...kaller.appspotmail.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Xin Long <lucien.xin@...il.com>, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net v3] cls_flower: call nla_ok() before nla_next()

On Thu, 14 Jan 2021 14:50:04 -0800 Cong Wang wrote:
> > static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key,
> >                             int depth, int option_len,
> >                             struct netlink_ext_ack *extack)
> > {
> >         struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_VXLAN_MAX + 1];
> >         struct vxlan_metadata *md;
> >         int err;
> >
> >         md = (struct vxlan_metadata *)&key->enc_opts.data[key->enc_opts.len];
> >         memset(md, 0xff, sizeof(*md));
> >
> >         if (!depth)
> >                 return sizeof(*md);
> >                 ^^^^^^^^^^^^^^^^^^^
> >
> > The mask is filled with all 1s if attribute is not provided.  
> 
> Hmm, then what is the length comparison check for?
> 
> fl_set_vxlan_opt() either turns negative or sizeof(*md), and negitve
> is already checked when it returns, so when we hit the length comparison
> it is always equal. So it must be redundant.
> 
> (Note, I am only talking about the vxlan case you pick here, because the
> geneve case is different, as it returns different sizes.)

Good question 🤷🏻‍♂️🤷🏻‍♂️  I'd guess GENEVE came first and and then
people copy / pasted the code structure other tunnel types?