linux-kernel - Re: [PATCH RESEND 2/2] Fix some kallsyms

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:	Fri, 16 Mar 2007 16:16:10 +0000
From:	Paulo Marques <pmarques@...popie.com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Alexey Dobriyan <adobriyan@...ru>, akpm@...l.org,
	linux-kernel@...r.kernel.org, devel@...nvz.org, tglx@...utronix.de,
	viro@...iv.linux.org.uk, rusty@...tcorp.com.au
Subject: Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races

Ingo Molnar wrote:
> * Alexey Dobriyan <adobriyan@...ru> wrote:
> 
>> [cc'ing folks whose proc files are affected]
>>
>> kallsyms_lookup() can call module_address_lookup() which iterates over 
>> modules list without module_mutex taken. Comment at the top of 
>> module_address_lookup() says it's for oops resolution so races are 
>> irrelevant, but in some cases it's reachable from regular code:
> 
> looking at the problem from another angle: wouldnt this be something 
> that would benefit from freeze_processes()/unfreeze_processes(), and 
> hence no locking would be required?

I also considered this, but it seemed a little too "blunt" to stop 
everything (including completely unrelated processes and kernel threads) 
just to remove a module.

How about something like this instead?  (just for review)

It's a little more intrusive, since the interface for symbol lookup is 
changed (and it affects the callers), but it makes the caller aware that 
  it should set "safe_to_lock" if possible.

This way we avoid exposing module_mutex outside of module.c and avoid 
returning any data pointer to some data structure that might disappear 
before the caller tries to use it.

Some of these changes are actually cleanups, because the callers where 
creating a dummy modname variables that they didn't use afterwards.

The thing I like the less about this patch is the increase of stack 
usage on some code paths by about 60 bytes.

Anyway, I don't have very strong feelings about this, so if you think it 
would be better to use freeze_processes()/unfreeze_processes(), I can 
cook up a patch for that too...

-- 
Paulo Marques - www.grupopie.com

"Very funny Scotty. Now beam up my clothes."

diffstat:
>  arch/parisc/kernel/unwind.c |    3 --
>  arch/powerpc/xmon/xmon.c    |   11 ++++-----
>  arch/ppc/xmon/xmon.c        |    8 +++---
>  arch/sh64/kernel/unwind.c   |    4 +--
>  arch/x86_64/kernel/traps.c  |   10 ++++----
>  fs/proc/base.c              |    3 --
>  include/linux/kallsyms.h    |    6 +++-
>  include/linux/module.h      |   44 +++++++++++++++++++-----------------
>  kernel/kallsyms.c           |   53 +++++++++++++++++++++-----------------------
>  kernel/kprobes.c            |    6 ++--
>  kernel/lockdep.c            |    3 --
>  kernel/module.c             |   44 +++++++++++++++++++++++++++---------
>  kernel/time/timer_list.c    |    3 --
>  kernel/time/timer_stats.c   |    3 --
>  mm/slab.c                   |    6 ++--
>  15 files changed, 114 insertions(+), 93 deletions(-)

View attachment "patch" of type "text/plain" (18450 bytes)