Re: [PATCH v2 12/17] arm64: dts: apple: t8112: add avd nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] media: apple: null pointer dereference in avd_init_job() on allocation failure
- [High] media: apple: use-after-free of avd_ctx in avd_watchdog_func()
- [Low] media: v4l2: off-by-one error allows out-of-bounds context_update_tile_id
- [Low] dt-bindings: media: apple,avd: missing t8132 and t8140 compatible strings
--

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

arm64: dts: apple: t8112: add avd nodes
    
This patch series adds the Apple Video Decoder (AVD) driver to support
stateless video decoding on newer Apple SoCs.

> diff --git a/Documentation/devicetree/bindings/media/apple,avd.yaml b/Documentation/devicetree/bindings/media/apple,avd.yaml
> new file mode 100644
> index 0000000000000..0aa2cffae50f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/apple,avd.yaml

[ ... ]

> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - apple,t6030-avd
> +              - apple,t6031-avd
> +          - const: apple,t8122-avd
> +      - enum:
> +          - apple,t8103-avd
> +          - apple,t8112-avd
> +          - apple,t8122-avd
> +          - apple,t6000-avd
> +          - apple,t6020-avd

[Severity: Low]
Do the dt-bindings miss the apple,t8132-avd and apple,t8140-avd compatible
strings?

The AVD driver explicitly registers support for apple,t8132-avd and
apple,t8140-avd via the avd_of_match array in avd-drv.c.

Because the device tree binding schema documentation omits these compatible
strings, validating device trees that contain these hardware variants fails
with a dt-schema validation error.

[ ... ]

> diff --git a/drivers/media/platform/apple/avd/avd-drv.c b/drivers/media/platform/apple/avd/avd-drv.c
> new file mode 100644
> index 0000000000000..68752c44d4ec6
> --- /dev/null
> +++ b/drivers/media/platform/apple/avd/avd-drv.c

[ ... ]

> +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]
Is there a risk of a null pointer dereference of job->buf.cpu on allocation
failure?

If avd_buf_alloc() fails and returns -ENOMEM, it leaves job->buf.cpu as null.
Since the return value assigned to ret is not checked, the subsequent memset()
will unconditionally use the null pointer.

This path is reachable from userspace by submitting decode jobs, meaning
memory pressure could induce a kernel panic.

> +	return ret;
> +}

[ ... ]

> +static void avd_watchdog_func(struct work_struct *work)
> +{
> +	struct avd_dev *avd;
> +	struct avd_ctx *ctx;
> +	int ret;
> +
> +	ctx = container_of(to_delayed_work(work), struct avd_ctx,
> +			   watchdog_work);
> +	if (!ctx)
> +		return;
> +
> +	avd = ctx->dev;

[Severity: High]
Could this result in a use-after-free of ctx if it runs concurrently with
avd_release()?

In avd_release(), cancel_delayed_work() is used to stop the watchdog timer,
which doesn't block if the work is currently executing on another CPU.
Following this, avd_release() frees the context:

avd-drv.c:avd_release() {
	...
	cancel_delayed_work(&ctx->watchdog_work);
	...
	kfree(ctx);
}

If avd_watchdog_func() is running concurrently, won't it dereference the
freed ctx pointer when accessing ctx->dev?

This race condition can be triggered by closing the video device while a
decoding timeout occurs, resulting in memory corruption or a kernel panic.

[ ... ]

> +static int avd_release(struct file *filp)
> +{
> +	struct avd_ctx *ctx = file_to_ctx(filp);
> +
> +	cancel_delayed_work(&ctx->watchdog_work);
> +
> +	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;
> +}

[ ... ]

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index 9caca56478d1a..11ced5f639071 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c

[ ... ]

> @@ -800,10 +800,25 @@ static int validate_av1_film_grain(struct v4l2_ctrl_av1_film_grain *fg)
>  	return 0;
>  }
>  
> +static int validate_av1_tile_info(struct v4l2_av1_tile_info *ti)
> +{
> +	if (ti->tile_cols > V4L2_AV1_MAX_TILE_COLS ||
> +	    ti->tile_rows > V4L2_AV1_MAX_TILE_ROWS)
> +		return -EINVAL;
> +
> +	if (ti->context_update_tile_id > ti->tile_cols * ti->tile_rows)

[Severity: Low]
Does this check allow an out-of-bounds context_update_tile_id to pass
validation?

Since context_update_tile_id is a 0-based index for the tiles, an ID exactly
equal to ti->tile_cols * ti->tile_rows is out-of-bounds.

Shouldn't this check use >= instead of > to properly enforce the AV1 bitstream
specification, which mandates that context_update_tile_id must be strictly
less than TileCols * TileRows?

This allows an invalid tile ID to be passed to hardware drivers, which could
result in undefined logic or hardware state.

> +		return -EINVAL;
> +
> +	return 0;
> +}

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