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: <c8108c6b-5276-4a49-01ae-a99795f04359@cumulusnetworks.com>
Date:   Thu, 26 Apr 2018 17:22:46 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org
Cc:     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 26/04/18 17:00, Nikolay Aleksandrov wrote:
> On 26/04/18 16:56, Hangbin Liu wrote:
>> When we set a bond slave's master to bridge via ioctl, we only check
>> the IFF_BRIDGE_PORT flag. Although we will find the slave's real master
>> at netdev_master_upper_dev_link() later, it already does some settings
>> and allocates some resources. So it would be better to return as early
>> as possible.
>>
>> Reported-by: syzbot+de73361ee4971b6e6f75@...kaller.appspotmail.com
>> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
>> ---
>>  net/bridge/br_if.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 82c1a6f..176de8a9 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -518,8 +518,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,
>>  		return -ELOOP;
>>  	}
>>  
>> -	/* Device is already being bridged */
>> -	if (br_port_exists(dev))
>> +	/* Device has master upper dev */
>> +	if (netdev_has_any_upper_dev(dev))
>>  		return -EBUSY;
>>  
>>  	/* No bridging devices that dislike that (e.g. wireless) */
>>
> 
> Not all upper devs are masters. This can break some setups.
> 
> 

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.

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
needed even there, but it doesn't hurt either so up to you.

Thanks,
 Nik