linux-kernel - Re: [PATCH v4] net: cgroup: fix access the unallocated memory in netprio cgroup

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] [day] [month] [year] [list]

Date:	Thu, 19 Jul 2012 06:28:39 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Li Zefan <lizefan@...wei.com>
Cc:	John Fastabend <john.r.fastabend@...el.com>,
	Gao feng <gaofeng@...fujitsu.com>, eric.dumazet@...il.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	davem@...emloft.net, Eric Dumazet <edumazet@...gle.com>,
	"Rustad, Mark D" <mark.d.rustad@...el.com>
Subject: Re: [PATCH v4] net: cgroup: fix access the unallocated memory in
 netprio cgroup

On Thu, Jul 19, 2012 at 09:00:51AM +0800, Li Zefan wrote:
> >>>>>  static struct cgroup_subsys_state *cgrp_create(struct cgroup *cgrp)
> 
> >>>>>  {
> >>>>>  	struct cgroup_netprio_state *cs;
> >>>>> -	int ret;
> >>>>> +	int ret = -EINVAL;
> >>>>>
> >>>>>  	cs = kzalloc(sizeof(*cs), GFP_KERNEL);
> >>>>>  	if (!cs)
> >>>>>  		return ERR_PTR(-ENOMEM);
> >>>>>
> >>>>> -	if (cgrp->parent && cgrp_netprio_state(cgrp->parent)->prioidx) {
> >>>>> -		kfree(cs);
> >>>>> -		return ERR_PTR(-EINVAL);
> >>>>> -	}
> >>>>> +	if (cgrp->parent && cgrp_netprio_state(cgrp->parent)->prioidx)
> >>>>> +		goto out;
> >>>>>
> >>>>>  	ret = get_prioidx(&cs->prioidx);
> >>>>> -	if (ret != 0) {
> >>>>> +	if (ret < 0) {
> >>>>>  		pr_warn("No space in priority index array\n");
> >>>>> -		kfree(cs);
> >>>>> -		return ERR_PTR(ret);
> >>>>> +		goto out;
> >>>>> +	}
> >>>>> +
> >>>>> +	ret = update_netdev_tables();
> >>>>> +	if (ret < 0) {
> >>>>> +		put_prioidx(cs->prioidx);
> >>>>> +		goto out;
> >>>>>  	}
> >>>>
> >>>> Gao,
> >>>>
> >>>> This introduces a null ptr dereference when netprio_cgroup is built
> >>>> into the kernel because update_netdev_tables() depends on init_net.
> >>>> However cgrp_create is being called by cgroup_init before
> >>>> do_initcalls() is called and before net_dev_init().
> >>>>
> >>>> .John
> >>>>
> >>> Not sure I follow here John.  Shouldn't init_net be initialized prior to any
> >>> network devices getting registered?  In other words, shouldn't for_each_netdev
> >>> just result in zero iterations through the loop?
> >>> Neil
> >>>
> >>
> >> init_net _is_ initialized prior to any network devices getting
> >> registered but not before cgrp_create called via cgroup_init.
> >>
> >> #define for_each_netdev(net, d)         \
> >>                 list_for_each_entry(d, &(net)->dev_base_head, dev_list)
> >>
> >> but dev_base_head is zeroed at this time. In netdev_init we have,
> >>
> >>         INIT_LIST_HEAD(&net->dev_base_head);
> >>
> >> but we haven't got that far yet because cgroup_init is called
> >> before do_initcalls().
> >>
> > ok, I see that, and it makes sense, but at this point I'm more concerned with
> > cgroups getting initalized twice.  The early_init flag is clear in the
> > cgroup_subsystem for netprio, so we really shouldn't be getting initalized from
> > cgroup_init. We should be getting initalized from the module_init() call that
> 
> > we register
> 
> If the early_init flag is set, a cgroup subsys will be initialized from
> cgroup_early_init(), otherwise cgroup_init().
> 
> If netprio is built as a module, the subsys will be initailized from module_init(),
> otherwise cgroup_init() (in this case cgroup_load_subsys() called in module_init()
> is a no-op).
> 
> So it won't get initialized twice.
> 
> 
Yeah, we already figured that out :).

Still its not a sane interface.  If you create a module_init function for a bit
of code, you expect that function to be called before the rest of your code ever
gets executed.  The way cgroup_init works, ss->cgroup_create gets called before
the module_init routine does when the module is built monolithically.  So no, no
double initalization, but definately some behavior that is going to be prone to
mistakes.
Neil

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/