LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] drivers:of:property.c: fix a potential double put (release) bug
@ 2021-08-18 13:09 Wentao_Liang
  2021-08-18 14:07 ` Rob Herring
  0 siblings, 1 reply; 4+ messages in thread
From: Wentao_Liang @ 2021-08-18 13:09 UTC (permalink / raw)
  To: robh+dt; +Cc: frowand.list, devicetree, linux-kernel, Wentao_Liang

In line 1423 (#1), of_link_to_phandle() is called. In the function
(line 1140, #2), "of_node_put(sup_np);" drops the reference to phandle
and may cause phandle to be released. However, after the function
returns, the phandle is subsequently dropped again (line 1424, #3) by
the same put function. Double putting the phandle can lead to an
incorrect reference count.

We believe that the first put of the phandle is unnecessary (#3). We
can fix the above bug by removing the redundant "of_node_put()" in line
1423.

1401 static int of_link_property(struct device_node *con_np,
				const char *prop_name)
1402 {
...
1409     while (!matched && s->parse_prop) {
...
1414
1415         while ((phandle = s->parse_prop(con_np, prop_name, i))) {
...
                 //#1 phandle is dropped in this function
1423             of_link_to_phandle(con_dev_np, phandle);

1424             //#3 the second drop to phandle
		 of_node_put(phandle);

1425             of_node_put(con_dev_np);
1426         }
...
1428     }
1429     return 0;
1430 }

1095 static int of_link_to_phandle(struct device_node *con_np,
1096                   struct device_node *sup_np)
1097 {
1098     struct device *sup_dev;
1099     struct device_node *tmp_np = sup_np;
...
1140     of_node_put(sup_np);  //#2 the first drop to phandle
			       //   (unnecessary)
1141
1142     return 0;
1143 }

Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
---
 drivers/of/property.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 6c028632f425..408fdde1a20c 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1137,7 +1137,6 @@ static int of_link_to_phandle(struct device_node *con_np,
 	put_device(sup_dev);
 
 	fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
-	of_node_put(sup_np);
 
 	return 0;
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drivers:of:property.c: fix a potential double put (release) bug
  2021-08-18 13:09 [PATCH] drivers:of:property.c: fix a potential double put (release) bug Wentao_Liang
@ 2021-08-18 14:07 ` Rob Herring
  2021-08-19  1:11   ` Saravana Kannan
  0 siblings, 1 reply; 4+ messages in thread
From: Rob Herring @ 2021-08-18 14:07 UTC (permalink / raw)
  To: Wentao_Liang, Saravana Kannan; +Cc: Frank Rowand, devicetree, linux-kernel

+Saravana

On Wed, Aug 18, 2021 at 8:26 AM Wentao_Liang <Wentao_Liang_g@163.com> wrote:
>
> In line 1423 (#1), of_link_to_phandle() is called. In the function
> (line 1140, #2), "of_node_put(sup_np);" drops the reference to phandle
> and may cause phandle to be released. However, after the function
> returns, the phandle is subsequently dropped again (line 1424, #3) by
> the same put function. Double putting the phandle can lead to an
> incorrect reference count.
>
> We believe that the first put of the phandle is unnecessary (#3). We
> can fix the above bug by removing the redundant "of_node_put()" in line
> 1423.
>
> 1401 static int of_link_property(struct device_node *con_np,
>                                 const char *prop_name)
> 1402 {
> ...
> 1409     while (!matched && s->parse_prop) {
> ...
> 1414
> 1415         while ((phandle = s->parse_prop(con_np, prop_name, i))) {
> ...
>                  //#1 phandle is dropped in this function
> 1423             of_link_to_phandle(con_dev_np, phandle);
>
> 1424             //#3 the second drop to phandle
>                  of_node_put(phandle);
>
> 1425             of_node_put(con_dev_np);
> 1426         }
> ...
> 1428     }
> 1429     return 0;
> 1430 }
>
> 1095 static int of_link_to_phandle(struct device_node *con_np,
> 1096                   struct device_node *sup_np)
> 1097 {
> 1098     struct device *sup_dev;
> 1099     struct device_node *tmp_np = sup_np;
> ...
> 1140     of_node_put(sup_np);  //#2 the first drop to phandle
>                                //   (unnecessary)
> 1141
> 1142     return 0;
> 1143 }
>
> Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
> ---
>  drivers/of/property.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 6c028632f425..408fdde1a20c 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1137,7 +1137,6 @@ static int of_link_to_phandle(struct device_node *con_np,
>         put_device(sup_dev);
>
>         fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> -       of_node_put(sup_np);
>
>         return 0;
>  }
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drivers:of:property.c: fix a potential double put (release) bug
  2021-08-18 14:07 ` Rob Herring
