| Message ID | 20240809124725.17956-1-abelova@astralinux.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/meson: add check to prevent dereference of NULL | expand |
Hello Anastasia, Thank you for working on this! On Fri, Aug 9, 2024 at 2:48 PM Anastasia Belova <abelova@astralinux.ru> wrote: [...] > @@ -373,9 +373,11 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) > free_drm: > drm_dev_put(drm); > > - meson_encoder_dsi_remove(priv); > - meson_encoder_hdmi_remove(priv); > - meson_encoder_cvbs_remove(priv); > + if (priv) { > + meson_encoder_dsi_remove(priv); > + meson_encoder_hdmi_remove(priv); > + meson_encoder_cvbs_remove(priv); > + } This is the straight-forward approach. There's been conversions from non-devm_ functions to their devm_* counterparts in the past in various subsystems. I just found that there's a devm_drm_dev_alloc() which seems to be calling drm_dev_put() automatically - but I have never used it myself before. As an alternative to your suggested approach: could you please look into whether devm_drm_dev_alloc() is a suitable replacement (if not: just explain why - then this patch is good to be merged)? Thank you! Martin
Hello Martin, 10/08/24 12:09, Martin Blumenstingl пишет: > Hello Anastasia, > > Thank you for working on this! > > On Fri, Aug 9, 2024 at 2:48 PM Anastasia Belova <abelova@astralinux.ru> wrote: > [...] >> @@ -373,9 +373,11 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) >> free_drm: >> drm_dev_put(drm); >> >> - meson_encoder_dsi_remove(priv); >> - meson_encoder_hdmi_remove(priv); >> - meson_encoder_cvbs_remove(priv); >> + if (priv) { >> + meson_encoder_dsi_remove(priv); >> + meson_encoder_hdmi_remove(priv); >> + meson_encoder_cvbs_remove(priv); >> + } > This is the straight-forward approach. > > There's been conversions from non-devm_ functions to their devm_* > counterparts in the past in various subsystems. > I just found that there's a devm_drm_dev_alloc() which seems to be > calling drm_dev_put() automatically - but I have never used it myself > before. > As an alternative to your suggested approach: could you please look > into whether devm_drm_dev_alloc() is a suitable replacement (if not: > just explain why - then this patch is good to be merged)? If I understood correctly, devm_drm_dev_alloc allows to delete drm_dev_put from error path in meson_drv_bind_master and in meson_drv_unbind. Then the proposed check may be ommited and function may just return instead of jumping to free_drm. And would it be better to rename free_drm to remove_encoders? Thank you, Anastasia Belova
Hello Anastasia, On Mon, Aug 12, 2024 at 11:32 AM Anastasia Belova <abelova@astralinux.ru> wrote: [...] > > As an alternative to your suggested approach: could you please look > > into whether devm_drm_dev_alloc() is a suitable replacement (if not: > > just explain why - then this patch is good to be merged)? > > If I understood correctly, devm_drm_dev_alloc allows to delete drm_dev_put > from error path in meson_drv_bind_master and in meson_drv_unbind. Correct, that's the idea. > Then the proposed check may be ommited and function may just return > instead of jumping to free_drm. That's right > And would it be better to rename free_drm to remove_encoders? That free_drm label is very confusing anyways. The short answer is: yes The longer answer is: we'll need to work on the removal order again: - encoders are probed *after* afbcd - removal should happen in opposite order of probe, so encoders should be freed *before* afbcd - however, this order is not implemented There's no harm for you to rename the label now. It just means that we need to do some more cleanups. Looking forward to v2 of the patch! Best regards, Martin
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index 4bd0baa2a4f5..3d143340e3f7 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -373,9 +373,11 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) free_drm: drm_dev_put(drm); - meson_encoder_dsi_remove(priv); - meson_encoder_hdmi_remove(priv); - meson_encoder_cvbs_remove(priv); + if (priv) { + meson_encoder_dsi_remove(priv); + meson_encoder_hdmi_remove(priv); + meson_encoder_cvbs_remove(priv); + } if (has_components) component_unbind_all(dev, drm);
If devm_kzalloc gives NULL instead of allocating priv, execution goes to free_drm where priv is dereferenced calling meson_encoder_dsi_remove, meson_encoder_hdmi_remove and meson_encoder_cvbs_remove. Add NULL-check for priv. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Anastasia Belova <abelova@astralinux.ru> --- drivers/gpu/drm/meson/meson_drv.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)