linux-kernel - Re: [PATCH v1] RDMA/nldev: Add length check for IFLA_BOND_ARP_IP

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7a2e7314.ee8a2.189abf00b34.Coremail.linma@zju.edu.cn>
Date:   Mon, 31 Jul 2023 20:33:02 +0800 (GMT+08:00)
From:   "Lin Ma" <linma@....edu.cn>
To:     "Leon Romanovsky" <leon@...nel.org>
Cc:     "Jakub Kicinski" <kuba@...nel.org>, jgg@...pe.ca,
        markzhang@...dia.com, michaelgur@...dia.com, ohartoov@...dia.com,
        chenzhongjin@...wei.com, yuancan@...wei.com,
        linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] RDMA/nldev: Add length check for
 IFLA_BOND_ARP_IP_TARGET parsing

Hello there,

> > > > Yeah I have seen that. Just as Jakub said, empty netlink attributes are valid 
> > > > (they are viewed as flag). The point is that different attribute has different
> > > > length requirement. For this specific code, the RDMA_NLDEV_ATTR_STAT_HWCOUNTERS
> > > > attribute is a nested one whose inner attributes should be NLA_U32. But as you
> > > > can see in variable nldev_policy, the description does not use nested policy to
> > > > enfore that, which results in the bug discussed in my commit message.
> > > > 
> > > >  [RDMA_NLDEV_ATTR_STAT_HWCOUNTERS]       = { .type = NLA_NESTED },
> > > > 
> > > > The elegant fix could be add the nested policy description to nldev_policy while
> > > > this is toublesome as no existing nla_attr has been given to this nested nlattr.
> > > > Hence, add the length check is the simplest solution and you can see such nla_len
> > > > check code all over the kernel.  
> > > 
> > > Right, and this is what bothers me.
> > > 
> > > I would more than happy to change nla_for_each_nested() to be something
> > > like nla_for_each_nested_type(...., sizeof(u32)), which will skip empty
> > > lines, for code which can't have them.
> > 
> > In general the idea of auto-skipping stuff kernel doesn't recognize
> > is a bit old school. Better direction would be extending the policy
> > validation to cover use cases for such loops.
> 
> I'm all in for any solution which will help for average developer to write
> netlink code without mistakes.
> 
> Thanks

I have just come out a new solution for such length issues. Please see
* https://lore.kernel.org/all/20230731121247.3972783-1-linma@zju.edu.cn/T/#u
* https://lore.kernel.org/all/20230731121324.3973136-1-linma@zju.edu.cn/T/#u

I'm not sure adding additional validation logic in the main nlattr code is
the best solution. Still, after investigating the code, the len field can
be very suitable for handling the NLA_NESTED cases here. And the developer
can do manual parsing with better nla_policy-based checking too.

If this idea is applied, I will also write a script to clean up other
nla_len patches based on the nla_policy check.

Regards
Lin