[PATCH v2] cdx: Fix possible UAF error in driver_override_show()

[PATCH v2] cdx: Fix possible UAF error in driver_override_show()

[Qiu-ji Chen]

There is a data race between the functions driver_override_show() and
driver_override_store(). In the driver_override_store() function, the
assignment to ret calls driver_set_override(), which frees the old value
while writing the new value to dev. If a race occurs, it may cause a
use-after-free (UAF) error in driver_override_show().

To fix this issue, we adopt a logic similar to the driver_override_show()
function in vmbus_drv.c, protecting dev within a lock to ensure its value
remains unchanged.

This possible bug is found by an experimental static analysis tool
developed by our team. This tool analyzes the locking APIs to extract
function pairs that can be concurrently executed, and then analyzes the
instructions in the paired functions to identify possible concurrency bugs
including data races and atomicity violations.

Fixes: 48a6c7bced2a ("cdx: add device attributes")
Cc: stable@vger.kernel.org
Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com>
---
V2:
Modified the title and description.
Removed the changes to cdx_bus_match().
---
 drivers/cdx/cdx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index 07371cb653d3..4af1901c9d52 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -470,8 +470,12 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct cdx_device *cdx_dev = to_cdx_device(dev);
+	ssize_t len;
 
-	return sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
+	device_lock(dev);
+	len = sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
+	device_unlock(dev);
+	return len;
 }
 static DEVICE_ATTR_RW(driver_override);
 
-- 
2.34.1

Re: [PATCH v2] cdx: Fix possible UAF error in driver_override_show()

[Greg KH]

On Wed, Nov 13, 2024 at 12:23:38AM +0800, Qiu-ji Chen wrote:
> There is a data race between the functions driver_override_show() and
> driver_override_store(). In the driver_override_store() function, the
> assignment to ret calls driver_set_override(), which frees the old value
> while writing the new value to dev. If a race occurs, it may cause a
> use-after-free (UAF) error in driver_override_show().
> 
> To fix this issue, we adopt a logic similar to the driver_override_show()
> function in vmbus_drv.c, protecting dev within a lock to ensure its value
> remains unchanged.
> 
> This possible bug is found by an experimental static analysis tool
> developed by our team. This tool analyzes the locking APIs to extract
> function pairs that can be concurrently executed, and then analyzes the
> instructions in the paired functions to identify possible concurrency bugs
> including data races and atomicity violations.
> 
> Fixes: 48a6c7bced2a ("cdx: add device attributes")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com>
> ---
> V2:
> Modified the title and description.
> Removed the changes to cdx_bus_match().
> ---
>  drivers/cdx/cdx.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> index 07371cb653d3..4af1901c9d52 100644
> --- a/drivers/cdx/cdx.c
> +++ b/drivers/cdx/cdx.c
> @@ -470,8 +470,12 @@ static ssize_t driver_override_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
>  {
>  	struct cdx_device *cdx_dev = to_cdx_device(dev);
> +	ssize_t len;
>  
> -	return sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
> +	device_lock(dev);
> +	len = sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
> +	device_unlock(dev);

No, you should not need to lock a device in a sysfs callback like this,
especially for just printing out a string.

greg k-h

Re: [PATCH v2] cdx: Fix possible UAF error in driver_override_show()

[Qiu-ji Chen]

> > ---
> >  drivers/cdx/cdx.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > index 07371cb653d3..4af1901c9d52 100644
> > --- a/drivers/cdx/cdx.c
> > +++ b/drivers/cdx/cdx.c
> > @@ -470,8 +470,12 @@ static ssize_t driver_override_show(struct device *dev,
> >                                   struct device_attribute *attr, char *buf)
> >  {
> >       struct cdx_device *cdx_dev = to_cdx_device(dev);
> > +     ssize_t len;
> >
> > -     return sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
> > +     device_lock(dev);
> > +     len = sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
> > +     device_unlock(dev);
>
> No, you should not need to lock a device in a sysfs callback like this,
> especially for just printing out a string.

This function is part of DEVICE_ATTR_RW, which includes both
driver_override_show() and driver_override_store(). These functions
can be executed concurrently in sysfs.

The driver_override_store() function uses driver_set_override() to
update the driver_override value, and driver_set_override() internally
locks the device (device_lock(dev)). If driver_override_show() reads
cdx_dev->driver_override without locking, it could potentially access
a freed pointer if driver_override_store() frees the string
concurrently. This could lead to printing a kernel address, which is a
security risk since DEVICE_ATTR can be read by all users.

Additionally, a similar pattern is used in drivers/amba/bus.c, as well
as many other bus drivers, where device_lock() is taken in the show
function, and it has been working without issues.

Regards,
Qiu-ji Chen

Re: [PATCH v2] cdx: Fix possible UAF error in driver_override_show()

[Greg KH]

On Wed, Jan 15, 2025 at 12:04:16PM +0800, Qiu-ji Chen wrote:
> > > ---
> > >  drivers/cdx/cdx.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
> > > index 07371cb653d3..4af1901c9d52 100644
> > > --- a/drivers/cdx/cdx.c
> > > +++ b/drivers/cdx/cdx.c
> > > @@ -470,8 +470,12 @@ static ssize_t driver_override_show(struct device *dev,
> > >                                   struct device_attribute *attr, char *buf)
> > >  {
> > >       struct cdx_device *cdx_dev = to_cdx_device(dev);
> > > +     ssize_t len;
> > >
> > > -     return sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
> > > +     device_lock(dev);
> > > +     len = sysfs_emit(buf, "%s\n", cdx_dev->driver_override);
> > > +     device_unlock(dev);
> >
> > No, you should not need to lock a device in a sysfs callback like this,
> > especially for just printing out a string.
> 
> This function is part of DEVICE_ATTR_RW, which includes both
> driver_override_show() and driver_override_store(). These functions
> can be executed concurrently in sysfs.
> 
> The driver_override_store() function uses driver_set_override() to
> update the driver_override value, and driver_set_override() internally
> locks the device (device_lock(dev)). If driver_override_show() reads
> cdx_dev->driver_override without locking, it could potentially access
> a freed pointer if driver_override_store() frees the string
> concurrently. This could lead to printing a kernel address, which is a
> security risk since DEVICE_ATTR can be read by all users.

Ah, I missed the mess in driver_override_store(), so yes, that does make
more sense now.  Please document this in the changelog so we understand
it when you resubmit it as normally, we do not care about racing in
sysfs attributes because it does not matter for simple values.

thanks,

greg k-h