netdev - Re: [PATCH net] bridge: check iface upper dev when setting master via ioctl

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]

Date:   Fri, 27 Apr 2018 11:33:06 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, Dmitry Vyukov <dvyukov@...gle.com>,
        syzbot <syzbot+de73361ee4971b6e6f75@...kaller.appspotmail.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net] bridge: check iface upper dev when setting master via
 ioctl

On 27/04/18 04:31, Hangbin Liu wrote:
> Hi Nikolay,
> 
> Thanks for the comments.
> On Thu, Apr 26, 2018 at 05:22:46PM +0300, Nikolay Aleksandrov wrote:
>>> Not all upper devs are masters. This can break some setups.
> 
> Ah, like vlan device.. So how about
> 
> +	if (netdev_master_upper_dev_get(dev))
>   		return -EBUSY;

That should be fine, yes.

> 
>>>
>>>
>>
>> Also it's not really a bug, the device begins to get initialized but it
>> will get removed at netdev_master_upper_dev_link() anyway if there's
>> already a master. Why would it be better ?
> 
>> It's clearly wrong to try and enslave a device that already has a master
>> via ioctl, rtnetlink already deals with that and the old ioctl interface
>> will get an error, yes it will initialize some structs but they'll get
>> freed later. This is common practice, check the bonding for example.
> 
> Bonding use netdev_is_rx_handler_busy(slave_dev) to check if the slave
> already has a master, which is another solution.

Some masters don't use rx_handlers and the bonding fails at linking them
as a master which is still fine, it cleans up after the error like the bridge.

>>
>> If anything do the check in the ioctl interface (add_del_if) only and
>> maybe target net-next, there's really no bug fix here. IMO it's not
> 
> What if someone do like
> 
> while true; do brctl addif br0 bond_slave &; done
> 
> I know this is stupid and almost no one will do that in real world.
> But syzbot run some similar test and get warn from kobject_add_internal()
> with -ENOMEM. That's why I think we should fix it before allocate any
> resource.
> 
> What do you think?

The bridge code is only a symptom of what happened, that warn was placed to
warn people against doing stupid things - it was literally in the commit message
of some kobject patch. As long as the resources involved are cleaned up and it's
returned to the bridge to cleanup after itself, it should be fine.
  
You can add the check if you feel like it, I don't have an
objection against failing earlier. My main concern was the netdev_has_any_upper
usage which can break some setups.

Cheers,
  Nik

> 
> [1] https://syzkaller.appspot.com/bug?id=3e0339080acd6a2a350a900bc6533b03f5498490
> 
> Thanks
> Hangbin
>