From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S935749AbcIVTnB (ORCPT <rfc822;w@1wt.eu>);
        Thu, 22 Sep 2016 15:43:01 -0400
Received: from mail-wm0-f68.google.com ([74.125.82.68]:35010 "EHLO
        mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S933053AbcIVTm7 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 22 Sep 2016 15:42:59 -0400
Subject: Re: [PATCH 4/5] ipc/msg: Lockless security checks for msgsnd
To: Davidlohr Bueso <dave@stgolabs.net>
References: <58c2536e-aebd-e21e-6d78-003dcd10443d@colorfullife.com>
 <20160921222122.GA13358@linux-80c1.suse>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>, akpm@linux-foundation.org
From: Manfred Spraul <manfred@colorfullife.com>
Message-ID: <0106cf74-e308-fc20-b76f-e8a6253c3d66@colorfullife.com>
Date: Thu, 22 Sep 2016 21:42:54 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <20160921222122.GA13358@linux-80c1.suse>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 09/22/2016 12:21 AM, Davidlohr Bueso wrote:
> 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 ;)
I looked at it by comparing how we handle ipcperms and 
security_msg_queue_xx():
The security_msg_queue_xx are called under the lock, or actually the 
msgrcv even looks at the message that is transferred.

For ipc/sem.c and ipc/shm.c, both operations are called outside the lock.

So, my proposal would be that ipcperms and security_xx should show the 
same behavior regarding concurrent ops.
And this would mean that we should not move ipcperms() in msgrcv outside 
of the lock, i.e. drop the patch.

But if you have a better rational, feel free. But please document it in 
the changelog.

--
     Manfred