netdev - Re: [PATCH net-next] team: prevent adding a device which is already a team device lower

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGWr4cQNhd2UQn33F_JJUE5tFrQgRHe0BZs-kGO4cno4uZ6HnA@mail.gmail.com>
Date: Thu, 2 Jan 2025 11:50:25 -0800
From: Octavian Purdila <tavip@...gle.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: jiri@...nulli.us, andrew+netdev@...n.ch, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, 
	syzbot+3c47b5843403a45aef57@...kaller.appspotmail.com
Subject: Re: [PATCH net-next] team: prevent adding a device which is already a
 team device lower

On Thu, Jan 2, 2025 at 12:53 AM Hangbin Liu <liuhangbin@...il.com> wrote:
>
> On Thu, Jan 02, 2025 at 08:50:42AM +0000, Hangbin Liu wrote:
> > On Mon, Dec 30, 2024 at 12:56:47PM -0800, Octavian Purdila wrote:
> > > Prevent adding a device which is already a team device lower,
> > > e.g. adding veth0 if vlan1 was already added and veth0 is a lower of
> > > vlan1.
> > >
> > > This is not useful in practice and can lead to recursive locking:
> > >
> > > $ ip link add veth0 type veth peer name veth1
> > > $ ip link set veth0 up
> > > $ ip link set veth1 up
> > > $ ip link add link veth0 name veth0.1 type vlan protocol 802.1Q id 1
> > > $ ip link add team0 type team
> > > $ ip link set veth0.1 down
> > > $ ip link set veth0.1 master team0
> > > team0: Port device veth0.1 added
> > > $ ip link set veth0 down
> > > $ ip link set veth0 master team0
> > >
>
> I didn't test, what if enslave veth0 first and then add enslave veth0.1 later.
>

Hi Hangbin,

Thanks for the review!

I was not able to reproduce the crash with this scenario. I think this
is because adding veth0.1 does not affect the link state for veth0,
while in the original scenario bringing up veth0 would also bring up
veth0.1.

Regardless, allowing this setup seems risky and syzkaller may find
other ways to trigger it, so maybe a more generic check like below
would be better?

        list_for_each_entry(tmp, &team->port_list, list) {
                if (netdev_has_upper_dev(tmp->dev, port_dev) ||
                        netdev_has_upper_dev(port_dev, tmp->dev)) {
                        NL_SET_ERR_MSG(extack, "Device is a lower or
upper of an enslaved device");
                        netdev_err(dev, "Device %s is a lower or upper
device of enslaved device %s\n",
                                   portname, tmp->dev->name);
                        return -EBUSY;
                }
        }

Although I am not sure if there are legitimate use-cases that this may restrict?