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

[Jiri Pirko <jiri@resnulli.us>]

Fri, Feb 14, 2020 at 08:11:26AM CET, eric.dumazet@gmail.com wrote:
>
>
>On 2/12/20 10:58 PM, Eric Dumazet 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.
>> 
>> 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.
>
>Also, related to this new fancy code ;)
>
>Is there anything preventing netdev_name_node_alt_destroy() from destroying the primary
>ifname ?
>
>netdev_name_node_lookup() should be able to find dev->name_node itself ?
>
>Then we would leave a dangling pointer in dev->name_node, and crash later.
>
>I am thinking we need this fix :

That is correct. We need that. Thanks!


>
>diff --git a/net/core/dev.c b/net/core/dev.c
>index a6316b336128cdb31eea6e80f1a47620abbd0d31..3fa2bc2c30ee1350b5b4b400f0552b9bf2a62697 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -331,6 +331,11 @@ int netdev_name_node_alt_destroy(struct net_device *dev, const char *name)
>        name_node = netdev_name_node_lookup(net, name);
>        if (!name_node)
>                return -ENOENT;
>+
>+       /* Do not trust users, ever ! */
>+       if (name_node == dev->name_node || name_node->dev != dev)
>+               return -EINVAL;
>+
>        __netdev_name_node_alt_destroy(name_node);
> 
>        return 0;