Re: [PATCH net] net: rtnetlink: fix bugs in rtnl_alt_ifname()

[Jiri Pirko <jiri@resnulli.us>]

Thu, Feb 13, 2020 at 07:58:01AM CET, eric.dumazet@gmail.com wrote:
>
>
>On 2/12/20 10:45 PM, Jiri Pirko wrote:
>> Thu, Feb 13, 2020 at 05:58:26AM CET, edumazet@google.com wrote:
>>> Since IFLA_ALT_IFNAME is an NLA_STRING, we have no
>>> guarantee it is nul terminated.
>>>
>>> We should use nla_strdup() instead of kstrdup(), since this
>>> helper will make sure not accessing out-of-bounds data.
>>>
>>> 
>>> Fixes: 36fbf1e52bd3 ("net: rtnetlink: add linkprop commands to add and delete alternative ifnames")
>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>> Cc: Jiri Pirko <jiri@mellanox.com>
>>> Reported-by: syzbot <syzkaller@googlegroups.com>
>>> ---
>>> net/core/rtnetlink.c | 26 ++++++++++++--------------
>>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>>> index 09c44bf2e1d28842d77b4ed442ef2c051a25ad21..e1152f4ffe33efb0a69f17a1f5940baa04942e5b 100644
>>> --- a/net/core/rtnetlink.c
>>> +++ b/net/core/rtnetlink.c
>>> @@ -3504,27 +3504,25 @@ static int rtnl_alt_ifname(int cmd, struct net_device *dev, struct nlattr *attr,
>>> 	if (err)
>>> 		return err;
>>>
>>> -	alt_ifname = nla_data(attr);
>>> +	alt_ifname = nla_strdup(attr, GFP_KERNEL);
>>> +	if (!alt_ifname)
>>> +		return -ENOMEM;
>>> +
>>> 	if (cmd == RTM_NEWLINKPROP) {
>>> -		alt_ifname = kstrdup(alt_ifname, GFP_KERNEL);
>>> -		if (!alt_ifname)
>>> -			return -ENOMEM;
>>> 		err = netdev_name_node_alt_create(dev, alt_ifname);
>>> -		if (err) {
>>> -			kfree(alt_ifname);
>>> -			return err;
>>> -		}
>>> +		if (!err)
>>> +			alt_ifname = NULL;
>>> 	} else if (cmd == RTM_DELLINKPROP) {
>>> 		err = netdev_name_node_alt_destroy(dev, alt_ifname);
>>> -		if (err)
>>> -			return err;
>>> 	} else {
>> 
>> 
>>> -		WARN_ON(1);
>>> -		return 0;
>>> +		WARN_ON_ONCE(1);
>>> +		err = -EINVAL;
>> 
>> These 4 lines do not seem to be related to the rest of the patch. Should
>> it be a separate patch?
>
>Well, we have to kfree(alt_ifname).
>
>Generally speaking I tried to avoid return in the middle of this function.

Yep, but you also change the return value. So that looked to me like
something for another patch. But as you wish :)


>
>The WARN_ON(1) is dead code today, making it a WARN_ON_ONCE(1) is simply
>a matter of avoiding syslog floods if this path is ever triggered in the future.
>
>> 
>> Otherwise, the patch looks fine to me.
>>
>
>Thanks !
>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>