linux-kernel - Re: ping: drivers/staging/usbip/ abuses task_is_dead/exit

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: <20120313114512.GM21503@distanz.ch>
Date:	Tue, 13 Mar 2012 12:45:12 +0100
From:	Tobias Klauser <tklauser@...tanz.ch>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Matt Mooney <mfm@...eddisk.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: ping: drivers/staging/usbip/ abuses task_is_dead/exit_state

On 2012-03-08 at 19:57:39 +0100, Oleg Nesterov <oleg@...hat.com> wrote:
> On 03/06, Tobias Klauser wrote:
> >
> > On 2012-03-06 at 18:39:25 +0100, Oleg Nesterov <oleg@...hat.com> wrote:
> > >
> > > OK, since nobody cares, probably I should make the patch even if I don't
> > > understand this code at all and can't test the change.
> > >
> > > But, Tobias, may be you can explain what this task_is_dead() check was
> > > supposed to do?
> >
> > As mentioned in the commit message, this was needed for me to work
> > around a NULL pointer dereference I got during unbinding
> 
> Where? OK, I guess you do not remember the trace ;)

Yup, sorry I can't precisely recall where it happened anymore.

> > (I only
> > experienced this behaviour on the nios2 platform though, couldn't
> > reproduce it on e.g. x86_64).
> 
> OK,
> 
> > I wasn't really familiar with the codebase of usbip (and still am not)
> > but came up with the fix by more or less blindly copying what the
> > opposite side is checking for in stub_shutdown_connection(). This fixed
> > the behaviour for me and seemed legitimate as it was done equally there.
> 
> But this looks "obviously wrong", and afaics just hides the problem.
> Not to mention this check is racy, it is simply unsafe to dereference
> this task_struct if the kthread has already exited.
> 
> At first glance we need something like the patch below (and stub_dev.c
> needs the same fix). It assumes that:
> 
> 	- vhci_shutdown_connection() is the only caller of kthread_stop(),
> 	  iow nobody else does kthread_stop(tcp_*x)
> 
> 	- we can't leak the task_struct, vhci_shutdown_connection() should
> 	  be called in any case at some point.
> 
> I'll try to grep more, but perhaps you can ack my understanding?

Looks OK to me at a first glance. I'll try to dig up my test system
where I was able to produce that NULL pointer dereference and test your
patch there and will then let you know my results.

Thanks
Tobias
--
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/