Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Low] The commit message claims to inline `drm_simple_encoder_init()` for the meson driver, but it only modifies the HDMI encoder, leaving the deprecated helper in use by other encoders in the same driver. Pre-existing issues: - [High] The `meson_encoder_hdmi` structure (which embeds `struct drm_encoder`) is allocated with `devm_drm_bridge_alloc()`, but the encoder is initialized with `drm_encoder_init()`. This creates a mismatch in lifecycles that leads to a use-after-free when the platform device is unbound but userspace still holds a reference to the DRM device. -- --- Patch [1]: [PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init() --- commit bb202202974455054588168b82acfc49ff85b23f Author: Liam Zuiderhoek <[email protected]> GPU: drm: meson: inline call to drm_simple_encoder_init() This commit inlines the function drm_simple_encoder_init for the meson driver. It replaces the simple encoder init call with drm_encoder_init and a custom cleanup struct. > This commit inlines the function drm_simple_encoder_init for the meson > driver. It replaces the simple encoder init call with drm_encoder_init > and a custom cleanup struct. [Severity: Low] Does this commit miss updating other encoders in the meson driver? The commit message implies it inlines the function for the meson driver as a whole, but meson_encoder_cvbs_probe() in meson_encoder_cvbs.c and meson_encoder_dsi_probe() in meson_encoder_dsi.c still use drm_simple_encoder_init(). Additionally, is the include for <drm/drm_simple_kms_helper.h> still needed in meson_encoder_hdmi.c now that the helper has been open-coded? > diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c > b/drivers/gpu/drm/meson/meson_encoder_hdmi.c > index 0c7a72cb514ae..70a543358041f 100644 > --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c > +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c [ ... ] > @@ -407,8 +411,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv) > meson_encoder_hdmi->priv = priv; > > /* Encoder */ > - ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder, > - DRM_MODE_ENCODER_TMDS); > + ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder, > + &drm_simple_encoder_funcs_cleanup, > + DRM_MODE_ENCODER_TMDS, NULL); [Severity: High] This is a pre-existing issue, but does this allocation lifecycle mismatch lead to a use-after-free? Looking at meson_encoder_hdmi_probe(), the structure embedding the encoder is allocated with devm_drm_bridge_alloc(), tying its lifetime to the platform device: meson_encoder_hdmi_probe() { ... meson_encoder_hdmi = devm_drm_bridge_alloc(priv->dev, ...); ... } However, the encoder is initialized here with drm_encoder_init(), tying its cleanup to the DRM device's release. If the platform device is unbound, devres automatically frees meson_encoder_hdmi. If a userspace process holds an open file descriptor to the DRM device during unbind, the DRM device outlives the platform device. When the userspace process eventually closes the fd, drm_dev_release() calls drm_mode_config_cleanup(), which iterates over the mode config list and accesses the freed meson_encoder_hdmi->encoder to call its destroy callback. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
