Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4] ipc/mqueue, msg, sem: Avoid relying on a stack
 reference past its expiry
To:     Varad Gautam <varad.gautam@suse.com>, linux-kernel@vger.kernel.org
Cc:     Matthias von Faber <matthias.vonfaber@aox-tech.de>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Oleg Nesterov <oleg@redhat.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Davidlohr Bueso <dbueso@suse.de>,
        James Morris <jamorris@linux.microsoft.com>,
        Serge Hallyn <serge@hallyn.com>,
        Jules Irenge <jbi.octave@gmail.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Alexey Dobriyan <adobriyan@gmail.com>
References: <20210510102950.12551-1-varad.gautam@suse.com>
From:   Manfred Spraul <manfred@colorfullife.com>
Message-ID: <d1360f03-40ef-7930-e8af-9a3c77fdb5de@colorfullife.com>
Date:   Wed, 12 May 2021 20:55:14 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20210510102950.12551-1-varad.gautam@suse.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

Hi Varad,

On 5/10/21 12:29 PM, Varad Gautam wrote:
> do_mq_timedreceive calls wq_sleep with a stack local address. The
> sender (do_mq_timedsend) uses this address to later call
> pipelined_send.
>
> This leads to a very hard to trigger race where a do_mq_timedreceive call
> might return and leave do_mq_timedsend to rely on an invalid address,
> causing the following crash:
[...]
> Fixes: c5b2cbdbdac563 ("ipc/mqueue.c: update/document memory barriers")
> Fixes: 8116b54e7e23ef ("ipc/sem.c: document and update memory barriers")
> Fixes: 0d97a82ba830d8 ("ipc/msg.c: update and document memory barriers")
> Signed-off-by: Varad Gautam <varad.gautam@suse.com>
> Reported-by: Matthias von Faber <matthias.vonfaber@aox-tech.de>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Manfred Spraul <manfred@colorfullife.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Davidlohr Bueso <dbueso@suse.de>
> Cc: Manfred Spraul <manfred@colorfullife.com>
> ---
> v2: Call wake_q_add before smp_store_release, instead of using a
>      get_task_struct/wake_q_add_safe combination across
>      smp_store_release. (Davidlohr Bueso)
> v3: Comment/commit message fixup.
> v4: - v2 / v3 have potential for introducing lost wakeups. Return to v1
>      as the path-of-least-surprises to fix the race at hand.
>      - Also fix ipc/msg.c and ipc/sem.c which have the same usage
>      pattern.(Manfred Spraul)
>
Acked-by: Manfred Spraul <manfred@colorfullife.com>

I would recommend that you add cc:stable. The patch is obvious, and it 
is a bugfix.

>   
> diff --git a/ipc/sem.c b/ipc/sem.c
> index f6c30a85dadf..7d9c06b0ad6e 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -784,12 +784,14 @@ static int perform_atomic_semop(struct sem_array *sma, struct sem_queue *q)
>   static inline void wake_up_sem_queue_prepare(struct sem_queue *q, int error,
>   					     struct wake_q_head *wake_q)
>   {
> -	get_task_struct(q->sleeper);
> +	struct task_struct *sleeper;
> +
> +	sleeper = get_task_struct(q->sleeper);
>   
>   	/* see SEM_BARRIER_2 for purpuse/pairing */

This collides with a spelling correction.
s/purpuse/purpose/ required.


--

     Manfred