Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect

[Ruben Wauters <rubenru09@aol.com>]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

Hi

On Sun, 2026-01-04 at 01:47 +0800, Shenghao Yang wrote:
> Hi Ruben,
> 
> On 4/1/26 01:23, Ruben Wauters wrote:
> 
> > With the elimination of these two WARN_ON_ONCEs, it's possible that
> > crtc_state may not be assigned below, and therefore may be read/passed
> > to functions when it is NULL (e.g. line 488). Either protection for a
> > null crtc_state should be added to the rest of the function, or the
> > function shouldn't continue if crtc is NULL.
> > 
> > Ruben
> > > -	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > -
> > > -	mode = &crtc_state->mode;
> > > +	if (crtc)
> > > +		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > >  
> > >  	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
> > >  						  DRM_PLANE_NO_SCALING,
> > > @@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane,
> > >  	if (old_plane_state->rotation != new_plane_state->rotation)
> > >  		crtc_state->mode_changed = true;
> > >  
> > > +	mode = &crtc_state->mode;
> > > +	format = fb->format;
> 
> Yup - in this case I'm relying on drm_atomic_helper_check_plane_state()
> bailing out early after seeing that fb is NULL (since a NULL crtc should
> imply no fb) and setting plane_state->visible to false.
> 
> That would cause an early return in gud_plane_atomic_check() without
> dereferencing crtc_state.

This does work, however this ends up returning 0, which implies that
the atomic check succeded. In my opinion in this case, -EINVAL should
be returned, as both the crtc and fb don't exist, therefore the check
should not succeed. I would personally prefer a more explicit check
that does return -EINVAL instead of 0 from
drm_atomic_helper_check_planes()

As a side note, I'm not sure if there's a reasoning as to why
drm_atomic_helper_check_planes() returns 0 if fb is NULL instead of 
-EINVAL, I'm assuming it's not designed to come across this specific
case. Either way it's not too much of an issue but maybe one of the drm
maintainers can clarify why it's this way.

Ruben
> 
> Would a more explicit check be preferred?
> 
> Thanks,
> 
> Shenghao

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]