| Message ID | 20240217150228.5788-2-johan+linaro@kernel.org |
|---|---|
| State | Accepted |
| Commit | 9ee485bdda68d6d3f5728cbe3150eb9013d7d22b |
| Headers | show |
| Series | soc: qcom: pmic_glink_altmode: fix drm bridge use-after-free | expand |
> The two device node references taken during allocation need to be > dropped when the auxiliary device is freed. … > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c … > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > ret = auxiliary_device_init(adev); > if (ret) { > + of_node_put(adev->dev.platform_data); > + of_node_put(adev->dev.of_node); > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > kfree(adev); > return ERR_PTR(ret); The last two statements are also used in a previous if branch. https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/gpu/drm/bridge/aux-hpd-bridge.c#L63 How do you think about to avoid such a bit of duplicate source code by adding a label here? Regards, Markus
On Mon, Feb 19, 2024 at 06:48:30PM +0100, Markus Elfring wrote: > > The two device node references taken during allocation need to be > > dropped when the auxiliary device is freed. > … > > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > … > > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > > > ret = auxiliary_device_init(adev); > > if (ret) { > > + of_node_put(adev->dev.platform_data); > > + of_node_put(adev->dev.of_node); > > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > kfree(adev); > > return ERR_PTR(ret); > > The last two statements are also used in a previous if branch. > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/gpu/drm/bridge/aux-hpd-bridge.c#L63 > > How do you think about to avoid such a bit of duplicate source code > by adding a label here? No, the current code is fine and what you are suggesting is in any case unrelated to this fix. If this function ever grows a third error path like that, I too would consider it however. Johan
On Tue, 20 Feb 2024, Johan Hovold wrote: > On Mon, Feb 19, 2024 at 06:48:30PM +0100, Markus Elfring wrote: > > > The two device node references taken during allocation need to be > > > dropped when the auxiliary device is freed. > > … > > > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > > … > > > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > > > > > ret = auxiliary_device_init(adev); > > > if (ret) { > > > + of_node_put(adev->dev.platform_data); > > > + of_node_put(adev->dev.of_node); > > > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > > kfree(adev); > > > return ERR_PTR(ret); > > > > The last two statements are also used in a previous if branch. > > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/gpu/drm/bridge/aux-hpd-bridge.c#L63 > > > > How do you think about to avoid such a bit of duplicate source code > > by adding a label here? > > No, the current code is fine and what you are suggesting is in any case > unrelated to this fix. > > If this function ever grows a third error path like that, I too would > consider it however. I guess these of_node_puts can all go away shortly with cleanup anyway? julia
On Tue, 20 Feb 2024 at 13:52, Julia Lawall <julia.lawall@inria.fr> wrote: > > > > On Tue, 20 Feb 2024, Johan Hovold wrote: > > > On Mon, Feb 19, 2024 at 06:48:30PM +0100, Markus Elfring wrote: > > > > The two device node references taken during allocation need to be > > > > dropped when the auxiliary device is freed. > > > … > > > > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > > > … > > > > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > > > > > > > ret = auxiliary_device_init(adev); > > > > if (ret) { > > > > + of_node_put(adev->dev.platform_data); > > > > + of_node_put(adev->dev.of_node); > > > > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > > > kfree(adev); > > > > return ERR_PTR(ret); > > > > > > The last two statements are also used in a previous if branch. > > > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/gpu/drm/bridge/aux-hpd-bridge.c#L63 > > > > > > How do you think about to avoid such a bit of duplicate source code > > > by adding a label here? > > > > No, the current code is fine and what you are suggesting is in any case > > unrelated to this fix. > > > > If this function ever grows a third error path like that, I too would > > consider it however. > > I guess these of_node_puts can all go away shortly with cleanup anyway? I'm not sure about it. Those are long-living variables, so they are not a subject of cleanup.h, are they?
On Tue, 20 Feb 2024, Dmitry Baryshkov wrote: > On Tue, 20 Feb 2024 at 13:52, Julia Lawall <julia.lawall@inria.fr> wrote: > > > > > > > > On Tue, 20 Feb 2024, Johan Hovold wrote: > > > > > On Mon, Feb 19, 2024 at 06:48:30PM +0100, Markus Elfring wrote: > > > > > The two device node references taken during allocation need to be > > > > > dropped when the auxiliary device is freed. > > > > … > > > > > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > > > > … > > > > > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > > > > > > > > > ret = auxiliary_device_init(adev); > > > > > if (ret) { > > > > > + of_node_put(adev->dev.platform_data); > > > > > + of_node_put(adev->dev.of_node); > > > > > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > > > > kfree(adev); > > > > > return ERR_PTR(ret); > > > > > > > > The last two statements are also used in a previous if branch. > > > > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/gpu/drm/bridge/aux-hpd-bridge.c#L63 > > > > > > > > How do you think about to avoid such a bit of duplicate source code > > > > by adding a label here? > > > > > > No, the current code is fine and what you are suggesting is in any case > > > unrelated to this fix. > > > > > > If this function ever grows a third error path like that, I too would > > > consider it however. > > > > I guess these of_node_puts can all go away shortly with cleanup anyway? > > I'm not sure about it. Those are long-living variables, so they are > not a subject of cleanup.h, are they? OK, I didn't look at this code in detail, but cleanup would just call of_node_put, not actually free the data. julia
On Tue, 20 Feb 2024 at 14:56, Julia Lawall <julia.lawall@inria.fr> wrote: > > > > On Tue, 20 Feb 2024, Dmitry Baryshkov wrote: > > > On Tue, 20 Feb 2024 at 13:52, Julia Lawall <julia.lawall@inria.fr> wrote: > > > > > > > > > > > > On Tue, 20 Feb 2024, Johan Hovold wrote: > > > > > > > On Mon, Feb 19, 2024 at 06:48:30PM +0100, Markus Elfring wrote: > > > > > > The two device node references taken during allocation need to be > > > > > > dropped when the auxiliary device is freed. > > > > > … > > > > > > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > > > > > … > > > > > > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > > > > > > > > > > > ret = auxiliary_device_init(adev); > > > > > > if (ret) { > > > > > > + of_node_put(adev->dev.platform_data); > > > > > > + of_node_put(adev->dev.of_node); > > > > > > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > > > > > kfree(adev); > > > > > > return ERR_PTR(ret); > > > > > > > > > > The last two statements are also used in a previous if branch. > > > > > https://elixir.bootlin.com/linux/v6.8-rc5/source/drivers/gpu/drm/bridge/aux-hpd-bridge.c#L63 > > > > > > > > > > How do you think about to avoid such a bit of duplicate source code > > > > > by adding a label here? > > > > > > > > No, the current code is fine and what you are suggesting is in any case > > > > unrelated to this fix. > > > > > > > > If this function ever grows a third error path like that, I too would > > > > consider it however. > > > > > > I guess these of_node_puts can all go away shortly with cleanup anyway? > > > > I'm not sure about it. Those are long-living variables, so they are > > not a subject of cleanup.h, are they? > > OK, I didn't look at this code in detail, but cleanup would just call > of_node_put, not actually free the data. Yes. The nodes should be put either in case of the failure or (if everything goes fine) at the device unregistration.
On Sat, Feb 17, 2024 at 04:02:23PM +0100, Johan Hovold wrote: > The two device node references taken during allocation need to be > dropped when the auxiliary device is freed. > > Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers") > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > --- > drivers/gpu/drm/bridge/aux-hpd-bridge.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > index bb55f697a181..9e71daf95bde 100644 > --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > @@ -25,6 +25,7 @@ static void drm_aux_hpd_bridge_release(struct device *dev) > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > of_node_put(adev->dev.platform_data); > + of_node_put(adev->dev.of_node); > > kfree(adev); > } > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > ret = auxiliary_device_init(adev); > if (ret) { > + of_node_put(adev->dev.platform_data); > + of_node_put(adev->dev.of_node); > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > kfree(adev); > return ERR_PTR(ret); > -- > 2.43.0 >
On Sat, 17 Feb 2024 at 17:03, Johan Hovold <johan+linaro@kernel.org> wrote: > > The two device node references taken during allocation need to be > dropped when the auxiliary device is freed. > > Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers") > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/bridge/aux-hpd-bridge.c | 3 +++ > 1 file changed, 3 insertions(+)
On 17/02/2024 16:02, Johan Hovold wrote: > The two device node references taken during allocation need to be > dropped when the auxiliary device is freed. > > Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers") > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/gpu/drm/bridge/aux-hpd-bridge.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > index bb55f697a181..9e71daf95bde 100644 > --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > @@ -25,6 +25,7 @@ static void drm_aux_hpd_bridge_release(struct device *dev) > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > of_node_put(adev->dev.platform_data); > + of_node_put(adev->dev.of_node); > > kfree(adev); > } > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > ret = auxiliary_device_init(adev); > if (ret) { > + of_node_put(adev->dev.platform_data); > + of_node_put(adev->dev.of_node); > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > kfree(adev); > return ERR_PTR(ret); Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
On 17/02/2024 16:02, Johan Hovold wrote: > The two device node references taken during allocation need to be > dropped when the auxiliary device is freed. > > Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers") > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/gpu/drm/bridge/aux-hpd-bridge.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > index bb55f697a181..9e71daf95bde 100644 > --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c > +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c > @@ -25,6 +25,7 @@ static void drm_aux_hpd_bridge_release(struct device *dev) > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > > of_node_put(adev->dev.platform_data); > + of_node_put(adev->dev.of_node); > > kfree(adev); > } > @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, > > ret = auxiliary_device_init(adev); > if (ret) { > + of_node_put(adev->dev.platform_data); > + of_node_put(adev->dev.of_node); > ida_free(&drm_aux_hpd_bridge_ida, adev->id); > kfree(adev); > return ERR_PTR(ret); Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c index bb55f697a181..9e71daf95bde 100644 --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c @@ -25,6 +25,7 @@ static void drm_aux_hpd_bridge_release(struct device *dev) ida_free(&drm_aux_hpd_bridge_ida, adev->id); of_node_put(adev->dev.platform_data); + of_node_put(adev->dev.of_node); kfree(adev); } @@ -74,6 +75,8 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent, ret = auxiliary_device_init(adev); if (ret) { + of_node_put(adev->dev.platform_data); + of_node_put(adev->dev.of_node); ida_free(&drm_aux_hpd_bridge_ida, adev->id); kfree(adev); return ERR_PTR(ret);
The two device node references taken during allocation need to be dropped when the auxiliary device is freed. Fixes: 6914968a0b52 ("drm/bridge: properly refcount DT nodes in aux bridge drivers") Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: Neil Armstrong <neil.armstrong@linaro.org> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/gpu/drm/bridge/aux-hpd-bridge.c | 3 +++ 1 file changed, 3 insertions(+)