On Tue, 2007-07-03 at 14:05 +0200, Patrick McHardy wrote:
> > --- wireless-dev.orig/net/netlink/af_netlink.c 2007-07-03 00:10:31.617889695 +0200
> > +++ wireless-dev/net/netlink/af_netlink.c 2007-07-03 00:31:30.267889695 +0200
> > @@ -316,8 +316,11 @@ netlink_update_listeners(struct sock *sk
> >
> > for (i = 0; i < NLGRPSZ(tbl->groups)/sizeof(unsigned long); i++) {
> > mask = 0;
> > - sk_for_each_bound(sk, node, &tbl->mc_list)
> > - mask |= nlk_sk(sk)->groups[i];
> > + sk_for_each_bound(sk, node, &tbl->mc_list) {
> > + if (nlk_sk(sk)->ngroups >=
> > + (i + 1) * sizeof(unsigned long))
>
>
> This condition implies that a socket can bind to a non-existant
> group, which shouldn't be possible.
Actually, it's the other way around, the socket can bind to group 10
only 32 groups are present (one unsigned long) and then some other code
goes to add groups increasing the limit to 64, and then the socket still
only has a bitmap with 32 bits (one unsigned long) and we shouldn't
access beyond that just because the number of groups was increased.
However, you can in fact bind non-existing groups as long as the group
number is lower than the maximum, i.e. if you start out with just one
group as genetlink does, the netlink code increases that to 32 and you
can bind group 25 even if generic netlink doesn't know about it yet. I
plan to fix that when it actually matters, i.e. when I introduce
per-group permission checks.
> > + mask |= nlk_sk(sk)->groups[i];
> > + }
> > tbl->listeners[i] = mask;
> > }
> > /* this function is only called with the netlink table "grabbed", which
> > @@ -555,10 +558,11 @@ netlink_update_subscriptions(struct sock
> > nlk->subscriptions = subscriptions;
> > }
> >
> > -static int netlink_alloc_groups(struct sock *sk)
> > +static int netlink_realloc_groups(struct sock *sk)
> > {
> > struct netlink_sock *nlk = nlk_sk(sk);
> > unsigned int groups;
> > + unsigned long *new_groups;
> > int err = 0;
> >
> > netlink_lock_table();
> > @@ -570,9 +574,15 @@ static int netlink_alloc_groups(struct s
> > if (err)
> > return err;
> >
> > - nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
> > - if (nlk->groups == NULL)
> > + if (nlk->ngroups >= groups)
> > + return 0;
> > +
> > + new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
> > + if (new_groups == NULL)
> > return -ENOMEM;
> > + memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
> > + NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
> > + nlk->groups = new_groups;
>
>
> This should probably happen with the table grabbed to avoid races
> with concurrent broadcasts.
Hmm, possibly, I'll have to look again.
> > +int netlink_change_ngroups(int unit, unsigned int groups)
> > +{
> > + unsigned long *listeners;
> > + int err = 0;
> > +
> > + netlink_table_grab();
>
>
> Unfortunately that doesn't prevent races with netlink_has_listeners
> since its lockless (and should really stay that way).
Uh huh. Good point, I guess I'll have to use RCU or such here when
changing the listeners bitmap size.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
