Re: [PATCH v4] ipc/mqueue, msg, sem: Avoid relying on a stack reference past its expiry

[Manfred Spraul]

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@xxxxxxxx>
> Reported-by: Matthias von Faber <matthias.vonfaber@xxxxxxxxxxx>
> Cc: Christian Brauner <christian.brauner@xxxxxxxxxx>
> Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> Cc: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Davidlohr Bueso <dbueso@xxxxxxx>
> Cc: Manfred Spraul <manfred@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>

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