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 for Android: free password hash cracker in your pocket

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

Message-ID: <ff51329f-9f01-53c7-8214-96542321400f@intel.com>
Date:   Mon, 2 Mar 2020 15:21:06 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
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 3/2/2020 3:17 PM, Jakub Kicinski wrote:
> 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.
> 

Sure, I alto think that's the right approach to fix this.

As long we calculate the value using something larger than a u16 first,
that should work.

> 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).

I'll take a look at the qdisc code.

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

Right.

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

Yep. I wasn't sure who all to add here, so I tried looking at the
MAINTAINERS file.

Thanks,
Jake