linux-kernel - Re: [PATCH] hysdn: fix to a race condition in put_log

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: <a82052eb-2aec-ffa2-2cfd-c02db779d695@cogentembedded.com>
Date:   Thu, 3 Aug 2017 15:03:13 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Anton Volkov <avolkov@...ras.ru>, isdn@...ux-pingi.de
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, ldv-project@...uxtesting.org,
        khoroshilov@...ras.ru
Subject: Re: [PATCH] hysdn: fix to a race condition in put_log_buffer

Hello!

On 08/03/2017 02:33 PM, Anton Volkov wrote:

> The synchronization type that was used earlier to guard the loop that
> deletes unused log buffers may lead to a situation that prevents any
> thread from going through the loop.
> 
> The patch deletes previously used synchronization mechanism and moves
> the loop under the spin_lock so the similar cases won't be feasible in
> the future.
> 
> Found by by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Anton Volkov <avolkov@...ras.ru>
> ---
>   drivers/isdn/hysdn/hysdn_proclog.c | 27 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
> index 7b5fd8f..b152c6c 100644
> --- a/drivers/isdn/hysdn/hysdn_proclog.c
> +++ b/drivers/isdn/hysdn/hysdn_proclog.c
[...]
>   	else
>   		pd->log_tail->next = ib;	/* follows existing messages */
>   	pd->log_tail = ib;	/* new tail */
> -	i = pd->del_lock++;	/* get lock state */
> -	spin_unlock_irqrestore(&card->hysdn_lock, flags);
>   
>   	/* delete old entrys */
> -	if (!i)
> -		while (pd->log_head->next) {
> -			if ((pd->log_head->usage_cnt <= 0) &&
> -			    (pd->log_head->next->usage_cnt <= 0)) {
> -				ib = pd->log_head;
> -				pd->log_head = pd->log_head->next;
> -				kfree(ib);
> -			} else
> -				break;
> -		}		/* pd->log_head->next */
> -	pd->del_lock--;		/* release lock level */
> +	while (pd->log_head->next) {
> +		if ((pd->log_head->usage_cnt <= 0) &&
> +		    (pd->log_head->next->usage_cnt <= 0)) {
> +			ib = pd->log_head;
> +			pd->log_head = pd->log_head->next;
> +			kfree(ib);
> +		} else
> +			break;
> +	}		/* pd->log_head->next */

    Need {} in the *else* branch as *if* had them.

[...]

MBR, Sergei