Re: [RFC PATCH 6/6] ARM: dts: rockchip: Add RKVENC operating points to RV1126

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] media: rkvenc: probe fails in rkvenc_dvfs_cooling_init()
  without THERMAL_OF
- [High] media: rkvenc: teardown race in rkvenc_remove() and UAF in
  rkvenc_release()
- [High] media: rkvenc: SCLR_DONE IRQ and unlocked job_running race in
  rkvenc
- [High] media: rkvenc: slot DMA buffer overflow via
  rkvenc_s_fmt_vid_out()
- [Medium] media: rkvenc: missing initial OPP and PM lifecycle for
  venc-supply
- [Medium] media: rkvenc: unbalanced pm_runtime_enable() in
  rkvenc_probe()
--

commit f41b6ddf3d36774f6ef1b61d948fc7c27e1896c8
Author: Daniel Golle <daniel@makrotopia.org>

ARM: dts: rockchip: Add RKVENC operating points to RV1126

This commit adds the operating points table for the RKVENC video encoder on
RV1126 and registers the encoder node as a cooling device.

> diff --git a/arch/arm/boot/dts/rockchip/rv1126.dtsi b/arch/arm/boot/dts/rockchip/rv1126.dtsi
> index 63705291dd3ce..7e37fd89609a7 100644
> --- a/arch/arm/boot/dts/rockchip/rv1126.dtsi
> +++ b/arch/arm/boot/dts/rockchip/rv1126.dtsi
> @@ -104,6 +104,30 @@ xin24m: oscillator {
>  		#clock-cells = <0>;
>  	};
>  
> +	venc_opp_table: opp-table-venc {
> +		compatible = "operating-points-v2";
> +
> +		opp-297000000 {
> +			opp-hz = /bits/ 64 <297000000>;
> +			opp-microvolt = <725000 725000 1000000>;
> +		};

[Severity: Medium]
On boards that define venc-supply, does rkvenc_dvfs_init() ever apply the
initial OPP voltage or disable venc-supply across runtime suspend?

In commit "media: rockchip: rkvenc: Add frequency scaling and cooling",
devm_pm_opp_set_config() acquires venc-supply without enabling it or
setting its voltage, and devfreq_add_device() does not run an initial
update_devfreq() because polling_ms is 0:

drivers/media/platform/rockchip/rkvenc/rkvenc-dvfs.c:rkvenc_dvfs_init() {
    ...
	if (of_property_present(dev->of_node, "venc-supply"))
		opp_config.regulator_names = supply_names;

	ret = devm_pm_opp_set_config(dev, &opp_config);
	if (ret)
		return ret;

	ret = devm_pm_opp_of_add_table(dev);
    ...
}

As a result, the first batch of encode jobs runs at the 396 MHz assigned
clock rate before dev_pm_opp_set_rate() has ever configured or enabled
venc-supply. Once rkvenc_dvfs_job_end() later triggers dev_pm_opp_set_rate(),
rkvenc_runtime_suspend() only disables clocks without calling
dev_pm_opp_set_rate(dev, 0), leaving venc-supply enabled at the last active
OPP voltage (up to 825 mV) while idle.

[ ... ]

> @@ -665,6 +689,8 @@ venc: video-codec@ffbb0000 {
>  		clock-names = "axi", "ahb", "core";
>  		assigned-clocks = <&cru ACLK_VENC>, <&cru CLK_VENC_CORE>;
>  		assigned-clock-rates = <297000000>, <396000000>;
> +		operating-points-v2 = <&venc_opp_table>;
> +		#cooling-cells = <2>;

[Severity: High]
Does adding #cooling-cells = <2> cause rkvenc_probe() to fail with -ENODEV
when CONFIG_THERMAL_OF is disabled?

VIDEO_ROCKCHIP_RKVENC does not depend on or select THERMAL or THERMAL_OF,
and thermal_of_cooling_device_register() returns ERR_PTR(-ENODEV) when
CONFIG_THERMAL_OF is disabled, which rkvenc_dvfs_cooling_init() treats as a
fatal probe error:

drivers/media/platform/rockchip/rkvenc/rkvenc-dvfs.c:rkvenc_dvfs_cooling_init() {
	if (!of_property_present(dev->of_node, "#cooling-cells"))
		return 0;
    ...
	dvfs->cdev = thermal_of_cooling_device_register(
		dev->of_node, 0, "rkvenc", dvfs, &rkvenc_cdev_ops);
	if (IS_ERR(dvfs->cdev)) {
		ret = PTR_ERR(dvfs->cdev);
		dvfs->cdev = NULL;
		return dev_err_probe(dev, ret,
				     "could not add the cooling device\n");
	}
}

[Severity: Medium]
In commit "media: rockchip: Add RKVENC v1 video encoder driver", does
rkvenc_probe() leave runtime PM enabled if pm_runtime_resume_and_get()
fails?

drivers/media/platform/rockchip/rkvenc/rkvenc.c:rkvenc_probe() {
    ...
	pm_runtime_enable(dev);

	ret = pm_runtime_resume_and_get(dev);
	if (ret)
		return ret;
    ...
err_pm_disable:
	pm_runtime_disable(dev);
	return ret;
}

Returning directly instead of jumping to err_pm_disable leaves
dev->power.disable_depth unbalanced.

[Severity: High]
In commits "media: rockchip: Add RKVENC v1 video encoder driver" and
"media: rockchip: rkvenc: Add frequency scaling and cooling", can
rkvenc_remove() and rkvenc_release() trigger a use-after-free on
dvfs->devfreq, rkvenc_dev, and rkvenc->m2m_dev?

drivers/media/platform/rockchip/rkvenc/rkvenc.c:rkvenc_remove() {
	struct rkvenc_dev *rkvenc = platform_get_drvdata(pdev);

	cancel_delayed_work_sync(&rkvenc->watchdog);
	rkvenc_dvfs_exit(rkvenc);
	rkvenc_v4l2_cleanup(rkvenc);
	v4l2_m2m_release(rkvenc->m2m_dev);
	v4l2_device_unregister(&rkvenc->v4l2_dev);
	pm_runtime_disable(&pdev->dev);
}

rkvenc_remove() cancels the watchdog and tears down DVFS before stopping
V4L2/M2M:

CPU0                              CPU1
rkvenc_remove()
  rkvenc_dvfs_exit()
                                  rkvenc_irq_thread()
                                    rkvenc_finish_job()
                                      rkvenc_dvfs_job_end()
                                        devfreq = dvfs->devfreq
    devfreq_remove_device(devfreq)
                                        mutex_lock(&devfreq->lock)
                                          <-- use-after-free

In addition, active jobs during rkvenc_dvfs_exit() can re-queue
dvfs->throttle_work and rkvenc->watchdog after cancel_delayed_work_sync().
Finally, because struct rkvenc_dev is allocated with devm_kzalloc() and
rkvenc->m2m_dev is freed in rkvenc_remove(), closing an open V4L2 file
descriptor after unbind runs rkvenc_release() ->
rkvenc_dvfs_ctx_del(ctx->dev, ctx) and v4l2_m2m_ctx_release(ctx->m2m_ctx)
on freed ctx->dev and rkvenc->m2m_dev memory.

[Severity: High]
In commit "media: rockchip: Add RKVENC v1 video encoder driver", can
rkvenc_hw_recover() and unlocked updates to dev->job_running race with
rkvenc_irq_thread() and rkvenc_watchdog_work()?

First, the IRQ is registered with IRQF_ONESHOT, which keeps the interrupt
line masked while rkvenc_irq_thread() is running. When rkvenc_finish_job()
-> rkvenc_hw_recover() waits on dev->sclr_done from inside
rkvenc_irq_thread(), RKVENC_INT_SCLR_DONE cannot be delivered until
rkvenc_irq_thread() returns, so wait_for_completion_timeout() always times
out and the subsequent RKVENC_INT_SCLR_DONE interrupt wakes a spurious
rkvenc_irq_thread().

Second, rkvenc_device_run() sets dev->job_ctx and dev->job_running = true
without holding dev->irq_lock and before sleeping in
rkvenc_dvfs_job_begin(), calling pm_runtime_resume_and_get(), and starting
the hardware in rkvenc_hw_run():

CPU0                              CPU1
rkvenc_device_run()
  dev->job_ctx = ctx
  dev->job_running = true
  rkvenc_dvfs_job_begin()
                                  rkvenc_irq_thread() /
                                  rkvenc_watchdog_work()
                                    spin_lock_irqsave(&dev->irq_lock)
                                    if (dev->job_running)
                                      dev->job_running = false
                                    spin_unlock_irqrestore(&dev->irq_lock)
                                    rkvenc_finish_job()
                                      pm_runtime_put()
                                      v4l2_m2m_buf_done_and_job_finish()
  pm_runtime_resume_and_get()
  rkvenc_hw_run()  <-- starts DMA on completed vb2 buffers

When the spurious rkvenc_irq_thread() or a racing rkvenc_watchdog_work()
(since rkvenc_irq_thread() uses non-sync cancel_delayed_work()) runs in
that window, rkvenc_finish_job() reads MMIO while the device may be
suspended, underflows the runtime PM usage count, and returns the vb2
buffers to userspace before rkvenc_hw_run() starts DMA on them.

[Severity: High]
In commit "media: rockchip: Add RKVENC v1 video encoder driver", can
changing the OUTPUT format while CAPTURE is streaming overflow the
reference frame DMA buffers?

rkvenc_start_streaming() only calls rkvenc_ref_alloc() when starting the
CAPTURE queue, sizing ctx->slot[] from ctx->width and ctx->height once while
ctx->slots_alloc is false. However, rkvenc_s_fmt_vid_out() only checks
whether the OUTPUT queue is busy before updating ctx->width and ctx->height:

drivers/media/platform/rockchip/rkvenc/rkvenc-v4l2.c:rkvenc_s_fmt_vid_out() {
	struct rkvenc_ctx *ctx = fh_to_rkvenc_ctx(file->private_data);
	struct vb2_queue *vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);

	if (vb2_is_busy(vq))
		return -EBUSY;

	rkvenc_try_fmt_vid_out(ctx, f);
    ...
	ctx->width = f->fmt.pix_mp.width;
	ctx->height = f->fmt.pix_mp.height;
}

If userspace starts streaming on CAPTURE first and then increases the
OUTPUT dimensions with VIDIOC_S_FMT before starting OUTPUT,
rkvenc_device_run() programs the encoder with the larger resolution while
DMA-writing reconstructed frames into the smaller ctx->slot[] buffers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794666330.git.daniel@makrotopia.org?part=6