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] [day] [month] [year] [list]

Message-ID: <20250901113244.58c19ca1@kernel.org>
Date: Mon, 1 Sep 2025 11:32:44 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: Alok Tiwari <alok.a.tiwari@...cle.com>, jiri@...dia.com,
 stanislaw.gruszka@...ux.intel.com, andrew+netdev@...n.ch,
 davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 horms@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net] genetlink: fix genl_bind() invoking bind() after
 -EPERM

On Mon, 1 Sep 2025 03:23:36 +0200 Andrew Lunn wrote:
> > @@ -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?

The loop has a break at the end, there can be only one family that owns
a given mcast group ID. So the structure of the code is fine as is
(with the exception of the bug discussed).

As for the fix, to avoid future problems, I'd add a separate:

		if (ret)
			break;

rather than inserting a check into the bind condition.

> 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?

Good question, I think core checks if the group exists within
genetlink, here we should be more or less guaranteed to find
the family. Would be good to test that tho.

> 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.