Re: [PATCH 4/5] drm/vblank: Use drm_for_each_crtc() in drm_vblank_init()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Calling `drm_vblank_init` before creating CRTCs results in a zero-size allocation, leading to a kernel crash when vblank features are later used.
--

--- Patch [4]: [PATCH 4/5] drm/vblank: Use drm_for_each_crtc() in drm_vblank_init() ---
commit 4aec980231f556c8a36b5294d67b3d582d49d623
Author: Lyude Paul <[email protected]>

drm/vblank: Use drm_for_each_crtc() in drm_vblank_init()

This patch updates drm_vblank_init() to use the drm_for_each_crtc() iterator
instead of manually counting the number of CRTCs.

> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 17283e8920378..d57405050554f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -547,19 +547,20 @@ static void drm_vblank_init_release(struct drm_device *dev, void *ptr)
>  int drm_vblank_init(struct drm_device *dev)
>  {
>  	int ret;
> -	unsigned int num_crtcs = dev->mode_config.num_crtc;
> -	unsigned int i;
> +	struct drm_crtc *crtc;
>  
>  	spin_lock_init(&dev->vbl_lock);
>  	spin_lock_init(&dev->vblank_time_lock);
>  
> -	dev->vblank = drmm_kcalloc(dev, num_crtcs, sizeof(*dev->vblank), GFP_KERNEL);
> +	dev->vblank = drmm_kcalloc(dev, dev->mode_config.num_crtc,
> +				   sizeof(*dev->vblank), GFP_KERNEL);
>  	if (!dev->vblank)
>  		return -ENOMEM;
>  
>  	dev->has_hw_vblank = true;
>  
> -	for (i = 0; i < num_crtcs; i++) {
> +	drm_for_each_crtc(crtc, dev) {

[Severity: Critical]
Could this change lead to an uninitialized vblank array and subsequent page
fault for drivers that initialize vblank before creating CRTCs?

For example, in vkms_drv.c:vkms_create(), drm_vblank_init() is called before
vkms_modeset_init() creates the CRTCs:

	ret = drm_vblank_init(&vkms_device->drm);
	if (ret) {
		DRM_ERROR("Failed to vblank\n");
		goto out_devres;
	}

	ret = vkms_modeset_init(vkms_device);

Because the CRTCs are not yet created at this point, dev->mode_config.num_crtc
is 0. The allocation for the vblank array becomes a 0-byte allocation,
returning ZERO_SIZE_PTR (0x10). The drm_for_each_crtc() loop then executes 0
times, and drm_vblank_init() successfully returns 0.

When userspace later calls DRM_IOCTL_WAIT_VBLANK, the kernel looks up the
CRTC using drm_vblank_crtc(dev, pipe). This returns a pointer into the array,
which evaluates to ZERO_SIZE_PTR + pipe * sizeof(...).

Will dereferencing this pointer cause an immediate kernel OOPS?

> +		unsigned int i = crtc->index;
>  		struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, i);
>  
>  		vblank->dev = dev;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4