Re: [PATCH RFC 02/13] drm/gma500: Publish the Cedarview SGX as an auxiliary device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/gma500: race clearing sgx_data in
  gma_sgx_set_irq_handler()
- [Medium] drm/gma500: auxiliary device leak and UAF in
  psb_driver_load()
- [Medium] drm/gma500: unmasked _PSB_IRQ_SGX_FLAG and SGX write in
  gma_irq_preinstall()
--

commit 4cc756dfd45e5db0eddaf8ca3914b3626c097190
Author: Joseph Telaak <joseph@telaak.dev>

drm/gma500: Publish the Cedarview SGX as an auxiliary device

This commit publishes the Cedarview PowerVR SGX545 as an auxiliary device
("gma500_gfx.sgx"), skipping internal SGX MMU setup and register
initialization on chips with a dedicated SGX driver and routing SGX
interrupts to a registered callback.

> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index d17cb5b4a4bf2..c29cc64cc10b3 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c

[ ... ]

> @@ -383,6 +393,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
>  	spin_unlock_irqrestore(&dev_priv->irqmask_lock, irqflags);
>  
>  	gma_irq_install(dev);
> +	if (dev_priv->ops->sgx)
> +		gma_sgx_aux_init(dev);
>  
>  	dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */

[Severity: Medium]
Does calling gma_sgx_aux_init() here leak the auxiliary device and leave
a dangling aux->dev_priv pointer if gma_backlight_init() fails below?

drivers/gpu/drm/gma500/psb_drv.c:psb_driver_load() {
    ...
	if (ret)
		return ret;
	psb_intel_opregion_enable_asle(dev);

	return devm_add_action_or_reset(dev->dev, psb_device_release, dev);
    ...
}

When gma_backlight_init() returns an error, psb_driver_load() returns
directly before devm_add_action_or_reset() registers psb_device_release()
and without calling psb_driver_unload().  devres then frees dev_priv while
the auxiliary device remains registered on the auxiliary bus, causing a
use-after-free when an SGX driver probes or calls set_irq_handler().

> diff --git a/drivers/gpu/drm/gma500/psb_irq.c b/drivers/gpu/drm/gma500/psb_irq.c
> index c224c7ff353ce..426efc126bee6 100644
> --- a/drivers/gpu/drm/gma500/psb_irq.c
> +++ b/drivers/gpu/drm/gma500/psb_irq.c

[ ... ]

> @@ -257,8 +266,11 @@ void gma_irq_preinstall(struct drm_device *dev)
>  	PSB_WVDC32(0xFFFFFFFF, PSB_HWSTAM);
>  	PSB_WVDC32(0x00000000, PSB_INT_MASK_R);
>  	PSB_WVDC32(0x00000000, PSB_INT_ENABLE_R);
> -	PSB_WSGX32(0x00000000, PSB_CR_EVENT_HOST_ENABLE);
> -	PSB_RSGX32(PSB_CR_EVENT_HOST_ENABLE);
> +	/* An SGX driver of its own enables what it needs; until one binds, mask it. */
> +	if (!gma_sgx_external(dev_priv) || !READ_ONCE(dev_priv->sgx_handler)) {
> +		PSB_WSGX32(0x00000000, PSB_CR_EVENT_HOST_ENABLE);
> +		PSB_RSGX32(PSB_CR_EVENT_HOST_ENABLE);
> +	}

[Severity: Medium]
Can an active SGX interrupt trigger an unhandled IRQ storm while
dev_priv->sgx_handler is NULL?

