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