Re: [PATCH v2 06/17] media: apple: add avd driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] media: apple: avd: buffer overflow in push() due to missing bounds check
- [High] media: apple: avd: devm_kzalloc of video_device causes Use-After-Free
- [High] media: apple: avd: non-synchronous cancel_delayed_work() causes UAF and deadlock
- [High] media: apple: avd: missing error check in avd_init_job() causes NULL dereference
- [High] media: apple: avd: UAF of m2m_dev in threaded IRQ handler during unbind
- [High] media: apple: avd: off-by-one in AVD_PIODMA_CMD_SIZE causes OOB DMA read
- [High] media: apple: avd: VIDIOC_TRY_FMT mutates internal context state corrupting decodes
- [High] media: apple: avd: missing IS_ERR check on reset control causes panic
- [High] media: apple: avd: race between threaded IRQ and watchdog completes wrong job
- [High] media: apple: avd: avd_stop_streaming reclaims active buffers leading to DMA UAF
- [High] media: apple: avd: missing size validation in avd_boot causes OOB write
- [High] media: apple: avd: device left detached from IOMMU domain after reset
- [Medium] media: apple: avd: unsafe hardware register access in IRQ handler
--

commit f9c8639c9bc75cad903911b0f8b6e86b0705207f
Author: Sofus Forstreuter <sofus.c@icloud.com>

media: apple: add avd driver

Add the AVD (Apple Video Decoder) driver with V4L2 M2M stateless
support based largely on rockchips implementation.

> diff --git a/drivers/media/platform/apple/avd/avd-drv.c b/drivers/media/platform/apple/avd/avd-drv.c
> index 0000000000000..68752c44d4ec6 100644
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-drv.c
[ ... ]
> @@ -133,14 +133,16 @@ int avd_end_segment(struct avd_ctx *ctx, bool update_submit)
>  	struct avd_job *job = &ctx->job;
>  	struct avd_segment *seg = &job->segments[job->num];
>  
>  	/* avd_segment includes piodma_cmd which is not transferred */
>  	seg->piodma_cmd =
>  		AVD_PIODMA_CMD_SIZE((sizeof(struct avd_segment) - 8) / 4);

[Severity: High]
Does this calculation result in an off-by-one error for the command size?
The macro AVD_PIODMA_CMD_SIZE appears to expect the transfer size in number
of words minus one. Passing the exact word count (512) could cause the
hardware to read one word past the end of the segment's instructions array,
leading to an out-of-bounds DMA read.

