netdev - Re: [PATCH net-next v7 00/11] net: Improve netns handling in rtnetlink

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: <CABAhCOQAqspiaFO-486UtZpEWsua51f+1f6-LocNhHVfAqW=NQ@mail.gmail.com>
Date: Tue, 7 Jan 2025 18:46:38 +0800
From: Xiao Liang <shaw.leon@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: andrew+netdev@...n.ch, b.a.t.m.a.n@...ts.open-mesh.org, 
	bpf@...r.kernel.org, bridge@...ts.linux.dev, davem@...emloft.net, 
	donald.hunter@...il.com, dsahern@...nel.org, edumazet@...gle.com, 
	horms@...nel.org, idosch@...dia.com, jiri@...nulli.us, kuba@...nel.org, 
	linux-can@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-ppp@...r.kernel.org, 
	linux-rdma@...r.kernel.org, linux-wireless@...r.kernel.org, 
	linux-wpan@...r.kernel.org, liuhangbin@...il.com, netdev@...r.kernel.org, 
	osmocom-net-gprs@...ts.osmocom.org, pabeni@...hat.com, shuah@...nel.org, 
	wireguard@...ts.zx2c4.com
Subject: Re: [PATCH net-next v7 00/11] net: Improve netns handling in rtnetlink

On Tue, Jan 7, 2025 at 4:57 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Xiao Liang <shaw.leon@...il.com>
> Date: Sat,  4 Jan 2025 20:57:21 +0800
[...]
> > - In amt_newlink() drivers/net/amt.c:
> >
> >     amt->net = net;
> >     ...
> >     amt->stream_dev = dev_get_by_index(net, ...
> >
> >   Uses net, but amt_lookup_upper_dev() only searches in dev_net.
> >   So the AMT device may not be properly deleted if it's in a different
> >   netns from lower dev.
>
> I think you are right, and the upper device will be leaked
> and UAF will happen.
>
> amt must manage a list linked to a lower dev.
>
> Given no one has reported the issue, another option would be
> drop cross netns support in a short period.

Yes. I also noticed AMT sets dev->netns_local to prevent netns
change. Probably it also assumes the same netns during creation.

[...]
> >
> > - In gtp_newlink() in drivers/net/gtp.c:
> >
> >     gtp->net = src_net;
> >     ...
> >     gn = net_generic(dev_net(dev), gtp_net_id);
> >     list_add_rcu(&gtp->list, &gn->gtp_dev_list);
> >
> >   Uses src_net, but priv is linked to list in dev_net. So it may not be
> >   properly deleted on removal of link netns.
>
> The device is linked to a list in the same netns, so the
> device will not be leaked.  See gtp_net_exit_batch_rtnl().
>
> Rather, the problem is the udp tunnel socket netns could be
> freed earlier than the dev netns.

Yes, you're right. Actually I mean the netns of the socket by "link netns"
(there's some clarification about this in patch 02).

[...]
> >
> > - In pfcp_newlink() in drivers/net/pfcp.c:
> >
> >     pfcp->net = net;
> >     ...
> >     pn = net_generic(dev_net(dev), pfcp_net_id);
> >     list_add_rcu(&pfcp->list, &pn->pfcp_dev_list);
> >
> >   Same as above.
>
> I haven't tested pfcp but it seems to have the same problem.
>
> I'll post patches for gtp and pfcp.
>

It would be nice.

>
> >
> > - In lowpan_newlink() in net/ieee802154/6lowpan/core.c:
> >
> >     wdev = dev_get_by_index(dev_net(ldev), nla_get_u32(tb[IFLA_LINK]));
> >
> >   Looks for IFLA_LINK in dev_net, but in theory the ifindex is defined
> >   in link netns.
>
> I guess you mean the ifindex is defined in src_net instead.
> Not sure if it's too late to change the behaviour.

Yes, it's source net for lowpan. I think it depends on whether
the interpretation of IFLA_LINK should be considered as part API
provided by rtnetlink core, or something customizable by driver.
In the former case, this can be considered as a bug.

Thanks.