linux-kernel - Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer

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: <alpine.DEB.2.02.1403240027020.18573@ionos.tec.linutronix.de>
Date:	Mon, 24 Mar 2014 00:37:52 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Julia Lawall <julia.lawall@...6.fr>
cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [patch 00/16] timers: Plug debugobject leaks and use del_timer_sync()
 in exit/teardown

On Sun, 23 Mar 2014, Julia Lawall wrote:
> As far as I could tell, (part of) the issue is that any kind of exit or 
> close function should use del_timer_sync, because they could be called 
> from a different CPU than the one that started up the timer?
> 
> Here is a semantic patch that takes care of the case of simple module_exit 
> functions:
> 
> @r@
> declarer name module_exit;
> identifier ex;
> @@
> 
> module_exit(ex);
> 
> @@
> identifier r.ex;
> @@
> 
> ex(...) {
>   <...
> - del_timer
> + del_timer_sync
>     (...)
>   ...>
> }
> 
> The transformations it makes are below.  I haven't had a chance to check 
> which results overlap with what Thomas has already sent, but I could look 

Minimal overlap, but as I said I did just a few conversions.

> into it if this is the right idea.  I guess other kinds of close/exit 
> functions would have to be identified manually, to make more rules.

If you look through the examples I sent, you'll find the close()
callbacks of various devices. So everything which is a function
pointer to a ops->close(), ops->remove(), ops_>teardown() is dangerous
if using del_timer(). There are a few exceptions, but....

Another thing I saw is 

	del_timer(&bla->timer);
	....
	kfree(&bla);

That's also a potential source of trouble. You can't tell without
analyzing the code, whether it's a problem or not. But making the
responsible people to look at it is definitely a good thing.
 
> In what circumstance can one be sure that two instructions are executed on 
> the same CPU?

If interrupts or preemption are disabled. But that's not the issue at
hand.

The del_timer vs. del_timer_sync problem is:

CPU0	      	  		 CPU1

add_timer(&bla->timer);

				 close(bla)
timer expires		   	   del_timer(&bla->timer);
  callback is invoked
				   kfree(bla);
    derefernces bla

I'll look at your findings on Tuesday, but feel free to send them to
the relevant maintainers for review.

Thanks,

	tglx
--
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/