Re: [PATCH 3/4] drm/xe/guc: Don't read data from G2H prior to length check

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 05.11.2024 20:36, Matthew Brost wrote:
> On Tue, Nov 05, 2024 at 06:30:31PM +0100, Michal Wajdeczko wrote:
>> While highly unlikely, incoming G2H message might be too short
>> so we shouldn't read any data from it prior to checking a length.
>>
> 
> Is this really needed though? The *msg is a per CT member:
> 
> xe_guc_ct_types.h
> 
> 147         /** @msg: Message buffer */
> 148         u32 msg[GUC_CTB_MSG_MAX_LEN];
> 149         /** @fast_msg: Message buffer */
> 150         u32 fast_msg[GUC_CTB_MSG_MAX_LEN];
> 
> 
> I suppose this good practice but this is not an actual problem though, right?

it's not a problem in sense that it will not crash per today's
implementation, hence no Fixes: tag, but if that CT implementation would
change in the future then per function signature agreement it will be
the GuC submit code fault to read outside provided message len.

note that since we are provided with len and msg we shouldn't make any
further assumptions about implementation, as otherwise it would be
sufficient to access message directly using already provided guc:

	msg = guc->ct.msg;

> 
> Matt 
> 
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_guc_submit.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index 4481890be941..147000fd1177 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -1885,12 +1885,14 @@ static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q,
>>  int xe_guc_sched_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>>  {
>>  	struct xe_exec_queue *q;
>> -	u32 guc_id = msg[0];
>> -	u32 runnable_state = msg[1];
>> +	u32 guc_id, runnable_state;
>>  
>>  	if (unlikely(len < 2))
>>  		return -EPROTO;
>>  
>> +	guc_id = msg[0];
>> +	runnable_state = msg[1];
>> +
>>  	q = g2h_exec_queue_lookup(guc, guc_id);
>>  	if (unlikely(!q))
>>  		return -EPROTO;
>> @@ -1924,11 +1926,13 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
>>  int xe_guc_deregister_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>>  {
>>  	struct xe_exec_queue *q;
>> -	u32 guc_id = msg[0];
>> +	u32 guc_id;
>>  
>>  	if (unlikely(len < 1))
>>  		return -EPROTO;
>>  
>> +	guc_id = msg[0];
>> +
>>  	q = g2h_exec_queue_lookup(guc, guc_id);
>>  	if (unlikely(!q))
>>  		return -EPROTO;
>> @@ -1950,11 +1954,13 @@ int xe_guc_exec_queue_reset_handler(struct xe_guc *guc, u32 *msg, u32 len)
>>  {
>>  	struct xe_gt *gt = guc_to_gt(guc);
>>  	struct xe_exec_queue *q;
>> -	u32 guc_id = msg[0];
>> +	u32 guc_id;
>>  
>>  	if (unlikely(len < 1))
>>  		return -EPROTO;
>>  
>> +	guc_id = msg[0];
>> +
>>  	q = g2h_exec_queue_lookup(guc, guc_id);
>>  	if (unlikely(!q))
>>  		return -EPROTO;
>> @@ -2010,11 +2016,13 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
>>  {
>>  	struct xe_gt *gt = guc_to_gt(guc);
>>  	struct xe_exec_queue *q;
>> -	u32 guc_id = msg[0];
>> +	u32 guc_id;
>>  
>>  	if (unlikely(len < 1))
>>  		return -EPROTO;
>>  
>> +	guc_id = msg[0];
>> +
>>  	q = g2h_exec_queue_lookup(guc, guc_id);
>>  	if (unlikely(!q))
>>  		return -EPROTO;
>> -- 
>> 2.43.0
>>