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

[Matthew Brost]

On Tue, Nov 05, 2024 at 09:01:52PM +0100, Michal Wajdeczko wrote:
> 
> 
> 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;
> 

Fair enough:
Reviewed-by: Matthew Brost <matthew.brost at intel.com>

> > 
> > Matt 
> > 
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> Cc: Matthew Brost <matthew.brost at 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
> >>
>