netdev - Re: ip link vf info truncating with many VFs

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: <20200302151704.56fe3dd4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 2 Mar 2020 15:17:04 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Florian Westphal <fw@...len.de>, Thomas Graf <tgraf@...g.ch>
Subject: Re: ip link vf info truncating with many VFs

On Fri, 28 Feb 2020 16:33:40 -0800 Jacob Keller wrote:
> Hi,
> 
> I recently noticed an issue in the rtnetlink API for obtaining VF
> information.
> 
> If a device creates 222 or more VF devices, the rtnl_fill_vf function
> will incorrectly label the size of the IFLA_VFINFO_LIST attribute. This
> occurs because rtnl_fill_vfinfo will have added more than 65k (maximum
> size of a single attribute since nla_len is a __u16).
> 
> This causes the calculation in nla_nest_end to overflow and report a
> significantly shorter length value. Worse case, with 222 VFs, the "ip
> link show <device>" reports no VF info at all.
> 
> For some reason, the nla_put calls do not trigger an EMSGSIZE error,
> because the skb itself is capable of holding the data.
> 
> I think the right thing is probably to do some sort of
> overflow-protected calculation and print a warning... or find a way to
> fix nla_put to error with -EMSGSIZE if we would exceed the nested
> attribute size limit... I am not sure how to do that at a glance.

Making nla_nest_end() return an error on overflow seems like 
the most reasonable way forward to me, FWIW. Simply compare
the result to U16_MAX, I don't think anything more clever is
needed.

Some of the callers actually already check for errors of
nla_nest_end() (qdiscs' dump methods use the result which 
is later checked for less that zero).

Then rtnetlink code should be made aware that nla_nest_end() 
may fail.

(When you post it's probably a good idea to widen the CC list 
to Johannes Berg, Pablo, DaveA, Jiri..)