[PATCH 04/12] drm/v3d: Validate passed in drm syncobj handles in the timestamp extension

[Tvrtko Ursulin]

On 10/07/2024 18:06, Maíra Canal wrote:
> On 7/10/24 10:41, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>
>> If userspace provides an unknown or invalid handle anywhere in the handle
>> array the rest of the driver will not handle that well.
>>
>> Fix it by checking handle was looked up successfuly or otherwise fail the
> 
> I believe you mean "Fix it by checking if the handle..."
> 
> Also s/successfuly/successfully

Oops, thank you!

> 
>> extension by jumping into the existing unwind.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>> Fixes: 9ba0ff3e083f ("drm/v3d: Create a CPU job extension for the 
>> timestamp query job")
>> Cc: Maíra Canal <mcanal at igalia.com>
>> Cc: Iago Toral Quiroga <itoral at igalia.com>
>> Cc: <stable at vger.kernel.org> # v6.8+
>> ---
>>   drivers/gpu/drm/v3d/v3d_submit.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c 
>> b/drivers/gpu/drm/v3d/v3d_submit.c
>> index ca1b1ad0a75c..3313423080e7 100644
>> --- a/drivers/gpu/drm/v3d/v3d_submit.c
>> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
>> @@ -497,6 +497,10 @@ v3d_get_cpu_timestamp_query_params(struct 
>> drm_file *file_priv,
>>           }
>>           job->timestamp_query.queries[i].syncobj = 
>> drm_syncobj_find(file_priv, sync);
>> +        if (!job->timestamp_query.queries[i].syncobj) {
>> +            err = -ENOENT;
> 
> I'm not sure if err should be -ENOENT or -EINVAL, but based on other 
> drivers, I believe it should be -EINVAL.

After a quick grep I am inclined to think ENOENT is correct. DRM core 
uses that, and drivers seem generally confused (split between ENOENT and 
EINVAL). With one even going for ENODEV!

Regards,

Tvrtko
>> +            goto error;
>> +        }
>>       }
>>       job->timestamp_query.count = timestamp.count;
>> @@ -550,6 +554,10 @@ v3d_get_cpu_reset_timestamp_params(struct 
>> drm_file *file_priv,
>>           }
>>           job->timestamp_query.queries[i].syncobj = 
>> drm_syncobj_find(file_priv, sync);
>> +        if (!job->timestamp_query.queries[i].syncobj) {
>> +            err = -ENOENT;
>> +            goto error;
>> +        }
>>       }
>>       job->timestamp_query.count = reset.count;
>> @@ -613,6 +621,10 @@ v3d_get_cpu_copy_query_results_params(struct 
>> drm_file *file_priv,
>>           }
>>           job->timestamp_query.queries[i].syncobj = 
>> drm_syncobj_find(file_priv, sync);
>> +        if (!job->timestamp_query.queries[i].syncobj) {
>> +            err = -ENOENT;
>> +            goto error;
>> +        }
>>       }
>>       job->timestamp_query.count = copy.count;