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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 12 May 2021 20:55:14 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     Varad Gautam <varad.gautam@...e.com>, linux-kernel@...r.kernel.org
Cc:     Matthias von Faber <matthias.vonfaber@...-tech.de>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Oleg Nesterov <oleg@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Davidlohr Bueso <dbueso@...e.de>,
        James Morris <jamorris@...ux.microsoft.com>,
        Serge Hallyn <serge@...lyn.com>,
        Jules Irenge <jbi.octave@...il.com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Alexey Dobriyan <adobriyan@...il.com>
Subject: Re: [PATCH v4] ipc/mqueue, msg, sem: Avoid relying on a stack
 reference past its expiry

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@...e.com>
> Reported-by: Matthias von Faber <matthias.vonfaber@...-tech.de>
> Cc: Christian Brauner <christian.brauner@...ntu.com>
> Cc: Oleg Nesterov <oleg@...hat.com>
> Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
> Cc: Manfred Spraul <manfred@...orfullife.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Davidlohr Bueso <dbueso@...e.de>
> Cc: Manfred Spraul <manfred@...orfullife.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@...orfullife.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