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] [day] [month] [year] [list]

Message-ID: <2025011523-stage-snowbound-291a@gregkh>
Date: Wed, 15 Jan 2025 08:57:19 +0100
From: Greg KH <greg@...ah.com>
To: Qiu-ji Chen <chenqiuji666@...il.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()

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