Johannes Berg wrote:
> 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.
You're right, I misread this code.
> 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.
Yes, I was thinking of groups > 32.
>
>
>>>+ 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.
do_one_broadcast locks the table and checks nlk->groups. The
reallocation races with this without taking the lock or maybe
using rcu.
>
>
>>>+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.
That should work.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
