lkml.org 
[lkml]   [2016]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/5] ipc/msg: Lockless security checks for msgsnd
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?
>
>CPU1:
>* msgrcv()
>* ipcperms()
><sleep>
>
>CPU2:
>* 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.

Thanks,
Davidlohr

\
 
 \ /
  Last update: 2016-09-23 00:00    [W:0.041 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site