Re: [PATCH] drm: Prevent modeset helpers to access an uninitialized drm_mode_config

[Thomas Zimmermann]

Hi Javier

Am 24.07.22 um 20:41 schrieb Javier Martinez Canillas:
> Hello Thomas,
>
> Thanks for your feedback.
>
> On 7/24/22 20:24, Thomas Zimmermann wrote:
> > Hi Javier
> >
> > Am 24.07.22 um 14:37 schrieb Javier Martinez Canillas:
> > > DRM drivers initialize the mode configuration with drmm_mode_config_init()
> > > and that function (among other things) initializes mutexes that are later
> > > used by modeset helpers.
> > >
> > > But the helpers should only attempt to grab those locks if the mode config
> > > was properly initialized. Otherwise it can lead to kernel oops. An example
> > > is when a DRM driver using the component framework does not initialize the
> > > drm_mode_config, because its .bind callback was not being executed due one
> > > of its expected sub-devices' driver failing to probe.
> > >
> > > Some drivers check the struct drm_driver.registered field as an indication
> > > on whether their .shutdown callback should call helpers to tearn down the
> > > mode configuration or not, but most drivers just assume that it is always
> > > safe to call helpers such as drm_atomic_helper_shutdown() during shutdown.
> > >
> > > Let make the DRM core more robust and prevent this to happen, by marking a
> > > struct drm_mode_config as initialized during drmm_mode_config_init(). that
> > > way helpers can check for it and not attempt to grab uninitialized mutexes.
> >
> > I disagree. This patch looks like cargo-cult programming and entirely
> > arbitrary.  The solution here is to fix drivers.  The actual test to
> > perform is to instrument the mutex implementation to detect
> > uninitialized mutexes.
>
> While I do agree that drivers should be fixed, IMO we should try to make it
> hard for the kernel to crash. We already have checks in other DRM helpers to
> avoid accessing uninitialized data, so I don't see why we couldn't do the
> same here.

Code should stand on its own merits, instead of doing something because 
something else does it. The latter is what is referred to as cargo-cult 
programming.

Doing sanity checks on values is not a problem, but putting flag 
variables throughout the code to question other code's state is. That's 
not 'The Way of the C.' There's also the problem that a good part of 
struct drm_mode_config's initialization is open-coded in drivers. So the 
meaning of is_initialized is somewhat fuzzy.

> I wrote this patch after fixing a bug in the drm/msm driver [0]. By looking
> at how other drivers handled this case, I'm pretty sure that they have the
> same problem. A warning is much better than a kernel crash during shutdown.
>
> [0]: https://patchwork.kernel.org/project/dri-devel/patch/20220724111327.1195693-1-javierm@xxxxxxxxxx/

I see. I wasn't aware that missing mode_config_init() is a problem. From 
the linked URL, I cannot really understand how it's related. msm appears 
to be calling drm_mode_config_init(), right? The idiomatic solution 
would be to convert msm to managed code. But that's an entirely 
different patchset, of course. (I only took a brief look at the link TBH.)

Here's a suggestion on how to construct the mode-config code in order to 
make it hard to misuse:  Driver currently open-code the initialization 
of many fields in drm_mode_config. Expand the arguments of 
drm_mode_config_init() to take the pointer to the drm_mode_config_funcs. 
These functions are essential to do anything, so it's a good candidate 
for an argument. Drivers are easily converted the the new interface 
AFAICT.  After the conversion, add a test to drm_mode_config_reset() 
that tests for the funcs to be set.  drm_mode_config_reset() is also 
essential during initialization or the driver will fail immediately on 
the first modeset operation. That gives a test for an initialized 
mode_config without adding extra fields.

As a bit of a sidenote: we should consider making 
drm_mode_config_reset() and the reset callbacks return errors. The reset 
functions allocate memory for states and if this fails, we have no way 
of reporting the failure.

Best regards
Thomas



>   
> > Best regards
> > Thomas

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature