linux-kernel - Re: [PATCH v2] iio: inkern: fix endless loop in error path

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] [day] [month] [year] [list]

Date:   Mon, 24 Apr 2017 11:25:06 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Jonathan Cameron <jic23@...nel.org>, <linux-kernel@...r.kernel.org>
CC:     Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2] iio: inkern: fix endless loop in error path

Arrggggh!

Actually, this updated description is not at all accurate! It is still pretty
nasty to loop back and unlock the mutex again, but the loop isn't endless.
The code will only jump back once. That's of course bad enough, but there's
also the fact that the bug can only happen if type is not IIO_VAL_INT. And
IIUC, that really shouldn't be the case for raw values? Because inkern.c
iio_write_channel_raw() certainly implies it. If raw values really are
IIO_VAL_INT *always*, then the double unlock should never trigger.

And in that case the description of the original v1 patch is much more
accurate, since it really mostly is about a static checker issue.

So, please ignore this update and pick the original patch instead. I.e.
https://lkml.org/lkml/2017/4/20/887

Sorry for the noise.

Cheers,
peda

On 2017-04-24 10:57, Peter Rosin wrote:
> If ret ends up negative, mutex_unlock is called repeatedly in an infinite
> loop, which of course is pretty nasty...
> 
> Issue found by smatch:
> drivers/iio/inkern.c:751 iio_read_avail_channel_raw() error: double unlock 'mutex:&chan->indio_dev->info_exist_lock'
> 
> Fixes: 00c5f80c2fad ("iio: inkern: add helpers to query available values from channels")
> Signed-off-by: Peter Rosin <peda@...ntia.se>
> ---
>  drivers/iio/inkern.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> v1 -> v2 changes:
> - Be clear about that there is a real nasty issue and that it isn't only
>   about avoiding some insignificant static checker issue.
> 
> v1:  https://lkml.org/lkml/2017/4/20/887
> 
> Cheers,
> peda
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 7a13535dc3e9..a3941bade6a7 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -750,11 +750,9 @@ int iio_read_avail_channel_raw(struct iio_channel *chan,
>  err_unlock:
>  	mutex_unlock(&chan->indio_dev->info_exist_lock);
>  
> -	if (ret >= 0 && type != IIO_VAL_INT) {
> +	if (ret >= 0 && type != IIO_VAL_INT)
>  		/* raw values are assumed to be IIO_VAL_INT */
>  		ret = -EINVAL;
> -		goto err_unlock;
> -	}
>  
>  	return ret;
>  }
>