[PATCH] fbdev: smscufx: bail out of ufx_set_vid_mode() when the device is gone

[PATCH] fbdev: smscufx: bail out of ufx_set_vid_mode() when the device is gone

[YuanZhe Liu]

ufx_usb_disconnect() marks the device as virtualized and clears
usb_active, but the fbdev entry points that program the controller are
not serialized against it.  ufx_handle_damage(), ufx_dpy_deferred_io()
and ufx_ops_ioctl() all return early while usb_active is clear;
ufx_set_vid_mode() does not, so a mode set or a blank request that
races with the disconnect keeps issuing control transfers through
dev->udev.

The usb_device is released once the disconnect callback returns, while
dev->udev is left pointing at it, so those transfers dereference a
stale pointer and fault in __create_pipe():

  general protection fault, probably for non-canonical address
  0xdffffc0000000006: 0000 [#1] SMP
  KASAN: null-ptr-deref in range [0x0000000000000030-0x0000000000000037]
  RIP: 0010:__create_pipe include/linux/usb.h:2031
  Call Trace:
   ufx_set_vid_mode
   ufx_ops_set_par
   fb_set_var
   fb_set_var_from_user
   store_rotate

Return -ENODEV from ufx_set_vid_mode() when the device is no longer
active, matching the check the other transmit paths already use.  Both
callers, ufx_ops_set_par() and ufx_ops_blank(), handle the error: the
former skips the greenscreen fill and keeps the previous mode, the
latter already ignores the result.

The guard cannot live in ufx_reg_write()/ufx_reg_read() instead, since
probe programs the controller through ufx_config_ddr2(),
ufx_i2c_init(), ufx_setup_modes() and ufx_reg_set_bits() before it sets
usb_active.

Fixes: 3c8a63e22a08 ("Add support for SMSC UFX6000/7000 USB display adapters")
Cc: stable@vger.kernel.org
Co-developed-by: Bin Yu <byu@xidian.edu.cn>
Signed-off-by: Bin Yu <byu@xidian.edu.cn>
Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>
---
 drivers/video/fbdev/smscufx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 5704f60e1741..f1b640a5d712 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -661,8 +661,12 @@ static int ufx_set_vid_mode(struct ufx_data *dev, struct fb_var_screeninfo *var)
 	u32 temp;
 	u16 h_total, h_active, h_blank_start, h_blank_end, h_sync_start, h_sync_end;
 	u16 v_total, v_active, v_blank_start, v_blank_end, v_sync_start, v_sync_end;
+	int status;
+
+	if (!atomic_read(&dev->usb_active))
+		return -ENODEV;
 
-	int status = ufx_reg_write(dev, 0x8028, 0);
+	status = ufx_reg_write(dev, 0x8028, 0);
 	check_warn_return(status, "ufx_set_vid_mode error disabling RGB pad");
 
 	status = ufx_reg_write(dev, 0x8024, 0);
-- 
2.34.1

Re: [PATCH] fbdev: smscufx: bail out of ufx_set_vid_mode() when the device is gone

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] fbdev: smscufx: unsynchronized usb_active check in ufx_set_vid_mode()
--

commit 3a768d75a894178fec634e82fca9f1ca2c1f5f21
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

fbdev: smscufx: bail out of ufx_set_vid_mode() when the device is gone

This commit adds an atomic_read(&dev->usb_active) check at the start of
ufx_set_vid_mode() so that mode sets and blank requests return -ENODEV
after ufx_usb_disconnect() has cleared usb_active, avoiding control
transfers through a stale dev->udev pointer.

> diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
> index 34d8abd5b3544..a199e6d03f06b 100644
> --- a/drivers/video/fbdev/smscufx.c
> +++ b/drivers/video/fbdev/smscufx.c
> @@ -661,8 +661,12 @@ static int ufx_set_vid_mode(struct ufx_data *dev, struct fb_var_screeninfo *var)
>  	u32 temp;
>  	u16 h_total, h_active, h_blank_start, h_blank_end, h_sync_start, h_sync_end;
>  	u16 v_total, v_active, v_blank_start, v_blank_end, v_sync_start, v_sync_end;
> +	int status;
> +
> +	if (!atomic_read(&dev->usb_active))
> +		return -ENODEV;
>  
> -	int status = ufx_reg_write(dev, 0x8028, 0);
> +	status = ufx_reg_write(dev, 0x8028, 0);

[Severity: Critical]
Can ufx_set_vid_mode() still race with ufx_usb_disconnect() and dereference
a freed dev->udev after passing this initial atomic_read(&dev->usb_active)
check?

Neither ufx_set_vid_mode() nor its callers (ufx_ops_set_par() and
ufx_ops_blank()) acquire disconnect_mutex, and ufx_usb_disconnect() does
not acquire info->lock. After passing the check at entry, ufx_set_vid_mode()
performs around 30 synchronous USB control transfers via ufx_reg_write()
and ufx_reg_read() across ufx_blank(), ufx_disable(), ufx_config_pix_clk(),
ufx_enable(), and ufx_unblank(), including two msleep(1) delays inside
ufx_config_pix_clk():

drivers/video/fbdev/smscufx.c:ufx_config_pix_clk() {
	...
	status = ufx_reg_clear_bits(dev, 0x7000, 0x00000005);
	check_warn_return(status,
		"error clearing PLL0 bypass bits in 0x7000");
	msleep(1);

	status = ufx_reg_clear_bits(dev, 0x7000, 0x0000000A);
	check_warn_return(status,
		"error clearing PLL1 bypass bits in 0x7000");
	msleep(1);
	...
}

