On 14/03/24, Richard Guy Briggs wrote:
> On 14/03/23, David Miller wrote:
> > From: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