gma_irq_preinstall() and gma_irq_postinstall() unconditionally enable
_PSB_IRQ_SGX_FLAG in dev_priv->vdc_irq_mask (written to PSB_INT_MASK_R and
PSB_INT_ENABLE_R), and gma_sgx_set_irq_handler() never masks or unmasks
_PSB_IRQ_SGX_FLAG in the VDC when registering or clearing sgx_handler.
Meanwhile, gma_irq_preinstall() leaves PSB_CR_EVENT_HOST_ENABLE2 untouched
even though psb_do_init() skips psb_spank().  If an SGX event is enabled
in PSB_CR_EVENT_HOST_ENABLE2 across kexec, or remains enabled in
PSB_CR_EVENT_HOST_ENABLE when the SGX driver unregisters its handler on an
error path, gma_irq_handler() repeatedly sees sgx_int with a NULL
sgx_handler and returns IRQ_NONE without clearing the level-triggered SGX
interrupt, disabling the shared PCI interrupt line and breaking display
interrupts.

Also, writing PSB_CR_EVENT_HOST_ENABLE here when gma_sgx_external() is
true contradicts the comments in include/drm/gma500_sgx.h, psb_drv.h, and
sgx_aux.c stating that gma500 never touches SGX registers on chips that
publish the auxiliary device.  Should gma500 instead gate
_PSB_IRQ_SGX_FLAG in the VDC interrupt registers from
gma_sgx_set_irq_handler()?

> diff --git a/drivers/gpu/drm/gma500/sgx_aux.c b/drivers/gpu/drm/gma500/sgx_aux.c
> new file mode 100644
> index 0000000000000..671cb6c330ec7
> --- /dev/null
> +++ b/drivers/gpu/drm/gma500/sgx_aux.c
> @@ -0,0 +1,102 @@

[ ... ]

> +static int gma_sgx_set_irq_handler(struct gma500_sgx_device *sgx,
> +				   gma500_sgx_irq_handler_t handler, void *data)
> +{
> +	struct gma_sgx_aux *aux = container_of(sgx, struct gma_sgx_aux, sgx);
> +	struct drm_psb_private *dev_priv = aux->dev_priv;
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->dev.dev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev_priv->irqmask_lock, flags);
> +	if (handler && dev_priv->sgx_handler) {
> +		spin_unlock_irqrestore(&dev_priv->irqmask_lock, flags);
> +		return -EBUSY;
> +	}
> +	dev_priv->sgx_data = data;
> +	/* The data reaches the interrupt handler before the handler does. */
> +	smp_store_release(&dev_priv->sgx_handler, handler);
> +	spin_unlock_irqrestore(&dev_priv->irqmask_lock, flags);
> +
> +	/* No handler call still running on another CPU once this returns. */
> +	if (!handler && dev_priv->irq_enabled)
> +		synchronize_irq(pdev->irq);
> +	return 0;
> +}

[Severity: Medium]
Can unregistering the interrupt handler via
gma_sgx_set_irq_handler(sgx, NULL, NULL) pass a NULL data pointer to an
in-flight sgx_handler() on another CPU?

CPU0                                  CPU1 (<hardirq>)
gma_sgx_set_irq_handler(sgx, NULL, NULL)
                                      gma_irq_handler()
                                        sgx_handler = smp_load_acquire(
                                                        &dev_priv->sgx_handler);
  spin_lock_irqsave(&irqmask_lock)
  dev_priv->sgx_data = NULL;
  smp_store_release(&sgx_handler, NULL);
  spin_unlock_irqrestore(&irqmask_lock)
                                        sgx_handler(dev_priv->sgx_data)
                                          <-- NULL pointer dereference
  synchronize_irq(pdev->irq);

gma_irq_handler() drops dev_priv->irqmask_lock before loading sgx_handler
and reading dev_priv->sgx_data, while gma_sgx_set_irq_handler() overwrites
dev_priv->sgx_data before clearing dev_priv->sgx_handler and before
calling synchronize_irq(), causing the SGX driver's interrupt callback to
dereference NULL in hardirq context.  In addition, because sgx_handler is
already NULL when irqmask_lock is dropped before synchronize_irq()
finishes, a concurrent registration could overwrite dev_priv->sgx_data
while the old handler is still running.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-powervr-sgx-v1-0-c69b84bac011@telaak.dev?part=2