netdev - Re: [Patch net] rtnetlink: catch error pointer for rtnl_link_get

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: <Z0n4IGetFk+Y5wMz@pop-os.localdomain>
Date: Fri, 29 Nov 2024 09:21:36 -0800
From: Cong Wang <xiyou.wangcong@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: cong.wang@...edance.com, netdev@...r.kernel.org,
	syzbot+21ba4d5adff0b6a7cfc6@...kaller.appspotmail.com
Subject: Re: [Patch net] rtnetlink: catch error pointer for
 rtnl_link_get_net()

On Fri, Nov 29, 2024 at 04:36:09PM +0900, Kuniyuki Iwashima wrote:
> From: Cong Wang <xiyou.wangcong@...il.com>
> Date: Thu, 28 Nov 2024 22:31:12 -0800
> > From: Cong Wang <cong.wang@...edance.com>
> > 
> > Currently all callers of rtnl_link_get_net() assume that it always
> > returns a valid netns pointer,
> 
> because I assume it's always tested in rtnl_add_peer_net()...

Why is this assumption?

I seriouly doubt you can assume that, because for example in veth_newlink()
'peer_tb' is parsed within the same function and it is right before
rtnl_link_get_net().

> 
> 
> > when rtnl_link_get_net_ifla() fails,
> > it uses 'src_net' as a fallback.
> > 
> > This is not true,
> 
> because rtnl_link_get_net_ifla() isn't called if (!data ||
> !data[ops->peer_type]),
> 
> so the correct fix is:

It is not.

> 
> ---8<---
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index dd142f444659..c1f4aaa40823 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3815,6 +3815,10 @@ static int rtnl_add_peer_net(struct rtnl_nets *rtnl_nets,
>  	struct net *net;
>  	int err;
>  
> +	net = rtnl_link_get_net_ifla(tb);
> +	if (IS_ERR(net))
> +		return PTR_ERR(net);
> +
>  	if (!data || !data[ops->peer_type])

'tb' is not yet parsed at this point and you still want to pass it to
rtnl_link_get_net_ifla()? In fact, it is even uninitialized.

Thanks.