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]

Message-ID: <20180427013102.GJ20683@leo.usersys.redhat.com>
Date:   Fri, 27 Apr 2018 09:31:02 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.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

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;

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

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

Thanks
Hangbin