[v1] soc: qcom: pmic_glink_altmode: fix drm bridge use-after-free

[PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Johan Hovold 1 year, 11 months ago

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);
-- 
2.43.0

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Neil Armstrong 1 year, 11 months ago

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>

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Neil Armstrong 1 year, 11 months ago

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>

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Dmitry Baryshkov 1 year, 11 months ago

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(+)


-- 
With best wishes
Dmitry

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Bjorn Andersson 1 year, 11 months ago

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
>

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Markus Elfring 1 year, 11 months ago

> 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

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Johan Hovold 1 year, 11 months ago

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

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Julia Lawall 1 year, 11 months ago


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

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Dmitry Baryshkov 1 year, 11 months ago

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?


-- 
With best wishes
Dmitry

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Julia Lawall 1 year, 11 months ago


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

Re: [PATCH 1/6] drm/bridge: aux-hpd: fix OF node leaks

Posted by Dmitry Baryshkov 1 year, 11 months ago

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.

-- 
With best wishes
Dmitry