Re: [PATCH v2 6/6] staging: media: cedrus: Use devm_of_reserved_mem_device_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Early IRQ registration and incorrect devres initialization order causes kernel panics.
- [High] Devres cleanup clears CMA area before userspace file descriptors are closed, causing memory corruption on free.
--

commit be60086a6fce8e7046d627a68c0fd2fc94983ddb
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

staging: media: cedrus: Use devm_of_reserved_mem_device_init()

Use the devres-managed devm_of_reserved_mem_device_init() instead of
the manual of_reserved_mem_device_init()/of_reserved_mem_device_release()
pair, letting the device resource manager handle cleanup automatically.

> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index 444fb53878d12..4a15f45ad077f 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -266,7 +266,7 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
>  		return ret;
>  	}
>  
> -	ret = of_reserved_mem_device_init(dev->dev);
> +	ret = devm_of_reserved_mem_device_init(dev->dev);

[Severity: High]
This is a pre-existing issue, but does tying the reserved memory cleanup to
devres cause memory corruption when userspace holds file descriptors open?

The driver unregisters the video device via video_unregister_device() in
cedrus_remove(), which does not wait for userspace to close open file
descriptors. Userspace can still hold v4l2 buffers allocated from the
contiguous memory area pool.

By using devm_of_reserved_mem_device_init(), the device's memory area
(dev->cma_area) is automatically cleared on driver unbind.

When userspace eventually closes the file descriptor, cedrus_release() cleans
up the context:

drivers/staging/media/sunxi/cedrus/cedrus.c:cedrus_release() {
    ...
    v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
    ...
}

This will eventually call dma_free_contiguous() to free the buffers. Because
dev->cma_area is now null, dma_free_contiguous() assumes the memory was not
allocated from the device's reserved pool and falls back to __free_pages(page,
get_order(size)).

Since the contiguous allocator returns exactly the requested number of pages
while __free_pages() frees a power-of-two block, won't this free memory
belonging to other allocations and cause severe memory corruption?


[Severity: High]
This isn't a bug introduced by this patch, but is it safe to request the
interrupt this early in cedrus_hw_probe()?

Just above this diff in cedrus_hw_probe(), devm_request_irq() is called
before dev->watchdog_work and dev->m2m_dev are initialized (which happens
later in cedrus_probe()).

If the bootloader leaves the hardware active, an interrupt can fire
immediately, and cedrus_irq() will dereference dev->m2m_dev:

drivers/staging/media/sunxi/cedrus/cedrus_hw.c:cedrus_irq() {
    ...
    ctx = v4l2_m2m_get_curr_priv(dev->m2m_dev);
    ...
}

Since dev->m2m_dev is still null, won't this cause a null pointer dereference
inside spin_lock_irqsave() in v4l2_m2m_get_curr_priv()?

Additionally, devm_request_irq() is called before
devm_platform_ioremap_resource() maps dev->base. Because devres cleans up
resources in reverse order, dev->base will be unmapped before the interrupt
is freed.

If an interrupt fires during this unmapped window (such as during driver
unbind), won't cedrus_irq() access unmapped I/O memory and cause a page fault
or similar abort?

>  	if (ret && ret != -ENODEV) {
>  		dev_err(dev->dev, "Failed to reserve memory\n");

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902200703.2016410-1-mukesh.ojha@oss.qualcomm.com?part=6