linux-kernel - Re: [PATCH 4.4 089/105] media: dvb_frontend: fix locking issues at dvb_frontend_get

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: <1532112984.21552.39.camel@codethink.co.uk>
Date:   Fri, 20 Jul 2018 19:56:24 +0100
From:   Ben Hutchings <ben.hutchings@...ethink.co.uk>
To:     Mauro Carvalho Chehab <mchehab@...pensource.com>
Cc:     stable@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4.4 089/105] media: dvb_frontend: fix locking issues at
 dvb_frontend_get_event()

On Sun, 2018-07-01 at 18:02 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Mauro Carvalho Chehab <mchehab@...pensource.com>
> 
> commit 76d81243a487c09619822ef8e7201a756e58a87d upstream.
> 
> As warned by smatch:
> 	drivers/media/dvb-core/dvb_frontend.c:314 dvb_frontend_get_event() warn: inconsistent returns 'sem:&fepriv->sem'.
> 	  Locked on:   line 288
> 	               line 295
> 	               line 306
> 	               line 314
> 	  Unlocked on: line 303
> 
> The lock implementation for get event is wrong, as, if an
> interrupt occurs, down_interruptible() will fail, and the
> routine will call up() twice when userspace calls the ioctl
> again.
> 
> The bad code is there since when Linux migrated to git, in
> 2005.
[...] 
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
[...]
> +static int dvb_frontend_test_event(struct dvb_frontend_private *fepriv,
> +				   struct dvb_fe_events *events)
> +{
> +	int ret;
> +
> +	up(&fepriv->sem);
> +	ret = events->eventw != events->eventr;
> +	down(&fepriv->sem);
> +
> +	return ret;
> +}

This is broken.  You must not wait inside a wait condition.  (It would
be nice if this rule was explicitly documented.)

Why not leave the condition as it was and only change the
down_interruptible() to down()?

Ben.

>  static int dvb_frontend_get_event(struct dvb_frontend *fe,
> -			    struct dvb_frontend_event *event, int flags)
> +			          struct dvb_frontend_event *event, int flags)
>  {
>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>  	struct dvb_fe_events *events = &fepriv->events;
> @@ -249,13 +261,8 @@ static int dvb_frontend_get_event(struct
>  		if (flags & O_NONBLOCK)
>  			return -EWOULDBLOCK;
>  
> -		up(&fepriv->sem);
> -
> -		ret = wait_event_interruptible (events->wait_queue,
> -						events->eventw != events->eventr);
> -
> -		if (down_interruptible (&fepriv->sem))
> -			return -ERESTARTSYS;
> +		ret = wait_event_interruptible(events->wait_queue,
> +					       dvb_frontend_test_event(fepriv, events));
>  
>  		if (ret < 0)
>  			return ret;
> 
> 
> 
-- 
Ben Hutchings, Software Developer                         Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom