[PATCH] driver core: Fix concurrency issue in match driver interface.

[PATCH] driver core: Fix concurrency issue in match driver interface.

[Qiu-ji Chen]

This patch identifies a deeper issue. Taking the function
fsl_mc_bus_match() as an example, since the function is not protected by a
lock, there is a data race between fsl_mc_bus_match() and
driver_override_store(). After the check if (mc_dev->driver_override), the
driver_override field may be set to NULL, but the strcmp() function does
not perform a NULL pointer check. If a NULL driver_override field is passed
into strcmp(), it may lead to a null pointer dereference issue. We also
noticed that in the driver_override_store() function, the function
driver_set_override() is called, and this function frees the old value
after storing the new dev value. If a race occurs, a use-after-free (UAF)
issue could occur in fsl_mc_bus_match().

To fix this issue, we examined the interface that calls this function and
found that fsl_mc_bus_match() is called by the interface drv->bus->match,
which in turn is called by the function driver_match_device(). We found
three places where driver_match_device() is called, and the call in
__driver_attach() is locked. However, since the driver_override_store()
function is implemented in drivers such as amba, base, cdx, and others,
this data race issue is quite common. Adding a lock at the lower level
could conflict with the lock in __driver_attach(), leading to a potential
deadlock. Therefore, we decided to add locking to the other two calls to
driver_match_device() to ensure that when the function is called through
the match interface, dev is protected by a lock throughout the process, and
its value remains unchanged.

Fixes: 49b420a13ff9 ("driver core: check bus->match without holding device lock")
Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com>
---
In 2008, the kernel moved all match checks outside of the lock, and at that
time, there was no override, so this approach worked. In 2015, one function
moved the match check into the lock on one of its paths in order to support
asynchronous device binding, which led to inconsistent behavior. Later,
some kernel drivers added the driver_override feature to support binding
specific drivers, and the driver_set_override function was widely used to
control it. This function uses device_lock to control concurrency. This
issue is caused by both lower-level drivers and upper-level drivers, so
I'm not sure if the fixes tag is correct, and I haven't added a cc. I hope
to discuss this issue with the developers.
---
 drivers/base/bus.c | 16 +++++++++++-----
 drivers/base/dd.c  |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 657c93c38b0d..78d8c58fc50c 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -261,13 +261,19 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
 	const struct bus_type *bus = bus_get(drv->bus);
 	struct device *dev;
 	int err = -ENODEV;
+	int ret;
 
 	dev = bus_find_device_by_name(bus, NULL, buf);
-	if (dev && driver_match_device(drv, dev)) {
-		err = device_driver_attach(drv, dev);
-		if (!err) {
-			/* success */
-			err = count;
+	if (dev) {
+		device_lock(dev);
+		ret = driver_match_device(drv, dev);
+		device_unlock(dev);
+		if (ret) {
+			err = device_driver_attach(drv, dev);
+			if (!err) {
+				/* success */
+				err = count;
+			}
 		}
 	}
 	put_device(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f0e4b4aba885..0b894719eb28 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1169,7 +1169,9 @@ static int __driver_attach(struct device *dev, void *data)
 	 * is an error.
 	 */
 
+	device_lock(dev);
 	ret = driver_match_device(drv, dev);
+	device_unlock(dev);
 	if (ret == 0) {
 		/* no match */
 		return 0;
-- 
2.34.1

Re: [PATCH] driver core: Fix concurrency issue in match driver interface.

[Greg KH]

On Wed, Nov 13, 2024 at 12:30:41AM +0800, Qiu-ji Chen wrote:
> This patch identifies a deeper issue.

I don't understand this sentence, sorry.

> Taking the function
> fsl_mc_bus_match() as an example, since the function is not protected by a
> lock, there is a data race between fsl_mc_bus_match() and
> driver_override_store(). After the check if (mc_dev->driver_override), the
> driver_override field may be set to NULL, but the strcmp() function does
> not perform a NULL pointer check. If a NULL driver_override field is passed
> into strcmp(), it may lead to a null pointer dereference issue. We also
> noticed that in the driver_override_store() function, the function
> driver_set_override() is called, and this function frees the old value
> after storing the new dev value. If a race occurs, a use-after-free (UAF)
> issue could occur in fsl_mc_bus_match().

So shouldn't that be fixed there in the fsl bus code?

> To fix this issue, we examined the interface that calls this function and
> found that fsl_mc_bus_match() is called by the interface drv->bus->match,
> which in turn is called by the function driver_match_device(). We found
> three places where driver_match_device() is called, and the call in
> __driver_attach() is locked. However, since the driver_override_store()
> function is implemented in drivers such as amba, base, cdx, and others,
> this data race issue is quite common. Adding a lock at the lower level
> could conflict with the lock in __driver_attach(), leading to a potential
> deadlock. Therefore, we decided to add locking to the other two calls to
> driver_match_device() to ensure that when the function is called through
> the match interface, dev is protected by a lock throughout the process, and
> its value remains unchanged.

The device is protected from going away, but that doesn't affect the
override string from going away, right?


> 
> Fixes: 49b420a13ff9 ("driver core: check bus->match without holding device lock")
> Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com>
> ---
> In 2008, the kernel moved all match checks outside of the lock, and at that
> time, there was no override, so this approach worked. In 2015, one function
> moved the match check into the lock on one of its paths in order to support
> asynchronous device binding, which led to inconsistent behavior. Later,
> some kernel drivers added the driver_override feature to support binding
> specific drivers, and the driver_set_override function was widely used to
> control it. This function uses device_lock to control concurrency. This
> issue is caused by both lower-level drivers and upper-level drivers, so
> I'm not sure if the fixes tag is correct, and I haven't added a cc. I hope
> to discuss this issue with the developers.
> ---
>  drivers/base/bus.c | 16 +++++++++++-----
>  drivers/base/dd.c  |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 657c93c38b0d..78d8c58fc50c 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -261,13 +261,19 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
>  	const struct bus_type *bus = bus_get(drv->bus);
>  	struct device *dev;
>  	int err = -ENODEV;
> +	int ret;
>  
>  	dev = bus_find_device_by_name(bus, NULL, buf);
> -	if (dev && driver_match_device(drv, dev)) {
> -		err = device_driver_attach(drv, dev);
> -		if (!err) {
> -			/* success */
> -			err = count;
> +	if (dev) {
> +		device_lock(dev);
> +		ret = driver_match_device(drv, dev);
> +		device_unlock(dev);

Have you tested to make sure this works properly with lockdep enabled?
The driver/device locking is tricky, and adding a lock here feels wrong
as this has to do with manual binding, not the override string.
Shouldn't the string checking perhaps be made more safe by making it a
driver core function instead?

thanks,

greg k-h

Re: [PATCH] driver core: Fix concurrency issue in match driver interface.

[Qiu-ji Chen]

Hi, Greg

The driver_override is updated by driver_set_override(), a function
provided by the driver core. In driver_set_override(), the
driver_override is updated while holding the device_lock. Therefore,
locking is required when calling match to prevent the driver_override
from being modified within driver_set_override().

It’s fine to add locks in these two functions. The second half of
__driver_attach already performs lock and unlock operations, and the
device_driver_attach function in bind_store also has lock and unlock
operations. So, when calling the bind_store function and
__driver_attach, these are environments without locks, where adding
locks is appropriate.

In the current core code, among the three calls to
driver_match_device, two are not locked and one is locked. Therefore,
adding locks in the lower-level driver would conflict with the already
locked path in the upper-level function, causing a potential deadlock.
Thus, locking cannot be added in the lower-level driver. In the call
chain from __device_attach to __device_attach_driver to
driver_match_device, the call to __device_attach_driver is already
protected by a lock, so the call to driver_match_device in this chain
is locked. If we add locks in the lower-level driver, it could lead to
a deadlock due to the upper-level lock.

Changing the string checking process to a driver core function doesn't
help, because this checking function needs to hold the device_lock to
prevent updates by the driver_set_override() function, and it would
still be called in the implementation of the match interface.
Essentially, it would still involve adding a lock at the lower level.
As mentioned earlier, there is already a locked path for the match
interface at the upper level, which could lead to a deadlock. From our
perspective, it seems impossible to modify this locked path at the
upper level to be lock-free, so we did not choose the solution of
adding a lock at the lower-level driver.

Therefore, we recommend adding locks to all three calls to
driver_match_device in the upper-level code to ensure consistency and
prevent modifications to the driver_override field during the
driver_set_override function call, fixing the data race issue. Based
on your feedback, we have updated the description in v2. Thank you for
your discussion.

Regards,
Qiu-ji Chen

Re: [PATCH] driver core: Fix concurrency issue in match driver interface.

[Greg KH]

On Wed, Nov 13, 2024 at 02:21:57AM +0800, Qiu-ji Chen wrote:
> Hi, Greg
> 
> The driver_override is updated by driver_set_override(), a function
> provided by the driver core. In driver_set_override(), the
> driver_override is updated while holding the device_lock. Therefore,
> locking is required when calling match to prevent the driver_override
> from being modified within driver_set_override().

I'm sorry, but I lost all context.  What are you referring to here?

Remember, some of use get 1000+ emails a day to do something with,
always respond with proper quoting and response in order to have a
conversation that everyone can understand.

thanks,

greg k-h