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 <[email protected]> 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/[email protected]?part=1
