Re: [PATCH 4/5] ipc/msg: Lockless security checks for msgsnd

From: Davidlohr Bueso
Date: Wed Sep 21 2016 - 18:21:37 EST

On Sun, 18 Sep 2016, Manfred Spraul wrote:

Just as with msgrcv (along with the rest of sysvipc since a few years
ago), perform the security checks without holding the ipc object lock.
Thinking about it: isn't this wrong?

* msgrcv()
* ipcperms()

* msgctl(), change permissions
** msgctl() returns, new permissions should now be in effect
* msgsnd(), send secret message
** msgsnd() returns, new message stored.

CPU1: resumes, receives secret message

Hmm, would this not apply to everything IPC_SET, we do lockless ipcperms()
all over the place.

Obviously, we could argue that the msgrcv() was already ongoing and therefore the old permissions still apply - but then we don't need to recheck after sleeping at all.

There is that, and furthermore we make no such guarantees under concurrency.
Another way of looking at it could perhaps be IPC_SET returning EPERM if
there's an unserviced msgrcv -- but I'm not suggesting doing this btw ;)

This also reduces the hogging of the lock for the entire duration of a
sender, as we drop the lock upon every iteration -- and this is exactly
why we also check for racing with RMID in the first place.

Which hogging do you mean? The lock is dopped uppon every iteration, the schedule() is in the middle.
Which your patch, the lock are now dropped twice:
for (;;) {
struct msg_sender s;
err = -EACCES;
if (ipcperms(ns, &msq->q_perm, S_IWUGO))
- goto out_unlock0;
+ goto out_unlock1;
+ ipc_lock_object(&msq->q_perm);
/* raced with RMID? */
if (!ipc_valid_object(&msq->q_perm)) {
@@ -681,6 +681,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
goto out_unlock0;
+ ipc_unlock_object(&msq->q_perm);

This means the lock is dropped, just for ipcperms().
This doubles the lock acquire/release cycles.

The effectiveness all depends on the workload and degree of contention. But
I have no problem dropping this patch either, although this is standard for
all things ipc.