diff mbox series

drm/v3d: Fix out-of-bounds read in `v3d_csd_job_run()`

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

Commit Message

Maíra Canal Aug. 9, 2024, 3:18 p.m. UTC 
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(-)

Comments

Iago Toral Aug. 12, 2024, 7:01 a.m. UTC | #1 
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]);
>
Maíra Canal Aug. 12, 2024, 3:01 p.m. UTC | #2 
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 mbox series

Patch

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]);