>  	seg->piodma_cmd |= AVD_PIODMA_CMD_DEST(job->dest);
>  	seg->piodma_cmd |= AVD_PIODMA_CMD_CONST;
[ ... ]
> @@ -148,15 +150,16 @@ int avd_init_job(struct avd_ctx *ctx, enum avd_codec codec, size_t segments)
>  {
>  	int ret = 0;
>  	struct avd_job *job = &ctx->job;
>  
>  	job->codec = codec;
>  	job->dest = 0x1000;
>  	job->num = 0;
>  	job->num_submit = 0;
>  	job->num_alloc = segments;
>  	ret = avd_buf_alloc(ctx->dev, &job->buf,
>  			    job->num_alloc * sizeof(*job->segments));
>  	job->segments = job->buf.cpu;
>  	memset(job->buf.cpu, 0, job->buf.size);

[Severity: High]
Can job->buf.cpu be NULL here if the allocation failed?
The return value of avd_buf_alloc is assigned to ret, but it isn't checked
before the unconditional memset, which would lead to a NULL pointer dereference
under memory pressure.

>  	return ret;
>  }
[ ... ]
> @@ -195,30 +198,34 @@ static int avd_boot(struct avd_dev *avd)
>  {
>  	u32 val;
>  	int ret;
>  	char version[64];
>  
>  	if (avd->variant->revision != 3)
>  		dev_info_once(avd->dev, "booting hw version: %04x",
>  			      readl_relaxed(avd->ctrl));
>  
>  	writel(avd->sram_start, avd->piodma + 0x24);
>  	dev_info_once(avd->dev, "piodma version: %04x base: %08x",
>  		      readl_relaxed(avd->piodma + 0xb4),
>  		      readl_relaxed(avd->piodma + 0x24));
>  
>  	memcpy_toio(avd->code, avd->fw->data, avd->fw->size);

[Severity: High]
Is it possible for the firmware size to exceed the mapped IO memory region?
Since avd->fw->size is not validated against the resource size of avd->code,
an excessively large firmware image could cause an out-of-bounds MMIO write.

>  
>  	writel_relaxed(AVD_MBOX_ENABLE, avd->mbox + AVD_REG_MBOX1_STATUS);
[ ... ]
> @@ -235,21 +242,23 @@ static void avd_shutdown(struct avd_dev *avd)
>  static int avd_reset(struct avd_dev *avd)
>  {
>  	int ret = 0;
>  
>  	ret = pm_runtime_resume_and_get(avd->dev);
>  	if (ret < 0)
>  		return ret;
>  
>  	ret = reset_control_reset(avd->rstc);
>  	if (ret)
>  		dev_err(avd->dev, "reset: failed: %d", ret);
>  
>  	iommu_attach_device(avd->empty_domain, avd->dev);
>  	iommu_detach_device(avd->empty_domain, avd->dev);

[Severity: High]
Does this leave the device permanently detached from any functional IOMMU
translation context?
By detaching the empty domain without subsequently attaching the working
domain (avd->domain), it appears the device is left without an active domain,
which could cause IOMMU faults on the next DMA operation.

>  
>  	ret = avd_boot(avd);
[ ... ]
> @@ -288,27 +297,32 @@ static void avd_watchdog_func(struct work_struct *work)
>  static irqreturn_t avd_irq_handler(int irq, void *data)
>  {
>  	struct avd_dev *avd = data;
>  	struct avd_ctx *ctx = v4l2_m2m_get_curr_priv(avd->m2m_dev);
>  	enum vb2_buffer_state state;
>  	u32 status;
>  
>  	status = readl(avd->mbox + AVD_REG_MBOX0_RETRIEVE);

[Severity: Medium]
Could this register read happen while the hardware is suspended?
According to the power management guidelines, an IRQ handler shouldn't access
hardware registers without first using pm_runtime_get_if_active() to ensure
the device isn't in an RPM_SUSPENDED state, which could result in invalid data
(0xffffffff) or bus errors during a spurious interrupt.

>  	writel(AVD_MBOX0_NOT_EMPTY, avd->mbox + AVD_REG_MBOX_IRQ_CLR);
>  
>  	if (status & 0x10000) { /* dbg */
>  		dev_warn(avd->dev, "no handler for IRQ: %3d",
>  			 status & ~0x10000);
>  		writel_relaxed(0, avd->mbox + AVD_REG_MBOX_IRQ_ENABLE);
>  		return IRQ_HANDLED;
>  	}
>  
>  	if (!ctx)
>  		return IRQ_HANDLED;

[Severity: High]
Is it safe to retrieve the context via v4l2_m2m_get_curr_priv(avd->m2m_dev)
during teardown, or when a watchdog race occurs?

First, during module unbind (avd_remove), avd_v4l2_cleanup() is called which
frees the m2m_dev before the devres-managed IRQ handler is disabled. If an
interrupt fires in that window, wouldn't v4l2_m2m_get_curr_priv(avd->m2m_dev)
trigger a use-after-free on m2m_dev?

Second, if the watchdog completes a timed-out job and the M2M framework
dispatches a new job, a concurrent interrupt for the timed-out job could fetch
the newly dispatched job's context here. Calling avd_job_finish(ctx, state) on
this new context could prematurely complete it.

>  
>  	if (status & 0x1000) {
[ ... ]
> @@ -428,21 +442,22 @@ static int avd_open(struct file *filp)
>  static int avd_release(struct file *filp)
>  {
>  	struct avd_ctx *ctx = file_to_ctx(filp);
>  
>  	cancel_delayed_work(&ctx->watchdog_work);

[Severity: High]
Can a non-synchronous cancel_delayed_work() leave the watchdog executing
while the context is freed?
If the watchdog is executing on another CPU, cancel_delayed_work() returns
while the handler runs. The subsequent kfree(ctx) destroys the context, causing
a use-after-free in avd_watchdog_func(). Moreover, if the watchdog is blocked
from calling avd_job_finish(), v4l2_m2m_ctx_release() could block indefinitely
resulting in a deadlock.

>  
>  	v4l2_fh_del(&ctx->fh, filp);
>  	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>  	v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
>  	v4l2_fh_exit(&ctx->fh);
>  	avd_buf_free(ctx->dev, &ctx->inst);
>  	avd_buf_free(ctx->dev, &ctx->pipe_state);
>  	avd_buf_free(ctx->dev, &ctx->job.buf);
>  	kfree(ctx);
>  
>  	return 0;
>  }
[ ... ]
> @@ -507,21 +522,23 @@ static int avd_probe(struct platform_device *pdev)
>  	struct avd_dev *avd;
>  	int ret, irq;
>  
>  	avd = devm_kzalloc(&pdev->dev, sizeof(*avd), GFP_KERNEL);

[Severity: High]
Does allocating the avd structure with devm_kzalloc() create a use-after-free
risk when tied to the V4L2 device lifecycle?
The avd structure contains the video_device (avd->vdev). Because it's managed
by devres, it gets freed when avd_remove() completes. If a user keeps a file
descriptor open, V4L2 core will access the freed video_device memory upon
subsequent ioctls or closure.

>  	if (!avd)
>  		return -ENOMEM;
>  
>  	platform_set_drvdata(pdev, avd);
>  	avd->dev = &pdev->dev;
>  	avd->pdev = pdev;
>  
>  	mutex_init(&avd->vdev_lock);
>  
>  	avd->variant = of_device_get_match_data(&pdev->dev);
>  
>  	avd->rstc = devm_reset_control_get_exclusive(avd->dev, NULL);

[Severity: High]
Could avd->rstc be an ERR_PTR if devm_reset_control_get_exclusive() fails?
Without an IS_ERR check, a failure like -EPROBE_DEFER or -ENOMEM would store
an ERR_PTR here, which would later cause a kernel panic when dereferenced by
reset_control_reset(avd->rstc) in avd_reset().

>  
>  	avd->piodma = devm_platform_ioremap_resource_byname(pdev, "piodma");
[ ... ]
> @@ -541,20 +558,21 @@ static int avd_probe(struct platform_device *pdev)
>  	irq = platform_get_irq_byname(pdev, "mbox0");
>  	if (irq < 0)
>  		return irq;
>  	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, avd_irq_handler,
>  					IRQF_ONESHOT, dev_name(&pdev->dev),
>  					avd);

[Severity: High]
Does using devm_request_threaded_irq() keep the interrupt alive too long
during teardown?
Because devres cleans this up after avd_remove() finishes, an interrupt can
fire after avd_v4l2_cleanup() has manually freed m2m_dev, leading to the
aforementioned use-after-free in avd_irq_handler().

>  	if (ret) {
>  		dev_err(avd->dev, "Could not request IRQ 0");
>  		return ret;
>  	}
[ ... ]
> @@ -587,14 +605,15 @@ static int avd_probe(struct platform_device *pdev)
>  static void avd_remove(struct platform_device *pdev)
>  {
>  	struct avd_dev *avd = platform_get_drvdata(pdev);
>  
>  	avd_v4l2_cleanup(avd);

[Severity: High]
Could this manual cleanup lead to a use-after-free due to the devm_kzalloc() of
avd?
Once avd_v4l2_cleanup() and avd_remove() complete, devres reclaims the avd
memory. This leaves V4L2 with a dangling pointer to the video_device if file
descriptors are still open.

>  
>  	iommu_domain_free(avd->empty_domain);

> diff --git a/drivers/media/platform/apple/avd/avd-inst.h b/drivers/media/platform/apple/avd/avd-inst.h
> index 0000000000000..d44d1f7f66c20 100644
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-inst.h
[ ... ]
> @@ -134,14 +134,15 @@ static inline void push(struct avd_ctx *ctx, u32 inst)
>  {
>  	struct avd_job *job = &ctx->job;
>  	struct avd_segment *seg = &job->segments[job->num];
>  
>  	seg->instructions[seg->num++] = inst;

[Severity: Critical]
Is seg->num bounded against overflowing the instructions array?
Without checking if seg->num exceeds AVD_MAX_INST (512), a complex bitstream
from userspace could cause an out-of-bounds write. This could overwrite memory
like the piodma_cmd of the next segment, potentially injecting arbitrary DMA
commands.

>  }
>  

> diff --git a/drivers/media/platform/apple/avd/avd-v4l2.c b/drivers/media/platform/apple/avd/avd-v4l2.c
> index 0000000000000..88079da5ee276 100644
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-v4l2.c
[ ... ]
> @@ -121,21 +121,23 @@ static void avd_fill_decoded_pixfmt(struct avd_ctx *ctx,
>  				    struct v4l2_pix_format_mplane *pix_mp)
>  {
>  	v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat, pix_mp->width,
>  			    pix_mp->height);
>  
>  	if (is_interchange(pix_mp->pixelformat))
>  		pix_mp->plane_fmt[0].sizeimage = 0;
>  	ctx->comp.start_offset = pix_mp->plane_fmt[0].sizeimage;
>  
>  	fill_comp(&ctx->comp, ctx->image_fmt, pix_mp->width, pix_mp->height);

[Severity: High]
Does this mutate the driver context state during VIDIOC_TRY_FMT?
The V4L2 specification explicitly prohibits VIDIOC_TRY_FMT from modifying
internal state. Since avd_try_capture_fmt calls this helper, untrusted
userspace could probe formats during active streaming and silently corrupt
the global ctx->comp state used by avd_run_preamble() for active decodes.

>  	pix_mp->plane_fmt[0].sizeimage += ctx->comp.size;
[ ... ]
> @@ -321,18 +323,20 @@ static int avd_v4l2_init(struct avd_dev *avd)
>  	avd->mdev.dev = avd->dev;
>  	strscpy(avd->mdev.model, "avd", sizeof(avd->mdev.model));
>  	strscpy(avd->mdev.bus_info, "platform:avd", sizeof(avd->mdev.bus_info));
>  	media_device_init(&avd->mdev);
>  	avd->mdev.ops = &avd_media_ops;
>  	avd->v4l2_dev.mdev = &avd->mdev;
>  
>  	avd->vdev.lock = &avd->vdev_lock;
>  	avd->vdev.v4l2_dev = &avd->v4l2_dev;
>  	avd->vdev.fops = &avd_fops;
>  	avd->vdev.release = video_device_release_empty;

[Severity: High]
Does setting release to video_device_release_empty bypass standard V4L2 cleanup?
Opting out of the standard V4L2 release tracking prevents the core from safely
deferring cleanup until the last file descriptor is closed, leading to the
use-after-free since the video_device is part of the devm_kzalloc'd avd struct.

>  	avd->vdev.vfl_dir = VFL_DIR_M2M;
[ ... ]
> @@ -495,27 +499,32 @@ static void avd_queue_cleanup(struct vb2_queue *vq, u32 state)
>  static void avd_stop_streaming(struct vb2_queue *q)
>  {
>  	struct avd_ctx *ctx = vb2_get_drv_priv(q);
>  
>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
>  		const struct avd_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>  
>  		if (WARN_ON(!desc))
>  			return;
>  
>  		if (desc->ops->stop)
>  			desc->ops->stop(ctx);
>  	}
>  
>  	avd_queue_cleanup(q, VB2_BUF_STATE_ERROR);

[Severity: High]
Is it safe to unconditionally reclaim all buffers while hardware decoding could
be active?
When VIDIOC_STREAMOFF invokes avd_stop_streaming(), avd_queue_cleanup() is
called without synchronizing against the actively running hardware job.
This causes the queues to be abruptly emptied, returning the buffers to vb2.
The hardware could continue its DMA into these reclaimed buffers.

>  }
>  
>  const struct vb2_ops avd_queue_ops = {
>  	.queue_setup = avd_queue_setup,
>  	.buf_prepare = avd_buf_prepare,
>  	.buf_queue = avd_buf_queue,
>  	.buf_out_validate = avd_buf_out_validate,
>  	.buf_request_complete = avd_buf_request_complete,
>  	.start_streaming = avd_start_streaming,
>  	.stop_streaming = avd_stop_streaming,
>  };
>  
>  void avd_job_finish_no_pm(struct avd_ctx *ctx, enum vb2_buffer_state result)
>  {
>  	if (ctx->coded_fmt_desc->ops->done) {
>  		struct vb2_v4l2_buffer *src_buf, *dst_buf;
>  
>  		src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);

[Severity: High]
Could v4l2_m2m_next_src_buf() return NULL here if avd_stop_streaming() emptied
the queue?
If a stream is closed while the hardware is still active, the queues are
cleared by avd_queue_cleanup(). When the hardware job eventually finishes,
this attempt to retrieve the source buffer will yield NULL, which will then
be passed to desc->ops->done(), causing a NULL pointer dereference.

>  		dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>  		ctx->coded_fmt_desc->ops->done(ctx, src_buf, dst_buf, result);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926-avd-v2-0-ecebe6a3648e@icloud.com?part=6