Message ID | 20240809152001.668314-1-mcanal@igalia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/v3d: Fix out-of-bounds read in `v3d_csd_job_run()` | expand |
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@igalia.com> > Signed-off-by: Maíra Canal <mcanal@igalia.com> > --- > 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@igalia.com> Iago > /* CFG0 write kicks off the job. */ > V3D_CORE_WRITE(0, csd_cfg0_reg, job->args.cfg[0]); >
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@igalia.com> >> Signed-off-by: Maíra Canal <mcanal@igalia.com> >> --- >> 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@igalia.com> 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]); >> >
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); + /* CFG0 write kicks off the job. */ V3D_CORE_WRITE(0, csd_cfg0_reg, job->args.cfg[0]);
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@igalia.com> Signed-off-by: Maíra Canal <mcanal@igalia.com> --- drivers/gpu/drm/v3d/v3d_sched.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)