netdev - Re: [PATCH net] genetlink: fix genl_bind() invoking bind() after -EPERM

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: <7bb4e094-fa20-42d6-89d5-c25cc0584309@lunn.ch>
Date: Mon, 1 Sep 2025 03:23:36 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Alok Tiwari <alok.a.tiwari@...cle.com>
Cc: jiri@...dia.com, stanislaw.gruszka@...ux.intel.com,
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH net] genetlink: fix genl_bind() invoking bind() after
 -EPERM

On Sun, Aug 31, 2025 at 12:03:13PM -0700, Alok Tiwari wrote:
> Per family bind/unbind callbacks were introduced to allow families
> to track multicast group consumer presence, e.g. to start or stop
> producing events depending on listeners.
> 
> However, in genl_bind() the bind() callback was invoked even if
> capability checks failed and ret was set to -EPERM. This means that
> callbacks could run on behalf of unauthorized callers while the
> syscall still returned failure to user space.
> 
> Fix this by only invoking bind() if (!ret && family->bind)
> i.e. after permission checks have succeeded.

Firstly, i don't know this code at all. I've no idea what it should
do....

> 
> Fixes: 3de21a8990d3 ("genetlink: Add per family bind/unbind callbacks")
> Signed-off-by: Alok Tiwari <alok.a.tiwari@...cle.com>
> ---
>  net/netlink/genetlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 104732d34543..3b51fbd068ac 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1836,7 +1836,7 @@ static int genl_bind(struct net *net, int group)
>  		    !ns_capable(net->user_ns, CAP_SYS_ADMIN))
>  			ret = -EPERM;
>  
> -		if (family->bind)
> +		if (!ret && family->bind)
>  			family->bind(i);

I agree, this fixes the issue you point out. But i think it would be
more robust if after each EPERM there was a continue.

Also, i don't understand how this ret value is used. It looks like the
bind() op could be called a number of times, and yet genl_bind()
returns -EPERM?

Also, struct genl_family defines bind() as returning an int. It does
not say so, but i assume the return value is 0 on success, negative
error code on failure. Should we be throwing this return value away?
Should genl_bind() return an error code if the bind failed?

And if genl_bind() does return an error, should it first cleanup and
unbind any which were successful bound?

As i said, i don't know this code, so all i can do is ask questions in
the hope somebody does know what is supposed to happen here.

       Andrew