linux-kernel - Re: [PATCH v2] cdx: Fix possible UAF error in driver_override

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: <CANgpojV8P7+LmDPNiJWezER6PK83Wj_QyX8iOKzfxO98WkSnDg@mail.gmail.com>
Date: Wed, 15 Jan 2025 12:04:16 +0800
From: Qiu-ji Chen <chenqiuji666@...il.com>
To: Greg KH <greg@...ah.com>
Cc: nipun.gupta@....com, nikhil.agarwal@....com, linux-kernel@...r.kernel.org, 
	baijiaju1990@...il.com, stable@...r.kernel.org
Subject: Re: [PATCH v2] cdx: Fix possible UAF error in driver_override_show()

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