irqchip/gic-v3: fix ppi-partitions lookup
PATCH 1/1 irqchip/gic-v3: fix ppi-partitions lookup
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.
--- drivers/irqchip/irq-gic-v3.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/irqchip/irq-gic-v3.c +6 -3
@@ -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)
Signature · 1 line
2.15.0
On Sat, 11 Nov 2017 17:51:25 +0100
Johan Hovold <[email protected]> wrote:
Johan,
Quoted 5 lines
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...
Quoted 4 lines
the ppi-partitions node was leaked.
Fixes e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs")Cc stable <[email protected]> # 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.
Quoted 42 lines
Cc Marc Zyngier <[email protected]>Signed-off-by Johan Hovold <[email protected]>--- drivers/irqchip/irq-gic-v3.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/irqchip/irq-gic-v3.c +6
-3@@ -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)
Signature · 1 line
Without deviation from the norm, progress is not possible.
Quoted 9 lines · Marc Zyngier
On Sat, 11 Nov 2017 17:51:25 +0100
Johan Hovold <[email protected]> wrote:Johan,
Quoted 4 lines
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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 4 lines
the ppi-partitions node was leaked.
Fixes e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs")Cc stable <[email protected]> # 4.7Do you have an example of this causing any trouble in the wild? As far as I remember, the whole
of_noderefcounting 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.
Quoted 1 line
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
Quoted 12 lines · On Sun, 12 Nov 2017 14:47:59 +0100 Johan Hovold <[email protected]> wrote:
Quoted 11 lines · Marc Zyngier
On Sat, 11 Nov 2017 17:51:25 +0100
Johan Hovold <[email protected]> wrote:Johan,
Quoted 5 lines
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
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
the ppi-partitions node was leaked.
Fixes e3825ba1af3a ("irqchip/gic-v3: Add support for partitioned PPIs")Cc stable <[email protected]> # 4.7
Quoted 7 lines
Quoted 3 lines
Do you have an example of this causing any trouble in the wild? As far as I remember, the whole
of_noderefcounting 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.
Signature · 1 line
Without deviation from the norm, progress is not possible.