Hi Iago,
On 8/12/24 04:01, Iago Toral wrote:
El vie, 09-08-2024 a las 12:18 -0300, Maíra Canal escribió:
When enabling UBSAN on Raspberry Pi 5, we get the following warning:
[ 387.894977] UBSAN: array-index-out-of-bounds in
drivers/gpu/drm/v3d/v3d_sched.c:320:3
[ 387.903868] index 7 is out of range for type '__u32 [7]'
[ 387.909692] CPU: 0 PID: 1207 Comm: kworker/u16:2 Tainted: G
WC 6.10.3-v8-16k-numa #151
[ 387.919166] Hardware name: Raspberry Pi 5 Model B Rev 1.0 (DT)
[ 387.925961] Workqueue: v3d_csd drm_sched_run_job_work [gpu_sched]
[ 387.932525] Call trace:
[ 387.935296] dump_backtrace+0x170/0x1b8
[ 387.939403] show_stack+0x20/0x38
[ 387.942907] dump_stack_lvl+0x90/0xd0
[ 387.946785] dump_stack+0x18/0x28
[ 387.950301] __ubsan_handle_out_of_bounds+0x98/0xd0
[ 387.955383] v3d_csd_job_run+0x3a8/0x438 [v3d]
[ 387.960707] drm_sched_run_job_work+0x520/0x6d0 [gpu_sched]
[ 387.966862] process_one_work+0x62c/0xb48
[ 387.971296] worker_thread+0x468/0x5b0
[ 387.975317] kthread+0x1c4/0x1e0
[ 387.978818] ret_from_fork+0x10/0x20
[ 387.983014] ---[ end trace ]---
This happens because the UAPI provides only seven configuration
registers and we are reading the eighth position of this u32 array.
Therefore, fix the out-of-bounds read in `v3d_csd_job_run()` by
accessing only seven positions on the '__u32 [7]' array. The eighth
register exists indeed on V3D 7.1, but it isn't currently used. That
being so, let's guarantee that it remains unused and add a note that
it
could be set in a future patch.
Fixes: 0ad5bc1ce463 ("drm/v3d: fix up register addresses for V3D
7.x")
Reported-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
---
drivers/gpu/drm/v3d/v3d_sched.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
b/drivers/gpu/drm/v3d/v3d_sched.c
index fd29a00b233c..e1565cf84abd 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -317,7 +317,7 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
struct v3d_dev *v3d = job->base.v3d;
struct drm_device *dev = &v3d->drm;
struct dma_fence *fence;
- int i, csd_cfg0_reg, csd_cfg_reg_count;
+ int i, csd_cfg0_reg;
v3d->csd_job = job;
@@ -337,9 +337,17 @@ v3d_csd_job_run(struct drm_sched_job *sched_job)
v3d_switch_perfmon(v3d, &job->base);
csd_cfg0_reg = V3D_CSD_QUEUED_CFG0(v3d->ver);
- csd_cfg_reg_count = v3d->ver < 71 ? 6 : 7;
- for (i = 1; i <= csd_cfg_reg_count; i++)
+ for (i = 1; i <= 6; i++)
V3D_CORE_WRITE(0, csd_cfg0_reg + 4 * i, job-
args.cfg[i]);
+
+ /* Although V3D 7.1 has an eighth configuration register, we
are not
+ * using it. Therefore, make sure it remains unused.
+ *
+ * XXX: Set the CFG7 register
+ */
+ if (v3d->ver >= 71)
+ V3D_CORE_WRITE(0, csd_cfg0_reg + 4 * i, 0);
+
Since we know we want to write CFG7 I'd suggest that we just write that
directly (csd_cfg0_reg + 4 * 7), instead of making it depend on the
value of 'i' exiting the previous loop. I think that makes it more
robust against future changes. Either way:
Reviewed-by: Iago Toral Quiroga <itoral@xxxxxxxxxx>
I decided to go with:
V3D_CORE_WRITE(0, V3D_V7_CSD_QUEUED_CFG7, 0);
to make sure that we are writing the CFG7 register.
I pushed the patch to drm/misc/drm-misc-fixes! Thanks for the review!
Best Regards,
- Maíra
Iago
/* CFG0 write kicks off the job. */
V3D_CORE_WRITE(0, csd_cfg0_reg, job->args.cfg[0]);
