linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] irqchip/gic-v3: fix ppi-partitions lookup
@ 2017-11-11 16:51 Johan Hovold
  2017-11-12 12:32 ` Marc Zyngier
  0 siblings, 1 reply; 4+ messages in thread
From: Johan Hovold @ 2017-11-11 16:51 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Jason Cooper, linux-kernel, Johan Hovold, stable, Marc Zyngier

Fix child-node lookup during initialisation, which ended up searching
the whole device tree depth-first starting at the parent rather than
just matching on its children.

To make things worse, the parent giq node was prematurely freed, while
the ppi-partitions node was leaked.

Fixes: e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs")
Cc: stable <stable@vger.kernel.org>     # 4.7
Cc: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/irqchip/irq-gic-v3.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 17221143f505..af1f8373d8bf 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1103,18 +1103,18 @@ static void __init gic_populate_ppi_partitions(struct device_node *gic_node)
 	int nr_parts;
 	struct partition_affinity *parts;
 
-	parts_node = of_find_node_by_name(gic_node, "ppi-partitions");
+	parts_node = of_get_child_by_name(gic_node, "ppi-partitions");
 	if (!parts_node)
 		return;
 
 	nr_parts = of_get_child_count(parts_node);
 
 	if (!nr_parts)
-		return;
+		goto out_put_node;
 
 	parts = kzalloc(sizeof(*parts) * nr_parts, GFP_KERNEL);
 	if (WARN_ON(!parts))
-		return;
+		goto out_put_node;
 
 	for_each_child_of_node(parts_node, child_part) {
 		struct partition_affinity *part;
@@ -1181,6 +1181,9 @@ static void __init gic_populate_ppi_partitions(struct device_node *gic_node)
 
 		gic_data.ppi_descs[i] = desc;
 	}
+
+out_put_node:
+	of_node_put(parts_node);
 }
 
 static void __init gic_of_setup_kvm_info(struct device_node *node)
-- 
2.15.0

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

* Re: [PATCH] irqchip/gic-v3: fix ppi-partitions lookup
  2017-11-11 16:51 [PATCH] irqchip/gic-v3: fix ppi-partitions lookup Johan Hovold
@ 2017-11-12 12:32 ` Marc Zyngier
  2017-11-12 13:47   ` Johan Hovold
  0 siblings, 1 reply; 4+ messages in thread
From: Marc Zyngier @ 2017-11-12 12:32 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Thomas Gleixner, Jason Cooper, linux-kernel, stable

On Sat, 11 Nov 2017 17:51:25 +0100
Johan Hovold <johan@kernel.org> wrote:

Johan,

> Fix child-node lookup during initialisation, which ended up searching
> the whole device tree depth-first starting at the parent rather than
> just matching on its children.
> 
> To make things worse, the parent giq node was prematurely freed, while

s/giq/gic/.

Care to point out where that node would be prematurely freed? I don't
see your patch addressing that either...

> the ppi-partitions node was leaked.
> 
> Fixes: e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs")
> Cc: stable <stable@vger.kernel.org>     # 4.7

Do you have an example of this causing any trouble in the wild? As far
as I remember, the whole of_node refcounting isn't really enforced, so
while this is definitely a bug, it wouldn't cause any harm anywhere.

Or am I missing something obvious?

Thanks,

	M.

> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 17221143f505..af1f8373d8bf 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1103,18 +1103,18 @@ static void __init gic_populate_ppi_partitions(struct device_node *gic_node)
>  	int nr_parts;
>  	struct partition_affinity *parts;
>  
> -	parts_node = of_find_node_by_name(gic_node, "ppi-partitions");
> +	parts_node = of_get_child_by_name(gic_node, "ppi-partitions");
>  	if (!parts_node)
>  		return;
>  
>  	nr_parts = of_get_child_count(parts_node);
>  
>  	if (!nr_parts)
> -		return;
> +		goto out_put_node;
>  
>  	parts = kzalloc(sizeof(*parts) * nr_parts, GFP_KERNEL);
>  	if (WARN_ON(!parts))
> -		return;
> +		goto out_put_node;
>  
>  	for_each_child_of_node(parts_node, child_part) {
>  		struct partition_affinity *part;
> @@ -1181,6 +1181,9 @@ static void __init gic_populate_ppi_partitions(struct device_node *gic_node)
>  
>  		gic_data.ppi_descs[i] = desc;
>  	}
> +
> +out_put_node:
> +	of_node_put(parts_node);
>  }
>  
>  static void __init gic_of_setup_kvm_info(struct device_node *node)



-- 
Without deviation from the norm, progress is not possible.

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

* Re: [PATCH] irqchip/gic-v3: fix ppi-partitions lookup
  2017-11-12 12:32 ` Marc Zyngier
