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]

Message-ID: <a2109821-861c-7b9c-d718-e035a2b1d73d@ispras.ru>
Date:   Sat, 7 Oct 2017 20:59:27 +0300
From:   Alexey Khoroshilov <khoroshilov@...ras.ru>
To:     Evgeniy Polyakov <zbr@...emap.net>,
        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 Evgeniy,

mutex_lock() and atomic_inc() are not nested currently:

  ret = mutex_lock_interruptible(&dev->bus_mutex);
  ...
  atomic_inc(THERM_REFCNT(family_data));

  ...

  mutex_unlock(&dev->bus_mutex);
  ...
  atomic_dec(THERM_REFCNT(family_data));

As a result, error handling without returns will be still quite messy.

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
?

--
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;
>>          }
>