Re: [PATCH] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs <rgb@redhat.com>]

On 14/03/24, Richard Guy Briggs wrote:
> On 14/03/23, David Miller wrote:
> > From: Richard Guy Briggs <rgb@redhat.com>
> > Date: Fri, 21 Mar 2014 12:39:11 -0400
> > 
> > > @@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> > >  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> > >  		return 0;
> > >  
> > > +	if (nlk->netlink_bind && nladdr->nl_groups) {
> > > +		int i;
> > > +
> > > +		for (i = 0; i < nlk->ngroups; i++)
> > > +			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
> > > +				err = nlk->netlink_bind(i);
> > > +				if (err)
> > > +					return err;
> > > +			}
> > > +	}
> > > +
> > 
> > You can't just leave a partially set of completed bindings in place.
> 
> In the general case, I agree.
> 
> > It's not valid to leave half-baked state like this.
> 
> In the one existing case (netfilter), it adds a module that is never
> unloaded.  (refcounts are bumped up and down, but I don't see an
> auto-reap based on cleared multicast group subscriptions.)  For that
> matter, netlink_realloc_groups() isn't reversed on error either.

Ok, in netlink_bind(), netlink_insert()/netlink_autobind() also need
to be undone with netlink_remove() if nlk->portid was not set.

> In the proposed case (audit) it is only a permissions check, so there is
> nothing to undo.
> 
> So, I was being lazy looking at the existing situation.
> 
> > If you return an error, all of the binding state changes must be
> > completely undone.
> 
> Is it time to add a ".unbind = netlink_unbind" to struct proto_ops
> netlink_ops?  (I am only half serious here...)

At this stage, that function would be a no-op for netfilter and audit.
Are there any out-of-tree users of this per-protocol bind function?

> > If you can't find a way to do this cleanly, you'll need to find
> > a way for the audit code to not return an error.
> 
> Fair enough.  I'll go back and look at updating subscriptions and
> listeners first and undoing those actions if the bind fails.  In the
> case of netlink_setsockopt() it is just one to undo, which is easy.
> netlink_bind() is a bit more complex, but doable.
> 
> The whole purpose here was to add a way for each protocol to be able to
> add its own permissions check and signal a way for netlink to refuse the
> subscription if the userspace process doesn't have the required
> permissions, so not returning an error defeats that whole purpose.
> 
> - RGB

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545