When a userspace task invokes ufx_ops_set_par() or ufx_ops_blank() via
do_fb_ioctl() (FBIOPUT_VSCREENINFO or FBIOBLANK) on an open /dev/fbX file
descriptor, ufx_usb_disconnect() can run concurrently after
ufx_set_vid_mode() has already passed the usb_active check:

drivers/video/fbdev/smscufx.c:ufx_usb_disconnect() {
	...
	atomic_set(&dev->usb_active, 0);

	usb_set_intfdata(interface, NULL);

	/* if clients still have us open, will be freed on last close */
	if (dev->fb_count == 0)
		ufx_free_framebuffer(dev);

	/* this function will wait for all in-flight urbs to complete */
	if (dev->urbs.count > 0)
		ufx_free_urb_list(dev);

	pr_debug("freeing ufx_data %p", dev);

	unregister_framebuffer(info);
	...
}

Here ufx_free_urb_list() only waits on dev->urbs.limit_sem for the
pre-allocated bulk URBs in dev->urbs, not for synchronous control URBs
allocated inside usb_control_msg() or for an in-flight ufx_set_vid_mode()
call. Moreover, when dev->fb_count > 0 and CONFIG_FRAMEBUFFER_CONSOLE is
disabled, unregister_framebuffer() does not take info->lock or
console_lock() and returns without waiting for the in-progress ioctl.

Because ufx_usb_probe() stores dev->udev = usbdev without taking a
reference via usb_get_dev(), ufx_usb_disconnect() can finish and allow the
USB core to release struct usb_device while ufx_set_vid_mode() is sleeping
in msleep(1) or between control transfers, causing subsequent
ufx_reg_write() and ufx_reg_read() calls to dereference a freed dev->udev.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102072120.3022-1-25031212351@stu.xidian.edu.cn?part=1

[PATCH v2] fbdev: smscufx: keep the usb_device alive while ufx_data is in use

[YuanZhe Liu]

ufx_usb_probe() stores the usb_device pointer in dev->udev without
taking a reference, and ufx_usb_disconnect() does not wait for in-flight
control transfers.  A mode set or blank request that runs while the
device is being unplugged - through ufx_ops_set_par() or ufx_ops_blank()
on an open /dev/fbN file - keeps issuing synchronous control messages for
the whole ufx_set_vid_mode() sequence, including two msleep(1) delays in
ufx_config_pix_clk().  Once the disconnect callback returns, the USB core
may release the usb_device, and the next ufx_reg_write()/ufx_reg_read()
dereferences dev->udev through __create_pipe():

  general protection fault, probably for non-canonical address
  0xdffffc0000000006: 0000 [#1] SMP
  KASAN: null-ptr-deref in range [0x0000000000000030-0x0000000000000037]
  RIP: __create_pipe include/linux/usb.h:2031
  Call Trace:
   ufx_set_vid_mode
   ufx_ops_set_par
   fb_set_var
   fb_set_var_from_user
   store_rotate

Take a reference to the usb_device in ufx_usb_probe() and drop it in
ufx_free(), so dev->udev and dev->gdev stay valid until the last
ufx_data user is gone.  The control transfers then fail with -ENODEV
instead of dereferencing freed memory.

Also return early from ufx_set_vid_mode() while the device is inactive,
matching the other transmit paths.  That check alone is not enough,
because it is not serialized against ufx_usb_disconnect() and the
transfer sequence continues after it; it only avoids a long series of
failing transfers.

Fixes: 3c8a63e22a08 ("Add support for SMSC UFX6000/7000 USB display adapters")
Cc: stable@vger.kernel.org
---
Changes in v2:
 - Hold a reference to the usb_device and drop it in ufx_free() so
   dev->udev stays valid for the whole transfer sequence; the
   usb_active check alone only covers the entry (review feedback).
 drivers/video/fbdev/smscufx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 5704f60e1741..eac303adb69a 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -661,8 +661,12 @@ static int ufx_set_vid_mode(struct ufx_data *dev, struct fb_var_screeninfo *var)
 	u32 temp;
 	u16 h_total, h_active, h_blank_start, h_blank_end, h_sync_start, h_sync_end;
 	u16 v_total, v_active, v_blank_start, v_blank_end, v_sync_start, v_sync_end;
+	int status;
+
+	if (!atomic_read(&dev->usb_active))
+		return -ENODEV;
 
-	int status = ufx_reg_write(dev, 0x8028, 0);
+	status = ufx_reg_write(dev, 0x8028, 0);
 	check_warn_return(status, "ufx_set_vid_mode error disabling RGB pad");
 
 	status = ufx_reg_write(dev, 0x8024, 0);
@@ -1064,6 +1068,7 @@ static void ufx_free(struct kref *kref)
 {
 	struct ufx_data *dev = container_of(kref, struct ufx_data, kref);
 
+	usb_put_dev(dev->udev);
 	kfree(dev);
 }
 
@@ -1606,6 +1611,8 @@ static int ufx_usb_probe(struct usb_interface *interface,
 
 	dev->udev = usbdev;
 	dev->gdev = &usbdev->dev; /* our generic struct device * */
+	/* keep the device alive until the last ufx_data user is gone */
+	usb_get_dev(usbdev);
 	usb_set_intfdata(interface, dev);
 
 	dev_dbg(dev->gdev, "%s %s - serial #%s\n",
-- 
2.34.1