ip link vf info truncating with many VFs

ip link vf info truncating with many VFs

[Jacob Keller]

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.

Thanks,
Jake

Re: ip link vf info truncating with many VFs

[Jakub Kicinski]

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

Re: ip link vf info truncating with many VFs

[Jacob Keller]

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

Re: ip link vf info truncating with many VFs

[David Ahern]

On 3/2/20 4:21 PM, Jacob Keller wrote:
> 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.
> 

Another pandora's box.

Seems like there are a few other places that set nla_len that should be
checked as well - like __nla_reserve and that bleeds into __nla_put.

Re: ip link vf info truncating with many VFs

[Jacob Keller]

On 3/3/2020 9:16 AM, David Ahern wrote:
> On 3/2/20 4:21 PM, Jacob Keller wrote:
>> 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.
>>
> 
> Another pandora's box.
> 
> Seems like there are a few other places that set nla_len that should be
> checked as well - like __nla_reserve and that bleeds into __nla_put.
> 
Yea, I bet. I'm working on a patch for this nla_nest_end first.

Thanks,
Jake