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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c6324a66-5886-4fbb-ba7b-fc7782c0f790@suse.de>
Date: Wed, 7 Jan 2026 08:46:44 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Ruben Wauters <rubenru09@....com>, Shenghao Yang <me@...nghaoyang.info>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB
 disconnect

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 the patch is fine and IIRC we have similar logic in other drivers.

Best regards
Thomas

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

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)