linux-kernel - Re: [PATCH] sigqueue_free: fix the race with collect

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <46CDFDD2.4010600@us.ibm.com>
Date:	Thu, 23 Aug 2007 14:36:18 -0700
From:	Sukadev Bhattiprolu <sukadev@...ibm.com>
To:	Oleg Nesterov <oleg@...sign.ru>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Alexey Dobriyan <adobriyan@...ru>, Ingo Molnar <mingo@...e.hu>,
	Jeremy Katz <jeremy.katz@...driver.com>,
	taoyue <yue.tao@...driver.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org, stable@...nel.org
Subject: Re: [PATCH] sigqueue_free: fix the race with collect_signal()

Oleg Nesterov wrote:
> Spotted by taoyue <yue.tao@...driver.com> and Jeremy Katz <jeremy.katz@...driver.com>.
>
> collect_signal:				sigqueue_free:
>
> 	list_del_init(&first->list);
> 						if (!list_empty(&q->list)) {
> 							// not taken
> 						}
> 						q->flags &= ~SIGQUEUE_PREALLOC;
>
> 	__sigqueue_free(first);			__sigqueue_free(q);
>
> Now, __sigqueue_free() is called twice on the same "struct sigqueue" with the
> obviously bad implications.
>
> In particular, this double free breaks the array_cache->avail logic, so the
> same sigqueue could be "allocated" twice, and the bug can manifest itself via
> the "impossible" BUG_ON(!SIGQUEUE_PREALLOC) in sigqueue_free/send_sigqueue.
>
> Hopefully this can explain these mysterious bug-reports, see
>
> 	http://marc.info/?t=118766926500003
> 	http://marc.info/?t=118466273000005
>
> Alexey Dobriyan reports this patch makes the difference for the testcase, but
> nobody has an access to the application which opened the problems originally.
>
> Also, this patch removes tasklist lock/unlock, ->siglock is enough.
>
> Signed-off-by: Oleg Nesterov <oleg@...sign.ru>
>
> --- t/kernel/signal.c~SQFREE	2007-08-22 20:06:31.000000000 +0400
> +++ t/kernel/signal.c	2007-08-23 16:02:57.000000000 +0400
> @@ -1297,20 +1297,19 @@ struct sigqueue *sigqueue_alloc(void)
>  void sigqueue_free(struct sigqueue *q)
>  {
>  	unsigned long flags;
> +	spinlock_t *lock = &current->sighand->siglock;
> +
>  	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
>  	/*
>  	 * If the signal is still pending remove it from the
> -	 * pending queue.
> +	 * pending queue. We must hold ->siglock while testing
> +	 * q->list to serialize with collect_signal().
>  	 */
> -	if (unlikely(!list_empty(&q->list))) {
> -		spinlock_t *lock = &current->sighand->siglock;
> -		read_lock(&tasklist_lock);
> -		spin_lock_irqsave(lock, flags);
>   
Hmm, but the existing code _does_ take the siglock here. Is that not 
sufficient ?
Isn't the first list_empty() check without lock only an optimization for 
the common
case ?

> -		if (!list_empty(&q->list))
> -			list_del_init(&q->list);
> -		spin_unlock_irqrestore(lock, flags);
> -		read_unlock(&tasklist_lock);
> -	}
> +	spin_lock_irqsave(lock, flags);
> +	if (!list_empty(&q->list))
> +		list_del_init(&q->list);
> +	spin_unlock_irqrestore(lock, flags);
> +
>  	q->flags &= ~SIGQUEUE_PREALLOC;
>  	__sigqueue_free(q);
>  }
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>   


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/