netdev - Re: [PATCH net] net/sched: Fix use of wild pointer in mq_destroy() when qdisc

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]

Message-ID: <20141024214511.GA10290@acer.localdomain>
Date:	Fri, 24 Oct 2014 22:45:12 +0100
From:	Patrick McHardy <kaber@...sh.net>
To:	Cong Wang <cwang@...pensource.com>
Cc:	John Fastabend <john.fastabend@...il.com>, wang.bo116@....com.cn,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>, cui.yunfeng@....com.cn
Subject: Re: [PATCH net] net/sched: Fix use of wild pointer in mq_destroy()
 when qdisc_alloc fail

On Fri, Oct 24, 2014 at 01:52:00PM -0700, Cong Wang wrote:
> On Fri, Oct 24, 2014 at 12:14 PM, Patrick McHardy <kaber@...sh.net> wrote:
> > On Fri, Oct 24, 2014 at 11:13:56AM -0700, Cong Wang wrote:
> >> On Fri, Oct 24, 2014 at 10:49 AM, John Fastabend
> >> <john.fastabend@...il.com> wrote:
> >> >
> >> > Patch looks fine, another way to fix this would be drop the
> >> > mq_destroy() call in the error path. I'm not convinced one
> >> > is any better than the other but maybe some other folks have
> >> > opinions, it seems a bit wrong to call mq_destroy twice so in
> >> > that sense it may be a bit nicer to drop the mq_destroy().
> >>
> >> Dropping mq_destroy() in error path is indeed better,
> >> because upper layer does cleanup intentionally.
> >> Look at what other qdisc's do. :)
> >
> > I would argue that the qdisc_destroy() call in qdisc_create_dflt()
> > is wrong, it should instead free the qdisc and release the module
> > reference manually as done in qdisc_create().
> >
> > qdisc_destroy() should only be called for fully initialized qdiscs.
> 
> Probably, but at least ->destroy() should be called, looking at
> those calling qdisc_watchdog_init(), they are supposed to call
> qdisc_watchdog_cancel() when >init() fails after that.

In which cases does it actually fail after that? Usually this is
called once initialization is complete.

> ->destroy() is supposed to be able to clean up even partially
> initialized qdisc's. So, for qdisc_create_dflt() we should probably
> just call ->destroy().

No, why do you think that? ->init() is supposed to clean up after
itself if it fails, both qdisc_destroy() and ->destroy are only
supposed to be called after ->init() succeeded.

Its simply symetrical, as everywhere else in the kernel. If a sub-init
funtion fails, it should clean up and return an error. We don't
destroy things we've never successfully initialized, they're supposed
to clean up after themselves.

> Reading the code again, seems it is inconsistent with qdisc_create(),
> where ->destroy() is skipped when ->init() fails. Hmm, we have
> a bigger problem here.
> 
> I am working on a patch now. Thanks for pointing this out.

No, that is not inconsistent, it is consistent with what we're doing
everywhere else in the kernel, what the qdisc layer has always done
and how qdiscs expect it.

Where are you seeing a problem now? Its a simple fix:
qdisc_create_dflt() should behave similar to qdisc_create().

If any ->init() functions than don't properly clean up on error, that's
an additional bug.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html