netdev - Re: [PATCH] netlink: allocate group bitmaps dynamically

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 03 Jul 2007 16:09:00 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Patrick McHardy <kaber@...sh.net>
Cc:	hadi@...erus.ca, Zhang Rui <rui.zhang@...el.com>,
	netdev@...r.kernel.org,
	"linux-acpi@...r" <linux-acpi@...r.kernel.org>, lenb@...nel.org,
	Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH] netlink: allocate group bitmaps dynamically

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

Download attachment "signature.asc" of type "application/pgp-signature" (191 bytes)