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

[Ruben Wauters <rubenru09@aol.com>]

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

Hi,

On Wed, 2026-01-07 at 08:46 +0100, Thomas Zimmermann wrote:
> Hi Ruben
> 
> Am 03.01.26 um 20:18 schrieb Ruben Wauters:
> > 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.
> 
> This is correct behavior.
> 
> > > 
> > > 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()
> 
> If the plane has been disbabled, fb and crtc are NULL. So this is 
> correct. drm_atomic_helper_check_plane_state() is the place to do this 
> testing before doing much else in the atomic_check handler. If 
> drm_atomic_helper_check_plane_state() gives an error, the error should 
> be returns. But if it returns 0 and sets ->visible to false, the 
> atomic_check should return 0.  That means that the plane can 
> successfully be disabled.
> 
> > 
> > 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.
> 
> Disabling a plane is not an error, but a common operation.

I think I may have misunderstood the drm docs on this, if having crtc
and fb be null and returning 0 then is ok, I am happy for this patch to
be applied. I have tested it and it indeed works, and removes the oops
present in the driver before this.
> 
> I think the patch is fine and IIRC we have similar logic in other drivers.

Reviewed-by: Ruben Wauters <rubenru09@aol.com>

I believe Shenghao mentioned another oops that is present? if so it may
be best to submit that in a separate patch rather than a v2 of this
one.

Ruben
> 
> Best regards
> Thomas
> 
> > 
> > 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 --]