Re: [PATCH v3] drm/vmwgfx: Add drm_panic support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/vmwgfx: invalid pointer dereference of ERR_PTR from devm_memremap
- [High] drm/vmwgfx: deadlock in panic handler due to spinlock acquisition on SVGA2
- [Medium] drm/vmwgfx: buffer overflow in drm_panic due to missing vram_size bounds check
--

commit a3fc842fd80e0209060a17de6e1213becdd10164
Author: Ian Forbes <ian.forbes@broadcom.com>

    drm/vmwgfx: Add drm_panic support
    
    Uses VRAM as the scanout buffer then switches the device to legacy mode.
    Restrict support to 64 Bit so we don't waste valuable VA space on the
    mostly unused VRAM mapping.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 0f101aedb49a0..e6e38b7a2d5b8 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -761,6 +761,12 @@ static int vmw_setup_pci_resources(struct vmw_private *dev,
>  		return -EINVAL;
>  	}
>  
> +#if defined(CONFIG_64BIT)
> +	dev->vram_mem = devm_memremap(dev->drm.dev,
> +				      dev->vram_start,
> +				      dev->vram_size,
> +				      MEMREMAP_WB | MEMREMAP_DEC);

[Severity: High]
Does this code handle failures from devm_memremap() correctly? 

If devm_memremap() fails during device initialization, it returns an ERR_PTR 
value, but this value is assigned to dev->vram_mem without validation using 
IS_ERR().

> +#endif
>  	/*
>  	 * This is approximate size of the vram, the exact size will only

[ ... ]

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index a6c3304e1a67d..942176e2b3c69 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2023,3 +2024,36 @@ bool vmw_user_object_is_null(struct vmw_user_object *uo)
>  {
>  	return !uo->buffer && !uo->surface;
>  }
> +
> +int
> +vmw_get_scanout_buffer(struct drm_plane *plane, struct drm_scanout_buffer *sb)
> +{
> +	void  *vram;
> +	struct vmw_private *vmw_priv = container_of(plane->dev, struct vmw_private, drm);
> +
> +	// Only call on the primary display
> +	if (container_of(plane, struct vmw_display_unit, primary)->unit != 0)
> +		return -EINVAL;
> +
> +	vram = vmw_priv->vram_mem;
> +	if (!vram)
> +		return -ENOMEM;

[Severity: High]
Can this code incorrectly forward an error pointer to iosys_map_set_vaddr()?

Because the previous devm_memremap() assignment was not checked for IS_ERR(),
this !vram check will not catch an ERR_PTR value. 

The invalid pointer will then be passed along, causing a page fault and 
recursive panic when the panic screen tries to render.

> +
> +	vmw_write(vmw_priv, SVGA_REG_CONFIG_DONE, false);

[Severity: High]
Is it safe to invoke vmw_write() here during a panic?

The panic handler runs in atomic context with interrupts disabled.
On SVGA2 hardware, vmw_write() acquires a spinlock:

vmw_write()
  spin_lock(&dev_priv->hw_lock);
  outl(offset, dev_priv->io_start + SVGA_INDEX_PORT);
  ...

If another CPU holds the lock, or if the panicking CPU was already holding it,
this will deadlock and permanently freeze the system instead of displaying the
panic screen.

> +
> +	iosys_map_set_vaddr(&sb->map[0], vram);
> +	sb->format = drm_format_info(DRM_FORMAT_RGB565);
> +	sb->width  = vmw_priv->initial_width;
> +	sb->height = vmw_priv->initial_height;
> +	sb->pitch[0] = drm_format_info_min_pitch(sb->format, 0, sb->width);

[Severity: Medium]
Could this result in a buffer overflow during panic rendering?

The vmw_priv->initial_width and initial_height dimensions are supplied by the 
host hypervisor and are copied directly to the drm_scanout_buffer without 
verifying that the resulting buffer size fits within the mapped dev->vram_size.

If the hypervisor provides excessively large dimensions, the generic drm_panic 
code will write out-of-bounds of the mapped PCI BAR region.

> +	return 0;
> +}
> +
> +void vmw_panic_flush(struct drm_plane *plane)
> +{
> +	struct vmw_private *vmw_priv = container_of(plane->dev, struct vmw_private, drm);
> +
> +	vmw_kms_write_svga(vmw_priv,
> +			   vmw_priv->initial_width, vmw_priv->initial_height,
> +			   vmw_priv->initial_width * 2, 16, 16);

[Severity: High]
Can this also cause a deadlock in the panic handler?

Similar to the vmw_write() call above, vmw_kms_write_svga() performs multiple
register reads and writes that invoke vmw_write() and acquire hw_lock on 
SVGA2 hardware. 

Since this runs in atomic panic context, acquiring a spinlock can deadlock 
the system.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929165104.928415-1-ian.forbes@broadcom.com?part=1