Re: [PATCH 6.6.y 2/2] fbdev: hyperv_fb: Allow graceful removal of framebuffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] workqueue: UAF of delayed work due to re-arming during teardown
- [High] vmbus: NULL pointer dereference on closed channel during fbdev ioctl
- [High] fbdev: UAF of parent device pointer in hvfb_destroy()
--

commit 0b97ae630048a0b4a041222965b966a2373daa70
Author: Saurabh Sengar <ssengar@linux.microsoft.com>

fbdev: hyperv_fb: Allow graceful removal of framebuffer

This commit defers the release of the framebuffer memory to the fb_ops
fb_destroy callback to prevent warnings when unbinding an in-use
framebuffer. It also moves the unregister_framebuffer call to the end of
hvfb_remove() and places it after vmbus_close().

Link: https://lore.kernel.org/r/1740845791-19977-3-git-send-email-ssengar@linux.microsoft.com

> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 80e6d96821795..e0d68953aa1ac 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
[ ... ]
> @@ -887,6 +889,17 @@ static void hvfb_cfb_imageblit(struct fb_info *p,
>  					       image->width, image->height);
>  }
>  
> +/*
> + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end
> + * of unregister_framebuffer() or fb_release(). Do any cleanup related to
> + * framebuffer here.
> + */
> +static void hvfb_destroy(struct fb_info *info)
> +{
> +	hvfb_putmem(info);

[Severity: High]
Is it safe to defer hvfb_putmem() to the fb_destroy callback?

hvfb_putmem() calls hvfb_release_phymem(info->device, ...), but info->device
points to the hdev->device. The VMBus core unregisters and frees the hv_device
(hdev) when hvfb_remove() completes.

If userspace keeps the framebuffer file descriptor open, hvfb_destroy() will
execute later. Does this mean info->device will be a dangling pointer when
passed to dma_free_coherent() in hvfb_release_phymem(), causing a
Use-After-Free?

> +	framebuffer_release(info);
> +}
> +
>  static const struct fb_ops hvfb_ops = {
>  	.owner = THIS_MODULE,
>  	.fb_check_var = hvfb_check_var,
[ ... ]
> @@ -1246,14 +1260,11 @@ static void hvfb_remove(struct hv_device *hdev)
>  
>  	fb_deferred_io_cleanup(info);
>  
> -	unregister_framebuffer(info);
>  	cancel_delayed_work_sync(&par->dwork);

[Severity: High]
Since unregister_framebuffer() is called after this point, is it possible for
concurrent fbcon console updates or user operations to re-arm the work item?

For instance, could a call to hvfb_cfb_fillrect() ->
hvfb_ondemand_refresh_throttle() unconditionally re-arm the delayed work while
the device is shutting down, leading to a Use-After-Free of the par structure
when the delayed work eventually executes?

>  
>  	vmbus_close(hdev->channel);

[Severity: High]
Does moving vmbus_close() before unregister_framebuffer() expose a race
condition where the VMBus channel and its ring buffers are freed while the
framebuffer interface is still fully active?

A concurrent user ioctl(FBIOPUT_VSCREENINFO) or active fbcon operation could
invoke synthvid_send() -> vmbus_sendpacket(). If vmbus_sendpacket() tries to
access the closed channel and its NULL ring_buffer structure, would this result
in a NULL pointer dereference?

>  	hv_set_drvdata(hdev, NULL);
> -
> -	hvfb_putmem(info);
> -	framebuffer_release(info);
> +	unregister_framebuffer(info);
>  }
>  
>  static int hvfb_suspend(struct hv_device *hdev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008191309.98263-1-artem@trailofbits.com?part=2