linux-kernel - Re: [PATCH] w1: keep balance of mutex locks and refcnts

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:   Mon, 09 Oct 2017 22:13:09 +0300
From:   Evgeniy Polyakov <zbr@...emap.net>
To:     Alexey Khoroshilov <khoroshilov@...ras.ru>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ldv-project@...uxtesting.org" <ldv-project@...uxtesting.org>
Subject: Re: [PATCH] w1: keep balance of mutex locks and refcnts

Hi Alexey

07.10.2017, 20:59, "Alexey Khoroshilov" <khoroshilov@...ras.ru>:
> Is it possible to switch to a nested variant:
> mutex_lock-atomic_inc-atomic_dec-mutex_unlock
> or
> atomic_inc-mutex_lock-mutex_unlock-atomic_dec
> ?

Yeah, you are right, it is a bit messy - we drop the lock while sleeping waiting for the bus master to complete operation,
and during this period family driver has to be referenced.

But we can easily grab the reference earlier and then try to lock the bus, so the second variant will work.

> --
> Alexey
>
> On 01.10.2017 08:55, Evgeniy Polyakov wrote:
>>  Hi Alex
>>
>>  29.09.2017, 23:23, "Alexey Khoroshilov" <khoroshilov@...ras.ru>:
>>>  w1_therm_eeprom() and w1_DS18B20_precision() decrement THERM_REFCNT
>>>  on error paths, while they did not increment it yet.
>>>
>>>  read_therm() unlocks bus mutex on some error paths,
>>>  while it is not acquired.
>>>
>>>  The patch makes sure all the functions keep the balance in usage of
>>>  the mutex and the THERM_REFCNT.
>>>
>>>  Found by Linux Driver Verification project (linuxtesting.org).
>>
>>  Yes, this looks like a bug, thanks for finding it!
>>
>>  Please update your patch to use single exit point and not a mix of returns in the body of the function.
>>
>>>           ret = mutex_lock_interruptible(&dev->bus_mutex);
>>>           if (ret != 0)
>>>  - goto post_unlock;
>>>  + return ret;
>>>
>>>           if (!sl->family_data) {
>>>  - ret = -ENODEV;
>>>  - goto pre_unlock;
>>>  + mutex_unlock(&dev->bus_mutex);
>>>  + return -ENODEV;
>>>           }