| 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: <CAA8EJpoxe8BmyFqMC5yrqdx-Sx2VR_2gT3x6WT9MyhdLuw+xmA@mail.gmail.com>
Date: Thu, 22 Feb 2024 22:57:07 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Johan Hovold <johan+linaro@...nel.org>
Cc: Bjorn Andersson <andersson@...nel.org>, Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Vinod Koul <vkoul@...nel.org>, Jonas Karlman <jonas@...boo.se>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jernej Skrabec <jernej.skrabec@...il.com>, Konrad Dybcio <konrad.dybcio@...aro.org>,
Kishon Vijay Abraham I <kishon@...nel.org>, Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>, Kuogee Hsieh <quic_khsieh@...cinc.com>,
freedreno@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-phy@...ts.infradead.org
Subject: Re: [PATCH 2/6] drm/bridge: aux-hpd: separate allocation and registration
On Sat, 17 Feb 2024 at 17:03, Johan Hovold <johan+linaro@...nel.org> wrote:
>
> Combining allocation and registration is an anti-pattern that should be
> avoided. Add two new functions for allocating and registering an dp-hpd
> bridge with a proper 'devm' prefix so that it is clear that these are
> device managed interfaces.
>
> devm_drm_dp_hpd_bridge_alloc()
> devm_drm_dp_hpd_bridge_add()
>
> The new interface will be used to fix a use-after-free bug in the
> Qualcomm PMIC GLINK driver and may prevent similar issues from being
> introduced elsewhere.
>
> The existing drm_dp_hpd_bridge_register() is reimplemented using the
> above and left in place for now.
>
> Signed-off-by: Johan Hovold <johan+linaro@...nel.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Minor nit below.
> ---
> drivers/gpu/drm/bridge/aux-hpd-bridge.c | 67 +++++++++++++++++++------
> include/drm/bridge/aux-bridge.h | 15 ++++++
> 2 files changed, 67 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> index 9e71daf95bde..6886db2d9e00 100644
> --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> @@ -30,16 +30,13 @@ static void drm_aux_hpd_bridge_release(struct device *dev)
> kfree(adev);
> }
>
> -static void drm_aux_hpd_bridge_unregister_adev(void *_adev)
> +static void drm_aux_hpd_bridge_free_adev(void *_adev)
> {
> - struct auxiliary_device *adev = _adev;
> -
> - auxiliary_device_delete(adev);
> - auxiliary_device_uninit(adev);
> + auxiliary_device_uninit(_adev);
> }
>
> /**
> - * drm_dp_hpd_bridge_register - Create a simple HPD DisplayPort bridge
> + * devm_drm_dp_hpd_bridge_alloc - allocate a HPD DisplayPort bridge
> * @parent: device instance providing this bridge
> * @np: device node pointer corresponding to this bridge instance
> *
> @@ -47,11 +44,9 @@ static void drm_aux_hpd_bridge_unregister_adev(void *_adev)
> * DRM_MODE_CONNECTOR_DisplayPort, which terminates the bridge chain and is
> * able to send the HPD events.
> *
> - * Return: device instance that will handle created bridge or an error code
> - * encoded into the pointer.
> + * Return: bridge auxiliary device pointer or an error pointer
> */
> -struct device *drm_dp_hpd_bridge_register(struct device *parent,
> - struct device_node *np)
> +struct auxiliary_device *devm_drm_dp_hpd_bridge_alloc(struct device *parent, struct device_node *np)
> {
> struct auxiliary_device *adev;
> int ret;
> @@ -82,13 +77,55 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent,
> return ERR_PTR(ret);
> }
>
> - ret = auxiliary_device_add(adev);
> - if (ret) {
> - auxiliary_device_uninit(adev);
> + ret = devm_add_action_or_reset(parent, drm_aux_hpd_bridge_free_adev, adev);
> + if (ret)
> return ERR_PTR(ret);
> - }
>
> - ret = devm_add_action_or_reset(parent, drm_aux_hpd_bridge_unregister_adev, adev);
> + return adev;
> +}
> +EXPORT_SYMBOL_GPL(devm_drm_dp_hpd_bridge_alloc);
> +
> +static void drm_aux_hpd_bridge_del_adev(void *_adev)
> +{
> + auxiliary_device_delete(_adev);
> +}
> +
> +/**
> + * devm_drm_dp_hpd_bridge_add - register a HDP DisplayPort bridge
> + * @dev: struct device to tie registration lifetime to
> + * @adev: bridge auxiliary device to be registered
> + *
> + * Returns: zero on success or a negative errno
> + */
> +int devm_drm_dp_hpd_bridge_add(struct device *dev, struct auxiliary_device *adev)
> +{
> + int ret;
> +
> + ret = auxiliary_device_add(adev);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dev, drm_aux_hpd_bridge_del_adev, adev);
> +}
> +EXPORT_SYMBOL_GPL(devm_drm_dp_hpd_bridge_add);
> +
> +/**
> + * drm_dp_hpd_bridge_register - allocate and register a HDP DisplayPort bridge
> + * @parent: device instance providing this bridge
> + * @np: device node pointer corresponding to this bridge instance
> + *
> + * Return: device instance that will handle created bridge or an error pointer
> + */
> +struct device *drm_dp_hpd_bridge_register(struct device *parent, struct device_node *np)
> +{
> + struct auxiliary_device *adev;
> + int ret;
> +
> + adev = devm_drm_dp_hpd_bridge_alloc(parent, np);
> + if (IS_ERR(adev))
> + return ERR_CAST(adev);
> +
> + ret = devm_drm_dp_hpd_bridge_add(parent, adev);
> if (ret)
> return ERR_PTR(ret);
>
> diff --git a/include/drm/bridge/aux-bridge.h b/include/drm/bridge/aux-bridge.h
> index c4c423e97f06..4453906105ca 100644
> --- a/include/drm/bridge/aux-bridge.h
> +++ b/include/drm/bridge/aux-bridge.h
> @@ -9,6 +9,8 @@
>
> #include <drm/drm_connector.h>
>
> +struct auxiliary_device;
> +
> #if IS_ENABLED(CONFIG_DRM_AUX_BRIDGE)
> int drm_aux_bridge_register(struct device *parent);
> #else
> @@ -19,10 +21,23 @@ static inline int drm_aux_bridge_register(struct device *parent)
> #endif
>
> #if IS_ENABLED(CONFIG_DRM_AUX_HPD_BRIDGE)
> +struct auxiliary_device *devm_drm_dp_hpd_bridge_alloc(struct device *parent, struct device_node *np);
> +int devm_drm_dp_hpd_bridge_add(struct device *dev, struct auxiliary_device *adev);
I had a pretty close idea during prototyping, but I ended up doing it
as a single function for the following reasons:
First, this exports the implementation detail that internally the code
uses an aux device.
Also, by exporting the aux device the code becomes less type-safe. By
mistake one can call devm_drm_dp_hpd_bridge_add() on any aux device,
which is not necessarily the HPD bridge.
I'd prefer to see an opaque device-specific structure instead. WDYT?
> struct device *drm_dp_hpd_bridge_register(struct device *parent,
> struct device_node *np);
> void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status status);
> #else
> +static inline struct auxiliary_device *devm_drm_dp_hpd_bridge_alloc(struct device *parent,
> + struct device_node *np)
> +{
> + return NULL;
> +}
> +
> +static inline int devm_drm_dp_hpd_bridge_add(struct auxiliary_device *adev)
> +{
> + return 0;
> +}
> +
> static inline struct device *drm_dp_hpd_bridge_register(struct device *parent,
> struct device_node *np)
> {
> --
> 2.43.0
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists