linux-kernel - Re: [PATCH-next] ipc: Fix race condition in ipc_idr

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:   Sun, 10 Mar 2019 20:35:10 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Waiman Long <longman@...hat.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org,
        Al Viro <viro@...iv.linux.org.uk>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        Takashi Iwai <tiwai@...e.de>, Davidlohr Bueso <dbueso@...e.de>,
        Manfred Spraul <manfred@...orfullife.com>
Subject: Re: [PATCH-next] ipc: Fix race condition in ipc_idr_alloc()

On Sun, Mar 10, 2019 at 09:25:36PM -0400, Waiman Long wrote:
> @@ -221,15 +221,34 @@ static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
>  	 */
>  
>  	if (next_id < 0) { /* !CHECKPOINT_RESTORE or next_id is unset */
> +		/*
> +		 * It is possible that another thread of the same
> +		 * kern_ipc_perm may have called ipc_obtain_object_check()
> +		 * concurrently with a recently deleted IPC id (idx|seq).
> +		 * If idr_alloc() happens to allocate this deleted idx value,
> +		 * the other thread may incorrectly get a handle to the new
> +		 * IPC id.
> +		 *
> +		 * To prevent this race condition from happening, we will
> +		 * always store a new sequence number into the kern_ipc_perm
> +		 * object before calling idr_alloc(). If we find out that we
> +		 * don't need to change seq, we write back the right value.
> +		 */
> +		new->seq = ids->seq + 1;
> +		if (new->seq > IPCID_SEQ_MAX)
> +			new->seq = 0;
> +
>  		if (ipc_mni_extended)
>  			idx = idr_alloc_cyclic(&ids->ipcs_idr, new, 0, ipc_mni,
>  						GFP_NOWAIT);
>  		else
>  			idx = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
>  
> -		if ((idx <= ids->last_idx) && (++ids->seq > IPCID_SEQ_MAX))
> -			ids->seq = 0;
> -		new->seq = ids->seq;
> +		/* Make ids->seq and new->seq stay in sync */
> +		if (idx <= ids->last_idx)
> +			ids->seq = new->seq;
> +		else
> +			new->seq = ids->seq;

This can't possibly be right.  It's no better to occasionally find the
wrong ID than to find an uninitialised ID.

The normal pattern for solving this kind of problem is to idr_alloc()
a NULL pointer, initialise new->seq, then call idr_replace() to turn
that NULL pointer into the actual pointer you want.