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]
Date:	Thu, 26 Sep 2013 14:08:08 -0300
From:	Mauro Carvalho Chehab <m.chehab@...sung.com>
To:	Alexey Khoroshilov <khoroshilov@...ras.ru>
Cc:	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	ldv-project@...uxtesting.org
Subject: Re: [PATCH] [media] dvb_demux: fix deadlock in
 dmx_section_feed_release_filter()

Em Sat, 17 Aug 2013 23:48:51 +0300
Alexey Khoroshilov <khoroshilov@...ras.ru> escreveu:

> dmx_section_feed_release_filter() locks dvbdmx->mutex and
> if the feed is still filtering, it calls feed->stop_filtering(feed).
> stop_filtering() is implemented by dmx_section_feed_stop_filtering()
> that first of all try to lock the same mutex: dvbdmx->mutex.
> That leads to a deadlock.
> 
> It does not happen often in practice because all callers of
> release_filter() stop filtering by themselves.
> So the problem can happen in case of race condition only.
> 
> The patch proposes to unlock dvbdmx->mutex before call feed->stop_filtering(feed)
> and recheck feed->is_filtering after reacquiring mutex.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@...ras.ru>
> ---
>  drivers/media/dvb-core/dvb_demux.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_demux.c b/drivers/media/dvb-core/dvb_demux.c
> index 3485655..9d517af 100644
> --- a/drivers/media/dvb-core/dvb_demux.c
> +++ b/drivers/media/dvb-core/dvb_demux.c
> @@ -1027,8 +1027,11 @@ static int dmx_section_feed_release_filter(struct dmx_section_feed *feed,
>  		return -EINVAL;
>  	}
>  
> -	if (feed->is_filtering)
> +	while (feed->is_filtering) {

Changing from if to while here can cause a dead lock, as, if the device
got removed, dmx_section_feed_stop_filtering() won't touch
feed->is_filtering. So, the loop will happen forever.

Except for that the patch looks correct on my eyes.

> +		mutex_unlock(&dvbdmx->mutex);

Please add a small comment about why the mutex needs to be unlocked
there, as this looks ugly, and likely deserve some cleanups in the future
(as both the spinclock and the mutex are taken on both callback and at
the release filter function).

>  		feed->stop_filtering(feed);
> +		mutex_lock(&dvbdmx->mutex);
> +	}
>  
>  	spin_lock_irq(&dvbdmx->lock);
>  	f = dvbdmxfeed->filter;


Thanks!
Mauro
--
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/

Powered by blists - more mailing lists