[PATCH v2] video: fbdev: smscufx: Fixed several use-after-free bugs

[Helge Deller]

On 10/21/22 03:15, Hyunwoo Kim wrote:
> Several types of UAFs can occur when physically removing a USB device.
>
> Adds ufx_ops_destroy() function to .fb_destroy of fb_ops, and
> in this function, there is kref_put() that finally calls ufx_free().
>
> This fix prevents multiple UAFs.
>
> Signed-off-by: Hyunwoo Kim <imv4bel at gmail.com>
> Link: https://lore.kernel.org/linux-fbdev/20221011153436.GA4446@ubuntu/

applied.
Thanks!
Helge

> ---
> v2:
> The v1 patch fixed several UAFs, but "info" was not kfree()d after the device
> was removed by calling the framebuffer_release() function from
> ufx_free_framebuffer().
> This is because fb_info->count was not 0 at the time the
> framebuffer_release() function was called.
>
> Moved the framebuffer_release() function to the ufx_ops_destory() function.
> The ufx_ops_destory() function is a .fb_destory ops that is called
> after fb_info->count goes to zero.
> ---
>   drivers/video/fbdev/smscufx.c | 55 +++++++++++++++++++----------------
>   1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
> index e65bdc499c23..9343b7a4ac89 100644
> --- a/drivers/video/fbdev/smscufx.c
> +++ b/drivers/video/fbdev/smscufx.c
> @@ -97,7 +97,6 @@ struct ufx_data {
>   	struct kref kref;
>   	int fb_count;
>   	bool virtualized; /* true when physical usb device not present */
> -	struct delayed_work free_framebuffer_work;
>   	atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */
>   	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
>   	u8 *edid; /* null until we read edid from hw or get from sysfs */
> @@ -1117,15 +1116,24 @@ static void ufx_free(struct kref *kref)
>   {
>   	struct ufx_data *dev = container_of(kref, struct ufx_data, kref);
>
> -	/* this function will wait for all in-flight urbs to complete */
> -	if (dev->urbs.count > 0)
> -		ufx_free_urb_list(dev);
> +	kfree(dev);
> +}
>
> -	pr_debug("freeing ufx_data %p", dev);
> +static void ufx_ops_destory(struct fb_info *info)
> +{
> +	struct ufx_data *dev = info->par;
> +	int node = info->node;
>
> -	kfree(dev);
> +	/* Assume info structure is freed after this point */
> +	framebuffer_release(info);
> +
> +	pr_debug("fb_info for /dev/fb%d has been freed", node);
> +
> +	/* release reference taken by kref_init in probe() */
> +	kref_put(&dev->kref, ufx_free);
>   }
>
> +
>   static void ufx_release_urb_work(struct work_struct *work)
>   {
>   	struct urb_node *unode = container_of(work, struct urb_node,
> @@ -1134,14 +1142,9 @@ static void ufx_release_urb_work(struct work_struct *work)
>   	up(&unode->dev->urbs.limit_sem);
>   }
>
> -static void ufx_free_framebuffer_work(struct work_struct *work)
> +static void ufx_free_framebuffer(struct ufx_data *dev)
>   {
> -	struct ufx_data *dev = container_of(work, struct ufx_data,
> -					    free_framebuffer_work.work);
>   	struct fb_info *info = dev->info;
> -	int node = info->node;
> -
> -	unregister_framebuffer(info);
>
>   	if (info->cmap.len != 0)
>   		fb_dealloc_cmap(&info->cmap);
> @@ -1153,11 +1156,6 @@ static void ufx_free_framebuffer_work(struct work_struct *work)
>
>   	dev->info = NULL;
>
> -	/* Assume info structure is freed after this point */
> -	framebuffer_release(info);
> -
> -	pr_debug("fb_info for /dev/fb%d has been freed", node);
> -
>   	/* ref taken in probe() as part of registering framebfufer */
>   	kref_put(&dev->kref, ufx_free);
>   }
> @@ -1169,11 +1167,13 @@ static int ufx_ops_release(struct fb_info *info, int user)
>   {
>   	struct ufx_data *dev = info->par;
>
> +	mutex_lock(&disconnect_mutex);
> +
>   	dev->fb_count--;
>
>   	/* We can't free fb_info here - fbmem will touch it when we return */
>   	if (dev->virtualized && (dev->fb_count == 0))
> -		schedule_delayed_work(&dev->free_framebuffer_work, HZ);
> +		ufx_free_framebuffer(dev);
>
>   	if ((dev->fb_count == 0) && (info->fbdefio)) {
>   		fb_deferred_io_cleanup(info);
> @@ -1186,6 +1186,8 @@ static int ufx_ops_release(struct fb_info *info, int user)
>
>   	kref_put(&dev->kref, ufx_free);
>
> +	mutex_unlock(&disconnect_mutex);
> +
>   	return 0;
>   }
>
> @@ -1292,6 +1294,7 @@ static const struct fb_ops ufx_ops = {
>   	.fb_blank = ufx_ops_blank,
>   	.fb_check_var = ufx_ops_check_var,
>   	.fb_set_par = ufx_ops_set_par,
> +	.fb_destroy = ufx_ops_destory,
>   };
>
>   /* Assumes &info->lock held by caller
> @@ -1673,9 +1676,6 @@ static int ufx_usb_probe(struct usb_interface *interface,
>   		goto destroy_modedb;
>   	}
>
> -	INIT_DELAYED_WORK(&dev->free_framebuffer_work,
> -			  ufx_free_framebuffer_work);
> -
>   	retval = ufx_reg_read(dev, 0x3000, &id_rev);
>   	check_warn_goto_error(retval, "error %d reading 0x3000 register from device", retval);
>   	dev_dbg(dev->gdev, "ID_REV register value 0x%08x", id_rev);
> @@ -1748,10 +1748,12 @@ static int ufx_usb_probe(struct usb_interface *interface,
>   static void ufx_usb_disconnect(struct usb_interface *interface)
>   {
>   	struct ufx_data *dev;
> +	struct fb_info *info;
>
>   	mutex_lock(&disconnect_mutex);
>
>   	dev = usb_get_intfdata(interface);
> +	info = dev->info;
>
>   	pr_debug("USB disconnect starting\n");
>
> @@ -1765,12 +1767,15 @@ static void ufx_usb_disconnect(struct usb_interface *interface)
>
>   	/* if clients still have us open, will be freed on last close */
>   	if (dev->fb_count == 0)
> -		schedule_delayed_work(&dev->free_framebuffer_work, 0);
> +		ufx_free_framebuffer(dev);
>
> -	/* release reference taken by kref_init in probe() */
> -	kref_put(&dev->kref, ufx_free);
> +	/* this function will wait for all in-flight urbs to complete */
> +	if (dev->urbs.count > 0)
> +		ufx_free_urb_list(dev);
>
> -	/* consider ufx_data freed */
> +	pr_debug("freeing ufx_data %p", dev);
> +
> +	unregister_framebuffer(info);
>
>   	mutex_unlock(&disconnect_mutex);
>   }