| Message ID | 20220925123253.GA381917@ubuntu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] video: fbdev: smscufx: Fix use-after-free in ufx_ops_open() | expand |
On 9/25/22 14:32, Hyunwoo Kim wrote: > A race condition may occur if the user physically removes the > USB device while calling open() for this device node. > > This is a race condition between the ufx_ops_open() function and > the ufx_usb_disconnect() function, which may eventually result in UAF. > > So, add a mutex to the ufx_ops_open() and ufx_usb_disconnect() functions > to avoid race contidion of krefs. > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> > --- > drivers/video/fbdev/smscufx.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c > index d7aa5511c361..02a22e31e9c0 100644 > --- a/drivers/video/fbdev/smscufx.c > +++ b/drivers/video/fbdev/smscufx.c > @@ -137,6 +137,8 @@ static int ufx_submit_urb(struct ufx_data *dev, struct urb * urb, size_t len); > static int ufx_alloc_urb_list(struct ufx_data *dev, int count, size_t size); > static void ufx_free_urb_list(struct ufx_data *dev); > > +static DEFINE_MUTEX(disconnect_mutex); > + > /* reads a control register */ > static int ufx_reg_read(struct ufx_data *dev, u32 index, u32 *data) > { > @@ -1065,15 +1067,21 @@ static int ufx_ops_open(struct fb_info *info, int user) > { > struct ufx_data *dev = info->par; > > + mutex_lock(&disconnect_mutex); > + > /* fbcon aggressively connects to first framebuffer it finds, > * preventing other clients (X) from working properly. Usually > * not what the user wants. Fail by default with option to enable. */ > - if (user == 0 && !console) > + if (user == 0 && !console) { > + mutex_unlock(&disconnect_mutex); > return -EBUSY; it seems user and console don't need to be protected by the lock. Does it make sense to move them out of the lock? Helge
On Sun, Sep 25, 2022 at 03:09:47PM +0200, Helge Deller wrote: > it seems user and console don't need to be protected by the lock. > Does it make sense to move them out of the lock? you're right. Since both variables are not related to a race condition, it is better to get them out of the lock. I will submit a v3 patch. Best Regards, Hyunwoo Kim.
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c index d7aa5511c361..02a22e31e9c0 100644 --- a/drivers/video/fbdev/smscufx.c +++ b/drivers/video/fbdev/smscufx.c @@ -137,6 +137,8 @@ static int ufx_submit_urb(struct ufx_data *dev, struct urb * urb, size_t len); static int ufx_alloc_urb_list(struct ufx_data *dev, int count, size_t size); static void ufx_free_urb_list(struct ufx_data *dev); +static DEFINE_MUTEX(disconnect_mutex); + /* reads a control register */ static int ufx_reg_read(struct ufx_data *dev, u32 index, u32 *data) { @@ -1065,15 +1067,21 @@ static int ufx_ops_open(struct fb_info *info, int user) { struct ufx_data *dev = info->par; + mutex_lock(&disconnect_mutex); + /* fbcon aggressively connects to first framebuffer it finds, * preventing other clients (X) from working properly. Usually * not what the user wants. Fail by default with option to enable. */ - if (user == 0 && !console) + if (user == 0 && !console) { + mutex_unlock(&disconnect_mutex); return -EBUSY; + } /* If the USB device is gone, we don't accept new opens */ - if (dev->virtualized) + if (dev->virtualized) { + mutex_unlock(&disconnect_mutex); return -ENODEV; + } dev->fb_count++; @@ -1097,6 +1105,8 @@ static int ufx_ops_open(struct fb_info *info, int user) pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d", info->node, user, info, dev->fb_count); + mutex_unlock(&disconnect_mutex); + return 0; } @@ -1741,6 +1751,8 @@ static void ufx_usb_disconnect(struct usb_interface *interface) { struct ufx_data *dev; + mutex_lock(&disconnect_mutex); + dev = usb_get_intfdata(interface); pr_debug("USB disconnect starting\n"); @@ -1761,6 +1773,8 @@ static void ufx_usb_disconnect(struct usb_interface *interface) kref_put(&dev->kref, ufx_free); /* consider ufx_data freed */ + + mutex_unlock(&disconnect_mutex); } static struct usb_driver ufx_driver = {
A race condition may occur if the user physically removes the USB device while calling open() for this device node. This is a race condition between the ufx_ops_open() function and the ufx_usb_disconnect() function, which may eventually result in UAF. So, add a mutex to the ufx_ops_open() and ufx_usb_disconnect() functions to avoid race contidion of krefs. Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com> --- drivers/video/fbdev/smscufx.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)