On 14/03/24, Richard Guy Briggs wrote:
> 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
Ok, sorry for all the noise. I had a problem between chair and keyboard
which can explain the number of useless revs of this patch... v6 is
current with potentially v7 with the inline hunk below...
> > > > @@ -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.
This is also true with netlink_setsockopt() in the
NETLINK_{ADD,DROP}_MEMBERSHIP case, where on ADD we should call
netlink_bind() but on DROP we should *not* call it and instead call
netlink_unbind() afterwards, perhaps such as:
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 45dccff..3354d54 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2108,7 +2108,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
return err;
if (!val || val - 1 >= nlk->ngroups)
return -EINVAL;
- if (nlk->netlink_bind) {
+ if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
err = nlk->netlink_bind(val);
if (err)
return err;
@@ -2117,6 +2117,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
netlink_update_socket_mc(nlk, val,
optname == NETLINK_ADD_MEMBERSHIP);
netlink_table_ungrab();
+ if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+ nlk->netlink_unbind(val);
err = 0;
break;
> > 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.
So netlink_setsockopt() was right, but ADD/DROP need slightly different
treatment.
netlink_bind() just needs to undo netlink_bind() on error.
> > 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.
We could do all we wanted in audit, but that would still not signal
netlink to block that subscription.
> > - RGB
>
> - 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
