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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <63054a6a-d0ea-c112-8422-0424e5dee704@redhat.com>
Date:   Mon, 11 Mar 2019 10:14:28 -0400
From:   Waiman Long <longman@...hat.com>
To:     Matthew Wilcox <willy@...radead.org>
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 03/10/2019 11:35 PM, Matthew Wilcox wrote:
> 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 kern_ipc_perm object isn't uninitialized. The seq value, however, is
tentative rather than final.

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

That will work too, I think. My only concern is that it will slow down
ipc_idr_alloc() process as each idr_*() call can be pretty expensive.
Even though it is in the slow path, we still don't want to introduce
unnecessary overhead.

Actually, it is no different from what ipc_idr_alloc() used to be. The
seq value in kern_ipc_perm was updated every time ipc_idr_alloc() was
called. So older IPC id would fail ipc_checkid(). This patch guarantees
that it will happen again. The new IPC id won't be returned until the
kern_ipc_perm object is correctly set. So there is no danger of new IPC
id failing the test. I will update my patch to better discuss the
rationale for this change.

Cheers,
Longman