@ 2021-08-19  1:11   ` Saravana Kannan
  2021-08-19  1:20     ` Saravana Kannan
  0 siblings, 1 reply; 4+ messages in thread
From: Saravana Kannan @ 2021-08-19  1:11 UTC (permalink / raw)
  To: Rob Herring; +Cc: Wentao_Liang, Frank Rowand, devicetree, linux-kernel

On Wed, Aug 18, 2021 at 7:07 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> +Saravana
>
> On Wed, Aug 18, 2021 at 8:26 AM Wentao_Liang <Wentao_Liang_g@163.com> wrote:
> >
> > In line 1423 (#1), of_link_to_phandle() is called. In the function
> > (line 1140, #2), "of_node_put(sup_np);" drops the reference to phandle
> > and may cause phandle to be released. However, after the function
> > returns, the phandle is subsequently dropped again (line 1424, #3) by
> > the same put function. Double putting the phandle can lead to an
> > incorrect reference count.
> >
> > We believe that the first put of the phandle is unnecessary (#3). We
> > can fix the above bug by removing the redundant "of_node_put()" in line
> > 1423.
> >
> > 1401 static int of_link_property(struct device_node *con_np,
> >                                 const char *prop_name)
> > 1402 {
> > ...
> > 1409     while (!matched && s->parse_prop) {
> > ...
> > 1414
> > 1415         while ((phandle = s->parse_prop(con_np, prop_name, i))) {
> > ...
> >                  //#1 phandle is dropped in this function
> > 1423             of_link_to_phandle(con_dev_np, phandle);
> >
> > 1424             //#3 the second drop to phandle
> >                  of_node_put(phandle);
> >
> > 1425             of_node_put(con_dev_np);
> > 1426         }
> > ...
> > 1428     }
> > 1429     return 0;
> > 1430 }
> >
> > 1095 static int of_link_to_phandle(struct device_node *con_np,
> > 1096                   struct device_node *sup_np)
> > 1097 {
> > 1098     struct device *sup_dev;
> > 1099     struct device_node *tmp_np = sup_np;
> > ...
> > 1140     of_node_put(sup_np);  //#2 the first drop to phandle
> >                                //   (unnecessary)
> > 1141
> > 1142     return 0;
> > 1143 }
> >
> > Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
> > ---
> >  drivers/of/property.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 6c028632f425..408fdde1a20c 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1137,7 +1137,6 @@ static int of_link_to_phandle(struct device_node *con_np,
> >         put_device(sup_dev);
> >
> >         fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> > -       of_node_put(sup_np);

Hi Wentao,

Thanks for noticing and reporting the bug! Your analysis is correct,
but the fix is definitely wrong. For one, the reference to the node
phandle is pointing to can be dropped in of_link_to_phandle() when it
calls of_get_compat_node(). It could also be dropped in one of the
error paths. So, now you'll be incorrectly dropping the reference for
the wrong node. Let me send out a fix and  mention you as the
reporter.

Thanks,
Saravana

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drivers:of:property.c: fix a potential double put (release) bug
  2021-08-19  1:11   ` Saravana Kannan
@ 2021-08-19  1:20     ` Saravana Kannan
  0 siblings, 0 replies; 4+ messages in thread
From: Saravana Kannan @ 2021-08-19  1:20 UTC (permalink / raw)
  To: Rob Herring; +Cc: Wentao_Liang, Frank Rowand, devicetree, linux-kernel

On Wed, Aug 18, 2021 at 6:11 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Aug 18, 2021 at 7:07 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > +Saravana
> >
> > On Wed, Aug 18, 2021 at 8:26 AM Wentao_Liang <Wentao_Liang_g@163.com> wrote:
> > >
> > > In line 1423 (#1), of_link_to_phandle() is called. In the function
> > > (line 1140, #2), "of_node_put(sup_np);" drops the reference to phandle
> > > and may cause phandle to be released. However, after the function
> > > returns, the phandle is subsequently dropped again (line 1424, #3) by
> > > the same put function. Double putting the phandle can lead to an
> > > incorrect reference count.
> > >
> > > We believe that the first put of the phandle is unnecessary (#3). We
> > > can fix the above bug by removing the redundant "of_node_put()" in line
> > > 1423.
> > >
> > > 1401 static int of_link_property(struct device_node *con_np,
> > >                                 const char *prop_name)
> > > 1402 {
> > > ...
> > > 1409     while (!matched && s->parse_prop) {
> > > ...
> > > 1414
> > > 1415         while ((phandle = s->parse_prop(con_np, prop_name, i))) {
> > > ...
> > >                  //#1 phandle is dropped in this function
> > > 1423             of_link_to_phandle(con_dev_np, phandle);
> > >
> > > 1424             //#3 the second drop to phandle
> > >                  of_node_put(phandle);
> > >
> > > 1425             of_node_put(con_dev_np);
> > > 1426         }
> > > ...
> > > 1428     }
> > > 1429     return 0;
> > > 1430 }
> > >
> > > 1095 static int of_link_to_phandle(struct device_node *con_np,
> > > 1096                   struct device_node *sup_np)
> > > 1097 {
> > > 1098     struct device *sup_dev;
> > > 1099     struct device_node *tmp_np = sup_np;
> > > ...
> > > 1140     of_node_put(sup_np);  //#2 the first drop to phandle
> > >                                //   (unnecessary)
> > > 1141
> > > 1142     return 0;
> > > 1143 }
> > >
> > > Signed-off-by: Wentao_Liang <Wentao_Liang_g@163.com>
> > > ---
> > >  drivers/of/property.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index 6c028632f425..408fdde1a20c 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -1137,7 +1137,6 @@ static int of_link_to_phandle(struct device_node *con_np,
> > >         put_device(sup_dev);
> > >
> > >         fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> > > -       of_node_put(sup_np);
>
> Hi Wentao,
>
> Thanks for noticing and reporting the bug! Your analysis is correct,
> but the fix is definitely wrong. For one, the reference to the node
> phandle is pointing to can be dropped in of_link_to_phandle() when it
> calls of_get_compat_node(). It could also be dropped in one of the
> error paths. So, now you'll be incorrectly dropping the reference for
> the wrong node. Let me send out a fix and  mention you as the
> reporter.
>

I spoke too soon. I think there is no refcount problem because
of_link_to_phandle() makes sure it doesn't change the ref count of any
of the DT nodes passed in as input. If you see of_get_compat_node(),
you'll notice that it does a of_node_get() first. So it returns a node
pointer (that could be the same as the input) and it makes sure it
increments that refcount for the node it's returning. And since we are
doing:

sup_np = of_get_compat_node(sup_np);

We are ensuring that by the time of_link_phandle() returns, we haven't
changed the refcount of any of the nodes.

So, I don't think there's any bug here.

-Saravana

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-08-19  1:20 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 13:09 [PATCH] drivers:of:property.c: fix a potential double put (release) bug Wentao_Liang
2021-08-18 14:07 ` Rob Herring
2021-08-19  1:11   ` Saravana Kannan
2021-08-19  1:20     ` Saravana Kannan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).