@ 2017-11-12 13:47   ` Johan Hovold
  2017-11-12 14:28     ` Marc Zyngier
  0 siblings, 1 reply; 4+ messages in thread
From: Johan Hovold @ 2017-11-12 13:47 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Johan Hovold, Thomas Gleixner, Jason Cooper, linux-kernel, stable

On Sun, Nov 12, 2017 at 12:32:08PM +0000, Marc Zyngier wrote:
> On Sat, 11 Nov 2017 17:51:25 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> Johan,
> 
> > Fix child-node lookup during initialisation, which ended up searching
> > the whole device tree depth-first starting at the parent rather than
> > just matching on its children.
> > 
> > To make things worse, the parent giq node was prematurely freed, while
> 
> s/giq/gic/.
> 
> Care to point out where that node would be prematurely freed? I don't
> see your patch addressing that either...

of_find_node_by_name() is used for tree-wide searches and, as
documented, drops a reference to its first argument, which in this case
is the parent gic node.

> > the ppi-partitions node was leaked.
> > 
> > Fixes: e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs")
> > Cc: stable <stable@vger.kernel.org>     # 4.7
> 
> Do you have an example of this causing any trouble in the wild? As far
> as I remember, the whole of_node refcounting isn't really enforced, so
> while this is definitely a bug, it wouldn't cause any harm anywhere.

Node refcounting is enabled with CONFIG_OF_DYNAMIC (e.g. when overlay
support is enabled) and getting the refcounting wrong can lead to all
sorts of issues like use-after-free and crashes.

Using the wrong of-helper this way to lookup child nodes have been
reproduced in several drivers, and I'm trying to fix them all up (and
amend the kernel docs) to prevent this pattern from spreading further.

In general you could end up matching and parsing an unrelated node
with whatever implications that may have for a driver too.

> Or am I missing something obvious?

Feel free to drop the stable tag if you deem the implications for this
particular driver to be benign.

I can't test this one myself, but note that the node refcount is
manipulated also after the unbalanced put (e.g. in
gic_of_setup_kvm_info()).

Thanks,
Johan

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

* Re: [PATCH] irqchip/gic-v3: fix ppi-partitions lookup
  2017-11-12 13:47   ` Johan Hovold
@ 2017-11-12 14:28     ` Marc Zyngier
  0 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2017-11-12 14:28 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Thomas Gleixner, Jason Cooper, linux-kernel, stable

On Sun, 12 Nov 2017 14:47:59 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Sun, Nov 12, 2017 at 12:32:08PM +0000, Marc Zyngier wrote:
> > On Sat, 11 Nov 2017 17:51:25 +0100
> > Johan Hovold <johan@kernel.org> wrote:
> > 
> > Johan,
> >   
> > > Fix child-node lookup during initialisation, which ended up searching
> > > the whole device tree depth-first starting at the parent rather than
> > > just matching on its children.
> > > 
> > > To make things worse, the parent giq node was prematurely freed, while  
> > 
> > s/giq/gic/.
> > 
> > Care to point out where that node would be prematurely freed? I don't
> > see your patch addressing that either...  
> 
> of_find_node_by_name() is used for tree-wide searches and, as
> documented, drops a reference to its first argument, which in this case
> is the parent gic node.

Got it. Yes, that's definitely a bad idea.

> 
> > > the ppi-partitions node was leaked.
> > > 
> > > Fixes: e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs")
> > > Cc: stable <stable@vger.kernel.org>     # 4.7  
> > 
> > Do you have an example of this causing any trouble in the wild? As far
> > as I remember, the whole of_node refcounting isn't really enforced, so
> > while this is definitely a bug, it wouldn't cause any harm anywhere.  
> 
> Node refcounting is enabled with CONFIG_OF_DYNAMIC (e.g. when overlay
> support is enabled) and getting the refcounting wrong can lead to all
> sorts of issues like use-after-free and crashes.

Ah, I completely forgot about this overlay madness. Fair enough, that's
tricky enough to spot that it is worth plugging ASAP.

I've queued this with a handful of other fixes for 4.15.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

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

end of thread, other threads:[~2017-11-12 14:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-11 16:51 [PATCH] irqchip/gic-v3: fix ppi-partitions lookup Johan Hovold
2017-11-12 12:32 ` Marc Zyngier
2017-11-12 13:47   ` Johan Hovold
2017-11-12 14:28     ` Marc Zyngier

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).