Re: [PATCH] drm/msm: avoid an unmatched fbdev IOVA pin

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/msm: VMA close and vma_ref underflow in
  msm_fbdev_driver_fbdev_probe()
--

commit a1a08825c16a09f8d3eedf5784d21f5c50d25613
Author: Michael McAllister <me@michaelmcallister.org>

drm/msm: avoid an unmatched fbdev IOVA pin

This commit replaces msm_gem_get_and_pin_iova() with msm_gem_get_iova() in
msm_fbdev_driver_fbdev_probe() and removes the corresponding
msm_gem_unpin_iova() call from the error path.

> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index dd6d6c507d77e..c3bec6719ca04 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -137,12 +137,8 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  		goto err_drm_gem_handle_delete;
>  	}
>  
> -	/*
> -	 * NOTE: if we can be guaranteed to be able to map buffer
> -	 * in panic (ie. lock-safe, etc) we could avoid pinning the
> -	 * buffer now:
> -	 */
> -	ret = msm_gem_get_and_pin_iova(bo, priv->kms->vm, &paddr);
> +	/* Scanout pins the buffer; only reserve its address here. */
> +	ret = msm_gem_get_iova(bo, priv->kms->vm, &paddr);
>  	if (ret) {
>  		drm_err(dev, "failed to get buffer obj iova: %d\n", ret);
>  		goto err_drm_client_buffer_delete;
> @@ -162,7 +158,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  	fbi->screen_buffer = msm_gem_get_vaddr(bo);
>  	if (IS_ERR(fbi->screen_buffer)) {
>  		ret = PTR_ERR(fbi->screen_buffer);
> -		goto err_msm_gem_unpin;
> +		goto err_msm_gem_vma_put;
>  	}
>  	fbi->screen_size = bo->size;
>  	fbi->fix.smem_start = paddr;
> @@ -179,8 +175,7 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  
>  	return 0;
>  
> -err_msm_gem_unpin:
> -	msm_gem_unpin_iova(bo, priv->kms->vm);
> +err_msm_gem_vma_put:
>  	msm_gem_vma_put(bo);
>  err_drm_client_buffer_delete:
>  	drm_client_buffer_delete(buffer);

[Severity: Medium]
Does calling msm_gem_get_iova() without a matching msm_gem_vma_get() cause
the IOVA reservation to be closed immediately and underflow vma_ref on the
error path?

drm_gem_handle_create() takes the only vma_ref on bo via msm_gem_open(),
and neither drm_client_buffer_create() nor msm_gem_get_iova() increments
vma_ref.  On the normal path, drm_gem_handle_delete() at the end of
msm_fbdev_driver_fbdev_probe() calls msm_gem_close() -> msm_gem_vma_put(),
which drops vma_ref to 0 and calls put_iova_spaces(..., true, "vma_put") to
close the VMA and release the IOVA range stored in fbi->fix.smem_start.

On the error path when msm_gem_get_vaddr() fails, err_msm_gem_vma_put calls
msm_gem_vma_put(bo) and then falls through to drm_gem_handle_delete(),
which calls msm_gem_vma_put(bo) a second time and underflows vma_ref to -1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179424371125.96483.2462884435294600442.msm-fbdev-pin-leak@michaelmcallister.org?part=1