Re: [PATCH v3] drm/xe/xe_devcoredump: Check NULL before dereferencing

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Mar 22, 2024 at 12:08:31PM -0400, Souza, Jose wrote:
> On Fri, 2024-03-22 at 11:50 -0400, Rodrigo Vivi wrote:
> > On Fri, Mar 22, 2024 at 09:57:32AM -0400, Souza, Jose wrote:
> > > On Fri, 2024-03-22 at 09:28 +0530, Himal Prasad Ghimiray wrote:
> > > > Dereference 'coredump' to access 'xe_devcoredump_snapshot' only if
> > > > 'coredump' is not NULL,
> > > > 
> > > > v2
> > > > - Fix commit messages.
> > > > 
> > > > v3
> > > > - Define variables before code.(Ashutosh/Jose)
> > > > 
> > > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_devcoredump.c | 13 +++++++++----
> > > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> > > > index 7d3aa6bd3524..5b7be3b5b906 100644
> > > > --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> > > > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> > > > @@ -77,17 +77,22 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset,
> > > >  				   size_t count, void *data, size_t datalen)
> > > >  {
> > > >  	struct xe_devcoredump *coredump = data;
> > > > -	struct xe_device *xe = coredump_to_xe(coredump);
> > > > -	struct xe_devcoredump_snapshot *ss = &coredump->snapshot;
> > > > +	struct xe_device *xe;
> > > > +	struct xe_devcoredump_snapshot *ss;
> > > >  	struct drm_printer p;
> > > >  	struct drm_print_iterator iter;
> > > >  	struct timespec64 ts;
> > > >  	int i;
> > > >  
> > > > -	/* Our device is gone already... */
> > > > -	if (!data || !coredump_to_xe(coredump))
> > > > +	if (!coredump)
> > > > +		return -ENODATA;
> > > > +
> > > > +	xe = coredump_to_xe(coredump);
> > > > +	if (!xe)
> > > >  		return -ENODEV;
> > > 
> > > this is real bug? because I can't see how this function is called with a null data.
> > > 
> > > if yes...
> > > 
> > > if coredump is not null this will never return -ENODEV.
> > > 
> > > would drop it and return ENODEV in the check above.
> > 
> > yeap, very good point
> > 
> > this patch could be simply a 2 line patch:
> > 
> > - if (!data || !coredump_to_xe(coredump))
> > + if (!coredump_to_xe(coredump))
> > 
> > we are happy and the static analyzer is happy
> 
> coredump_to_xe() will always return something so not a good idea do this check.

right, I had forgotten it was a container_of...

So, for now let's go with your solution

-	struct xe_device *xe = coredump_to_xe(coredump);
-	struct xe_devcoredump_snapshot *ss = &coredump->snapshot;
+	struct xe_device *xe;
+	struct xe_devcoredump_snapshot *ss;

-	/* Our device is gone already... */
-	if (!data || !coredump_to_xe(coredump))
+	if (!coredump)
+		return -ENODEV;
+
+	xe = coredump_to_xe(coredump);

and nothing else, at least the static analyzer is happy.


> 
> > 
> > > 
> > > >  
> > > > +	ss = &coredump->snapshot;
> > > > +
> > > >  	/* Ensure delayed work is captured before continuing */
> > > >  	flush_work(&ss->work);
> > > >  
> > > 
>