| 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
| ||
|
Message-ID: <001d01dbaa48$ead66d10$c0834730$@martijnvandeventer.nl>
Date: Thu, 10 Apr 2025 20:46:48 +0200
From: <linux@...tijnvandeventer.nl>
To: "'Martin Blumenstingl'" <martin.blumenstingl@...glemail.com>,
<linux-amlogic@...ts.infradead.org>,
<dri-devel@...ts.freedesktop.org>
Cc: <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<jbrunet@...libre.com>,
<neil.armstrong@...aro.org>,
"'Furkan Kardame'" <f.kardame@...jaro.org>
Subject: RE: [PATCH] drm/meson: fix resource cleanup in meson_drv_bind_master() on error
Hi Martin,
Thank you for the patch.
I encountered this issue some time ago as well and had a possible fix in my tree (see
below).
My apologies for not upstreaming it earlier.
While my fix is not as symmetric as yours—I like symmetry—it is somewhat simpler. It
did make the assumption that only calling component_unbind_all() was at fault and the the rest of the
code was correct. Therefore, calling one of the following functions:
meson_encoder_dsi_remove()
meson_encoder_hdmi_remove()
meson_encoder_cvbs_remove()
in case their counterpart was not called, should not result in any issues.
I just verified, and, as far as I understand, all of these functions do a check to confirm
whether the encoder was initialized before proceeding with cleanup.
-----------------------------------------------------
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index 81d2ee37e773..4e2d45a271c2 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -184,6 +184,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
const struct meson_drm_match_data *match;
struct meson_drm *priv;
struct drm_device *drm;
+ bool do_unbind = false;
struct resource *res;
void __iomem *regs;
int ret, i;
@@ -312,10 +313,9 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
ret = component_bind_all(dev, drm);
if (ret) {
dev_err(drm->dev, "Couldn't bind all components\n");
- /* Do not try to unbind */
- has_components = false;
goto exit_afbcd;
}
+ do_unbind = true;
}
ret = meson_encoder_hdmi_probe(priv);
@@ -378,7 +378,7 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
meson_encoder_hdmi_remove(priv);
meson_encoder_cvbs_remove(priv);
- if (has_components)
+ if (do_unbind)
component_unbind_all(dev, drm);
return ret;
-----------------------------
This patch has somewhat less code redundancy. I don’t have a strong
preference for either patch, so please feel free to choose whichever you
prefer.
If you decide to go with yours, I’ve provided one minor comment inline
for your consideration.
> -----Original Message-----
> From: linux-amlogic <linux-amlogic-bounces@...ts.infradead.org> On Behalf
> Of Martin Blumenstingl
> Sent: Wednesday, April 9, 2025 11:44 PM
> To: linux-amlogic@...ts.infradead.org; dri-devel@...ts.freedesktop.org
> Cc: linux-kernel@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> jbrunet@...libre.com; neil.armstrong@...aro.org; Martin Blumenstingl
> <martin.blumenstingl@...glemail.com>; Furkan Kardame
> <f.kardame@...jaro.org>
> Subject: [PATCH] drm/meson: fix resource cleanup in
> meson_drv_bind_master() on error
>
> meson_drv_bind_master() does not free resources in the order they are
> allocated. This can lead to crashes such as:
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000000000000c8
> [...]
> Hardware name: Beelink GT-King Pro (DT)
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
> lr : component_unbind+0x38/0x60
> [...]
> Call trace:
> meson_dw_hdmi_unbind+0x10/0x24 [meson_dw_hdmi]
> component_unbind+0x38/0x60
> component_unbind_all+0xb8/0xc4
> meson_drv_bind_master+0x1ec/0x514 [meson_drm]
> meson_drv_bind+0x14/0x20 [meson_drm]
> try_to_bring_up_aggregate_device+0xa8/0x160
> __component_add+0xb8/0x1a8
> component_add+0x14/0x20
> meson_dw_hdmi_probe+0x1c/0x28 [meson_dw_hdmi]
> platform_probe+0x68/0xdc
> really_probe+0xc0/0x39c
> __driver_probe_device+0x7c/0x14c
> driver_probe_device+0x3c/0x120
> __driver_attach+0xc4/0x200
> bus_for_each_dev+0x78/0xd8
> driver_attach+0x24/0x30
> bus_add_driver+0x110/0x240
> driver_register+0x68/0x124
> __platform_driver_register+0x24/0x30
> meson_dw_hdmi_platform_driver_init+0x20/0x1000 [meson_dw_hdmi]
> do_one_initcall+0x50/0x1bc
> do_init_module+0x54/0x1fc
> load_module+0x788/0x884
> init_module_from_file+0x88/0xd4
> __arm64_sys_finit_module+0x248/0x340
> invoke_syscall+0x48/0x104
> el0_svc_common.constprop.0+0x40/0xe0
> do_el0_svc+0x1c/0x28
> el0_svc+0x30/0xcc
> el0t_64_sync_handler+0x120/0x12c
> el0t_64_sync+0x190/0x194
>
> Clean up resources in the error path in the same order and under the
> same conditions as they were allocated to fix this.
>
> Reported-by: Furkan Kardame <f.kardame@...jaro.org>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
> ---
> This issue was reported off-list so I'm unable to provide a link to the
> report.
>
> I'm not sure which Fixes tag fits best. My preference so far is
> Fixes: 6a044642988b ("drm/meson: fix unbind path if HDMI fails to bind")
> but I'll happily take other suggestions as well.
>
>
> drivers/gpu/drm/meson/meson_drv.c | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_drv.c
> b/drivers/gpu/drm/meson/meson_drv.c
> index 81d2ee37e773..031686fd4104 100644
> --- a/drivers/gpu/drm/meson/meson_drv.c
> +++ b/drivers/gpu/drm/meson/meson_drv.c
> @@ -314,35 +314,35 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
> dev_err(drm->dev, "Couldn't bind all
> components\n");
> /* Do not try to unbind */
> has_components = false;
Above two lines are no longer used, so can be removed.
> - goto exit_afbcd;
> + goto cvbs_encoder_remove;
> }
> }
>
> ret = meson_encoder_hdmi_probe(priv);
> if (ret)
> - goto exit_afbcd;
> + goto unbind_components;
>
> if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> ret = meson_encoder_dsi_probe(priv);
> if (ret)
> - goto exit_afbcd;
> + goto hdmi_encoder_remove;
> }
>
> ret = meson_plane_create(priv);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> ret = meson_overlay_create(priv);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> ret = meson_crtc_create(priv);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> ret = request_irq(priv->vsync_irq, meson_irq, 0, drm->driver->name,
> drm);
> if (ret)
> - goto exit_afbcd;
> + goto dsi_encoder_remove;
>
> drm_mode_config_reset(drm);
>
> @@ -360,6 +360,16 @@ static int meson_drv_bind_master(struct device
> *dev, bool has_components)
>
> uninstall_irq:
> free_irq(priv->vsync_irq, drm);
> +dsi_encoder_remove:
> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> + meson_encoder_dsi_remove(priv);
> +hdmi_encoder_remove:
> + meson_encoder_hdmi_remove(priv);
> +unbind_components:
> + if (has_components)
> + component_unbind_all(dev, drm);
> +cvbs_encoder_remove:
> + meson_encoder_cvbs_remove(priv);
> exit_afbcd:
> if (priv->afbcd.ops)
> priv->afbcd.ops->exit(priv);
> @@ -374,13 +384,6 @@ 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 (has_components)
> - component_unbind_all(dev, drm);
> -
> return ret;
> }
>
--
Best regards,
Martijn
Powered by blists - more mailing lists