linux-kernel - Re: [PATCH] shm: fix a race between shm_exit() and shm

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:	Tue, 2 Aug 2011 13:33:56 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Vasiliy Kulikov <segoon@...nwall.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Manuel Lauss <manuel.lauss@...glemail.com>,
	linux-kernel@...r.kernel.org, Richard Weinberger <richard@....at>,
	Marc Zyngier <maz@...terjones.org>,
	Ingo Molnar <mingo@...e.hu>,
	kernel-hardening@...ts.openwall.com,
	"Paul E. McKenney" <paul.mckenney@...aro.org>
Subject: Re: [PATCH] shm: fix a race between shm_exit() and shm_init()

On Tue, 2 Aug 2011 16:45:30 +0400
Vasiliy Kulikov <segoon@...nwall.com> wrote:

> On thread exit shm_exit() is called, it uses shm_ids(ns).rw_mutex.

You meant shm_exit_ns().

> It is initialized in shm_init(), but it is not called yet at the moment
> of kernel threads exit.  Some kernel threads are created in
> do_pre_smp_initcalls(), and shm_init() is called in do_initcalls().
> 
> Static initialization of shm_ids(init_ipc_ns).rw_mutex fixes the race.
> 
> It fixes a kernel oops:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> ...
> [<c0320090>] (__down_write_nested+0x88/0xe0) from [<c015da08>] (exit_shm+0x28/0x48)
> [<c015da08>] (exit_shm+0x28/0x48) from [<c002e550>] (do_exit+0x59c/0x750)
> [<c002e550>] (do_exit+0x59c/0x750) from [<c003eaac>] (____call_usermodehelper+0x13c/0x154)
> [<c003eaac>] (____call_usermodehelper+0x13c/0x154) from [<c000f630>] (kernel_thread_exit+0x0/0x8)
> Code: 1afffffa e597c00c e58d0000 e587d00c (e58cd000)
> 
> ...
>
> --- a/ipc/msgutil.c
> +++ b/ipc/msgutil.c
> @@ -20,6 +20,9 @@
>  
>  DEFINE_SPINLOCK(mq_lock);
>  
> +#define INIT_IPC_SHM_IDS(name) \
> +	{ .rw_mutex = __RWSEM_INITIALIZER(name.rw_mutex), }
> +
>  /*
>   * The next 2 defines are here bc this is the only file
>   * compiled when either CONFIG_SYSVIPC and CONFIG_POSIX_MQUEUE
> @@ -27,6 +30,9 @@ DEFINE_SPINLOCK(mq_lock);
>   */
>  struct ipc_namespace init_ipc_ns = {
>  	.count		= ATOMIC_INIT(1),
> +	.ids	= {
> +		[IPC_SHM_IDS] = INIT_IPC_SHM_IDS(init_ipc_ns.ids[IPC_SHM_IDS]),
> +	},

That's what I meant by "nasty".  We initialise one field because we
happen to use that one at the wrong time, and leave everything else
uninitialised. eww.

But in this case it's not as bad as it might be -
shm_exit_ns()->free_ipcs() is a no-op because ids[2].inuse is zero, so
we kinda _did_ initialise that.  otoh we left ids[0].rw_mutex and
ids[1].rw_mutex uninitialised, so it's still nasty ;)

We could perhaps have fixed the bug by testing ids->inuse before taking
the mutex, which would also have been a speedup for that function. 
That would need some thought.

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