RE: [PATCH] drm/auth: Only drm_drop_master if it exists

[Cavitt, Jonathan]

-----Original Message-----
From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx> 
Sent: Wednesday, April 22, 2026 12:14 PM
To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx>
Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Gupta, Saurabhg <saurabhg.gupta@xxxxxxxxx>; Zuo, Alex <alex.zuo@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx; mripard@xxxxxxxxxx; tzimmerman@xxxxxxx; airlied@xxxxxxxxx; simona@xxxxxxxx
Subject: Re: [PATCH] drm/auth: Only drm_drop_master if it exists
> 
> On Fri, Apr 17, 2026 at 05:00:47AM +0800, Jonathan Cavitt wrote:
> > It is possible that both dev->master and file_priv->master are NULL when
> > passed to drm_master_release, which would result in dev being passed to
> > drm_drop_master (as NULL == NULL here).  This would result in a NULL
> > pointer dereference when passing dev->master to drm_master_put in
> > drm_drop_master.
> > 
> > Only call drm_drop_master if dev->master exists.  Also, make sure the
> > original calling requirement is maintained (dev->master ==
> > file_priv->master).
> > 
> > This fixes a static analysis issue.
> > 
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Cc: David Airlie <airlied@xxxxxxxxx>
> > Cc: Simona Vetter <simona@xxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_auth.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> > index e17bb0f1f9e0..e5013b870ba0 100644
> > --- a/drivers/gpu/drm/drm_auth.c
> > +++ b/drivers/gpu/drm/drm_auth.c
> > @@ -347,7 +347,7 @@ void drm_master_release(struct drm_file *file_priv)
> >  	if (!drm_is_current_master_locked(file_priv))
> >  		goto out;
> >  
> > -	if (dev->master == file_priv->master)
> 
> The previous call checks that file_priv->master != NULL. So, at this
> point the NULL == NULL check is not possible.

You mean drm_is_current_master_locked?

Internally, that performs the check
"fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master"

It doesn't explicitly check for fpriv->master != NULL, but I don't think the current execution
path allows for fpriv->is_master && !fpriv->master, so I'm guessing that's what you're referring to?
Otherwise, we could be passing a NULL pointer to drm_lease_owner, which would itself be a NULL
pointer dereference issue...

> 
> I'm sorry, but this looks like a false positive.

This has already been merged.  If you think it necessary, please feel free to file a revert
and send the request my way for review.
-Jonathan Cavitt

> 
> > +	if (dev->master && dev->master == file_priv->master)
> >  		drm_drop_master(dev, file_priv);
> >  out:
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET) && file_priv->is_master) {
> > -- 
> > 2.53.0
> > 
> 
> -- 
> With best wishes
> Dmitry
>