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

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

[Alok Tiwari]

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.

Fixes: 3de21a8990d3 ("genetlink: Add per family bind/unbind callbacks")
Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.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);
 
 		break;
-- 
2.50.1

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

[Andrew Lunn]

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

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

[ALOK TIWARI]

On 9/1/2025 6:53 AM, Andrew Lunn wrote:
>> Fixes: 3de21a8990d3 ("genetlink: Add per family bind/unbind callbacks")
>> Signed-off-by: Alok Tiwari<alok.a.tiwari@oracle.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


Thanks Andrew.

I am still pretty new to netdev and trying to understand these details.
I also had a similar feeling, returning -EPERM directly looks a bit
odd without a continue and I was also wondering how the ret value is 
actually used.

I am positive other maintainers will offer their valuable
suggestions and help clarify these doubts.


Thanks,
Alok

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

[Jakub Kicinski]

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.