linux-kernel - Re: [PATCH] regulator: core: fix race condition in regulator

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: <20150107170646.GX2634@sirena.org.uk>
Date:	Wed, 7 Jan 2015 17:06:46 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Ashay Jaiswal <ashayj@...eaurora.org>
Cc:	Liam Girdwood <lgirdwood@...il.com>, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org,
	Anirudh Ghayal <aghayal@...eaurora.org>
Subject: Re: [PATCH] regulator: core: fix race condition in regulator_put()

On Wed, Jan 07, 2015 at 07:21:23PM +0530, Ashay Jaiswal wrote:

> The regulator framework maintains a list of consumer regulators
> for a regulator device and protects it from concurrent access
> using the regulator device's mutex lock.

> In the case of regulator_put() the consumer is removed without
> holding the regulator device's mutex, resulting in a race condition
> between any regulator operation which traverses the consumer list
> and regulator_put() which releases the consumer regulator.
> Fix this race condition by holding the regulator device's mutex while
> removing and releasing the consumer regulator.

This is a good spot thanks but I think your analysis here is missing a
bit - it's not just the list manipulation that affects the rdev, it's
also the reference count in the rdev and the exclusive flag.  Indeed
some of this issue applies on the _get() side too, while we do add the
regulator to the list under the rdev mutex we don't have the mutex when
we update the reference count meaning that we've got a potential issue
with that.  That *is* kind of separate though so could be dealt with in
a separate patch.

The lock region also seems too wide, the lock is only needed for the
operations that affect the rdev not for the operations only on the
object being freed - holding the lock for too long means impacting other
users and some of the cleanup is potentially expensive.  The comment at
the top of the function needs updating too, it currently says that the
lock is held in the caller but this applies only to the regulator_list_mutex.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)