linux-kernel - Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC

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: <1387381587.2797.33.camel@buesod1.americas.hpqcorp.net>
Date:	Wed, 18 Dec 2013 07:46:27 -0800
From:	Davidlohr Bueso <davidlohr@...com>
To:	Rafael Aquini <aquini@...hat.com>
Cc:	Manfred Spraul <manfred@...orfullife.com>,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Greg Thelen <gthelen@...gle.com>
Subject: Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out
 IPC_RMID races

On Wed, 2013-12-18 at 10:51 -0200, Rafael Aquini wrote:
> On Wed, Dec 18, 2013 at 01:11:29PM +0100, Manfred Spraul wrote:
> > On 12/18/2013 12:28 AM, Rafael Aquini wrote:
> > >After the locking semantics for the SysV IPC API got improved, a couple of
> > >IPC_RMID race windows were opened because we ended up dropping the
> > >'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > >The spotted races got sorted out by re-introducing the old test within
> > >the racy critical sections.
> > >
> > >This patch introduces ipc_valid_object() to consolidate the way we cope with
> > >IPC_RMID races by using the same abstraction across the API implementation.
> > >
> > >Signed-off-by: Rafael Aquini <aquini@...hat.com>
> > >Acked-by: Rik van Riel <riel@...hat.com>
> > >Acked-by: Greg Thelen <gthelen@...gle.com>
> > >---
> > >Changelog:
> > >* v2:
> > >  - drop assert_spin_locked() from ipc_valid_object() for less overhead
> > a) sysv ipc is lockless whereever possible, without writing to any
> > shared cachelines.
> > Therefore my first reaction was: No, please leave the assert in. It
> > will help us to catch bugs.
> > 
> > b) then I noticed: the assert would be a bug, the comment in front
> > of ipc_valid_object() that the caller must hold _perm.lock is wrong:
> > >@@ -1846,7 +1846,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
> > >  	error = -EIDRM;
> > >  	locknum = sem_lock(sma, sops, nsops);
> > >-	if (sma->sem_perm.deleted)
> > >+	if (!ipc_valid_object(&sma->sem_perm))
> > >  		goto out_unlock_free;
> > simple semtimedop() operation do not acquire sem_perm.lock, they
> > only acquire the per-semaphore lock and check that sem_perm.lock is
> > not held. This is sufficient to prevent races with RMID.
> > 
> > Could you update the comment?
> 
> The comment for ipc_valid_object() is not entirely wrong, as holding the spinlock 
> is clearly necessary for all cases except for this one you pointed above. 
> When I dropped the assert as Davilohr suggested, I then could have this one exception 
> case (where the check can, eventually, be done lockless) converted too, but I did not include 
> an exception comment at that particular checkpoint. Perhaps, that's what I should have done, or
> perhaps the best thing is to just let all that as is sits right now.

Yeah, Manfred is entirely correct - I didn't mention that sem_lock()
tries to be fine grained about its locking, so semaphores can in fact
not take the larger ipc lock (kern perm), but just the sem->lock
instead. This means that ipc_valid_object() must be called either way
with some lock held, but that assertion is indeed incorrect, not just
redundant like I suggested before. So, I think that if you update the
comment mentioning this corner case, then it should be ok.

Thanks,
Davidlohr

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