Re: [PATCH v9 25/61] drm/rockchip: dw_hdmi_qp: Add missing newlines in dev_err_probe() messages

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Dangling pointer in drvdata leads to Use-After-Free during system suspend/resume.
--

--- Patch [25]: [PATCH v9 25/61] drm/rockchip: dw_hdmi_qp: Add missing newlines in dev_err_probe() messages ---
commit 999c1afbb80ca84beaa74359eb4e6c431826da22
Author: Cristian Ciocaltea <[email protected]>

drm/rockchip: dw_hdmi_qp: Add missing newlines in dev_err_probe() messages

This commit adds missing trailing newlines to a couple of dev_err_probe()
calls in dw_hdmi_qp_rockchip_bind().

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 7c9d26d796642..369f1066dabe8 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
[ ... ]
> @@ -681,7 +681,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	drm_encoder_helper_add(encoder, &dw_hdmi_qp_rockchip_encoder_helper_funcs);
>  	ret = drmm_encoder_init(drm, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
>  	if (ret)
> -		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder");
> +		return dev_err_probe(hdmi->dev, ret, "Failed to init encoder\n");
>  
>  	platform_set_drvdata(pdev, hdmi);
>  
> @@ -689,8 +689,7 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  	if (IS_ERR(hdmi->hdmi)) {
>  		ret = PTR_ERR(hdmi->hdmi);
>  		hdmi->hdmi = NULL;
> -		return dev_err_probe(hdmi->dev, ret,
> -				     "Failed to bind dw-hdmi-qp");
> +		return dev_err_probe(hdmi->dev, ret, "Failed to bind dw-hdmi-qp\n");
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free during system suspend?

Because hdmi is allocated with drmm_kzalloc(), if dw_hdmi_qp_bind() or
drm_bridge_connector_init() fails later in this function, the DRM framework
will automatically free the memory. However, platform_set_drvdata() has
already stored the pointer, and it is never cleared in the error paths or
in dw_hdmi_qp_rockchip_unbind().

During system suspend, dw_hdmi_qp_rockchip_suspend() fetches this pointer:

dw_hdmi_qp_rockchip_suspend() {
        struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);

        if (hdmi && hdmi->hdmi)
                dw_hdmi_qp_suspend(dev, hdmi->hdmi);

        return 0;
}

If the pointer was freed, does the if (hdmi && hdmi->hdmi) check operate on
freed memory?

>  
>  	connector = drm_bridge_connector_init(drm, encoder);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=25