Fix NULL ptr dereference in pci_bus_assign_domain_nr() on ARM

PATCH Applied Krzysztof Hałasa 3 participants 9 messages Mar 1, 2016 – Mar 8, 2016 PCI

Share & tools

Krzysztof Hałasa

author

2016-03-01 06:07 UTC #0 lore raw

PATCH 1/1 Fix NULL ptr dereference in pci_bus_assign_domain_nr() on ARM

Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)

{
        pci_common_init_dev(NULL, hw);
}

which means that pci_bus_assign_domain_nr() can be called without a parent. This patch fixes the NULL pointer dereference.

Signed-off-by Krzysztof Ha?asa <[email protected]>

Cc stable at vger.kernel.org

drivers/pci/pci.c +3 -1

index 602eb42..f89db3a 100644
@@ -4772,8 +4772,10 @@ int pci_get_new_domain_nr(void)
 void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 {
 	static int use_dt_domains = -1;
-	int domain = of_get_pci_domain_nr(parent->of_node);
+	int domain = -1;
 
+	if (parent)
+		domain = of_get_pci_domain_nr(parent->of_node);
 	/*
 	 * Check DT domain and use_dt_domains values.
 	 *

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Bjorn Helgaas

2016-03-03 17:35 UTC #1 lore raw

Hi Krzysztof,

Quoted 6 lines · Krzysztof Ha?asa

Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)

Quoted 6 lines

{
        pci_common_init_dev(NULL, hw);
}
which means that pci_bus_assign_domain_nr() can be called without a parent. This patch fixes the NULL pointer dereference.

What exactly is the impact of this? Does this fix need to be in v4.5? It sounds like it should be, but I need a little more detailed justification, e.g., "platforms X, Y, Z don't boot at all without this change."

Quoted 25 lines

Signed-off-by Krzysztof Ha?asa <[email protected]>

Cc stable at vger.kernel.org

drivers/pci/pci.c +3 -1

index 602eb42..f89db3a 100644
@@ -4772,8 +4772,10 @@ int pci_get_new_domain_nr(void)
 void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 {
 	static int use_dt_domains = -1;
-	int domain = of_get_pci_domain_nr(parent->of_node);
+	int domain = -1;
 
+	if (parent)
+		domain = of_get_pci_domain_nr(parent->of_node);
 	/*
 	 * Check DT domain and use_dt_domains values.
 	 *

Signature · 5 lines

-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Krzysztof Hałasa

author

2016-03-04 06:13 UTC #2 lore raw

Hi Bjorn,

Quoted 11 lines · Bjorn Helgaas

Quoted 10 lines · Krzysztof Ha?asa
Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)
{
        pci_common_init_dev(NULL, hw);
}

Quoted 7 lines

Quoted 2 lines
which means that pci_bus_assign_domain_nr() can be called without a parent. This patch fixes the NULL pointer dereference.
What exactly is the impact of this? Does this fix need to be in v4.5? It sounds like it should be, but I need a little more detailed justification, e.g., "platforms X, Y, Z don't boot at all without this change."

At least CNS3xxx doesn't boot. I haven't verified a couple of others, but they may be broken as well.

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Bjorn Helgaas

2016-03-04 16:27 UTC #3 lore raw

Quoted 18 lines · Krzysztof Ha?asa

Hi Bjorn,
Quoted 14 lines · Bjorn Helgaas
Quoted 12 lines · Krzysztof Ha?asa
Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)
{
        pci_common_init_dev(NULL, hw);
}
which means that pci_bus_assign_domain_nr() can be called without a parent. This patch fixes the NULL pointer dereference.

Quoted 7 lines

Quoted 4 lines
What exactly is the impact of this? Does this fix need to be in v4.5? It sounds like it should be, but I need a little more detailed justification, e.g., "platforms X, Y, Z don't boot at all without this change."
At least CNS3xxx doesn't boot. I haven't verified a couple of others, but they may be broken as well.

Good, thanks. Also (I should have asked this before), please include a "Fixes:" line so we know exactly when this broke and what stable kernels need the fix.

Bjorn

Krzysztof Hałasa

author

2016-03-07 13:54 UTC #4 lore raw

Quoted 6 lines · Bjorn Helgaas

Quoted 2 lines
At least CNS3xxx doesn't boot. I haven't verified a couple of others, but they may be broken as well.
Good, thanks. Also (I should have asked this before), please include a "Fixes:" line so we know exactly when this broke and what stable kernels need the fix.

The problem started here:

Fixes 8c7d14746abc ("ARM/PCI: Move to generic PCI domains")

It means the patch should be applicable starting with v4.0, and ... indeed v4.0 with the patch boots correctly, while v4.0 without the patch does not.

Thanks.

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Bjorn Helgaas

Applied

2016-03-07 22:33 UTC #5 lore raw

In reply to Krzysztof Hałasa

[+cc Lorenzo]

Quoted 10 lines · Krzysztof Ha?asa

Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)
{
        pci_common_init_dev(NULL, hw);
}

Quoted 5 lines

which means that pci_bus_assign_domain_nr() can be called without a parent. This patch fixes the NULL pointer dereference.
Signed-off-by Krzysztof Ha?asa <[email protected]>
Cc stable at vger.kernel.org

I applied this to for-linus with changelog as below for v4.5, thanks!

Wow, this is terrible. All ARM32 systems that use pci_common_init() crash at boot. That includes cns3xxx, dove, footbridge, iopl13xx, ip32x, iop33x, ixp4xx, ks8695, mv78xx0, orion5x, pxa, sa1100, etc. Apparently they've been crashing since v4.0, when 7c674700098c and 8c7d14746abc appeared. I can hardly believe nobody noticed until now.

Actually, I did find one problem report:
http://forum.doozan.com/read.php?2,17868,22070,quote=1 from last May, but apparently it got lost in a forum and never found its way upstream.

I reworked the changelog because this problem will affect *any* arch that enables CONFIG_PCI_DOMAINS_GENERIC and supplies NULL "parent" pointers -- ia64, mips, mn10300, s390, x86, etc., would be affected if they enabled CONFIG_PCI_DOMAINS_GENERIC.

I also added a "Fixes:" tag for 7c674700098c, since that's the commit that added the generic code we're fixing. Backports of 7c674700098c should also backport this change.

Bjorn

commit 71babd2a89fe

Author: Krzysztof =?utf-8?Q?Ha=C5=82asa?= <[email protected]>
Date:   Tue Mar 1 07:07:18 2016 +0100

PCI: Allow a NULL "parent" pointer in pci_bus_assign_domain_nr()

pci_create_root_bus() passes a "parent" pointer to pci_bus_assign_domain_nr(). When CONFIG_PCI_DOMAINS_GENERIC is defined, pci_bus_assign_domain_nr() dereferences that pointer. Many callers of pci_create_root_bus() supply a NULL "parent" pointer, which leads to a NULL pointer dereference error.

7c674700098c ("PCI: Move domain assignment from arm64 to generic code") moved the "parent" dereference from arm64 to generic code. Only arm64 used that code (because only arm64 defined CONFIG_PCI_DOMAINS_GENERIC), and it always supplied a valid "parent" pointer. Other arches supplied NULL "parent" pointers but didn't defined CONFIG_PCI_DOMAINS_GENERIC, so they used a no-op version of pci_bus_assign_domain_nr().

8c7d14746abc ("ARM/PCI: Move to generic PCI domains") defined CONFIG_PCI_DOMAINS_GENERIC on ARM, and many ARM platforms use pci_common_init(), which supplies a NULL "parent" pointer. These platforms (cns3xxx, dove, footbridge, iop13xx, etc.) crash with a NULL pointer dereference like this while probing PCI:

Unable to handle kernel NULL pointer dereference at virtual address 000000a4 PC is at pci_bus_assign_domain_nr+0x10/0x84
LR is at pci_create_root_bus+0x48/0x2e4
Kernel panic - not syncing: Attempted to kill init!

[bhelgaas: changelog, add "Reported:" and "Fixes:" tags]
Reported: http://forum.doozan.com/read.php?2,17868,22070,quote=1

Fixes 8c7d14746abc ("ARM/PCI: Move to generic PCI domains")

Fixes 7c674700098c ("PCI: Move domain assignment from arm64 to generic code")

Signed-off-by Krzysztof Ha?asa <[email protected]>

Signed-off-by Bjorn Helgaas <[email protected]>

CC stable at vger.kernel.org # v4.0+

drivers/pci/pci.c +3 -1

index 602eb42..f89db3a 100644
@@ -4772,8 +4772,10 @@ int pci_get_new_domain_nr(void)
 void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 {
 	static int use_dt_domains = -1;
-	int domain = of_get_pci_domain_nr(parent->of_node);
+	int domain = -1;
 
+	if (parent)
+		domain = of_get_pci_domain_nr(parent->of_node);
 	/*
 	 * Check DT domain and use_dt_domains values.
 	 *

Lorenzo Pieralisi

Acked-by

2016-03-08 02:59 UTC #6 lore raw

Quoted 32 lines · Bjorn Helgaas

[+cc Lorenzo]
Quoted 10 lines · Krzysztof Ha??asa
Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)
{
        pci_common_init_dev(NULL, hw);
}
Quoted 5 lines
which means that pci_bus_assign_domain_nr() can be called without a parent. This patch fixes the NULL pointer dereference.
Signed-off-by Krzysztof Ha??asa <[email protected]>
Cc [email protected]
I applied this to for-linus with changelog as below for v4.5, thanks!
Wow, this is terrible. All ARM32 systems that use pci_common_init() crash at boot. That includes cns3xxx, dove, footbridge, iopl13xx, ip32x, iop33x, ixp4xx, ks8695, mv78xx0, orion5x, pxa, sa1100, etc. Apparently they've been crashing since v4.0, when 7c674700098c and 8c7d14746abc appeared. I can hardly believe nobody noticed until now.
Actually, I did find one problem report:
http://forum.doozan.com/read.php?2,17868,22070,quote=1 from last May, but apparently it got lost in a forum and never found its way upstream.

Quoted 8 lines

I reworked the changelog because this problem will affect *any* arch that enables CONFIG_PCI_DOMAINS_GENERIC and supplies NULL "parent" pointers -- ia64, mips, mn10300, s390, x86, etc., would be affected if they enabled CONFIG_PCI_DOMAINS_GENERIC.
I also added a "Fixes:" tag for 7c674700098c, since that's the commit that added the generic code we're fixing. Backports of 7c674700098c should also backport this change.

That's really unfortunate, when I moved code from arm64 to generic I did not spot this issue in the original code and carried it over, you summarized the reasons in the commit log so without any further ado (and with my apologies):

Acked-by Lorenzo Pieralisi <[email protected]>

Quoted 60 lines

Bjorn
commit 71babd2a89fe
Author: Krzysztof =?utf-8?Q?Ha=C5=82asa?= <[email protected]>
Date:   Tue Mar 1 07:07:18 2016 +0100
PCI: Allow a NULL "parent" pointer in pci_bus_assign_domain_nr()
pci_create_root_bus() passes a "parent" pointer to pci_bus_assign_domain_nr(). When CONFIG_PCI_DOMAINS_GENERIC is defined, pci_bus_assign_domain_nr() dereferences that pointer. Many callers of pci_create_root_bus() supply a NULL "parent" pointer, which leads to a NULL pointer dereference error.
7c674700098c ("PCI: Move domain assignment from arm64 to generic code") moved the "parent" dereference from arm64 to generic code. Only arm64 used that code (because only arm64 defined CONFIG_PCI_DOMAINS_GENERIC), and it always supplied a valid "parent" pointer. Other arches supplied NULL "parent" pointers but didn't defined CONFIG_PCI_DOMAINS_GENERIC, so they used a no-op version of pci_bus_assign_domain_nr().
8c7d14746abc ("ARM/PCI: Move to generic PCI domains") defined CONFIG_PCI_DOMAINS_GENERIC on ARM, and many ARM platforms use pci_common_init(), which supplies a NULL "parent" pointer. These platforms (cns3xxx, dove, footbridge, iop13xx, etc.) crash with a NULL pointer dereference like this while probing PCI:
Unable to handle kernel NULL pointer dereference at virtual address 000000a4 PC is at pci_bus_assign_domain_nr+0x10/0x84
LR is at pci_create_root_bus+0x48/0x2e4
Kernel panic - not syncing: Attempted to kill init!
[bhelgaas: changelog, add "Reported:" and "Fixes:" tags]
Reported: http://forum.doozan.com/read.php?2,17868,22070,quote=1
Fixes 8c7d14746abc ("ARM/PCI: Move to generic PCI domains")
Fixes 7c674700098c ("PCI: Move domain assignment from arm64 to generic code")
Signed-off-by Krzysztof Ha??asa <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
CC [email protected] # v4.0+
drivers/pci/pci.c +3 -1
index 602eb42..f89db3a 100644
@@ -4772,8 +4772,10 @@ int pci_get_new_domain_nr(void)
 void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 {
 	static int use_dt_domains = -1;
-	int domain = of_get_pci_domain_nr(parent->of_node);
+	int domain = -1;
 
+	if (parent)
+		domain = of_get_pci_domain_nr(parent->of_node);
 	/*
 	 * Check DT domain and use_dt_domains values.
 	 *

Bjorn Helgaas

2016-03-08 04:24 UTC #7 lore raw

Quoted 42 lines · Lorenzo Pieralisi

Quoted 32 lines · Bjorn Helgaas
[+cc Lorenzo]
Quoted 10 lines · Krzysztof Ha??asa
Many ARM platforms use a wrapper:
/*
* Compatibility wrapper for older platforms that do not care about
* passing the parent device.
*/
static inline void pci_common_init(struct hw_pci *hw)
{
        pci_common_init_dev(NULL, hw);
}
Quoted 5 lines
which means that pci_bus_assign_domain_nr() can be called without a parent. This patch fixes the NULL pointer dereference.
Signed-off-by Krzysztof Ha??asa <[email protected]>
Cc stable at vger.kernel.org
I applied this to for-linus with changelog as below for v4.5, thanks!
Wow, this is terrible. All ARM32 systems that use pci_common_init() crash at boot. That includes cns3xxx, dove, footbridge, iopl13xx, ip32x, iop33x, ixp4xx, ks8695, mv78xx0, orion5x, pxa, sa1100, etc. Apparently they've been crashing since v4.0, when 7c674700098c and 8c7d14746abc appeared. I can hardly believe nobody noticed until now.
Actually, I did find one problem report:
http://forum.doozan.com/read.php?2,17868,22070,quote=1 from last May, but apparently it got lost in a forum and never found its way upstream.
Quoted 8 lines
I reworked the changelog because this problem will affect *any* arch that enables CONFIG_PCI_DOMAINS_GENERIC and supplies NULL "parent" pointers -- ia64, mips, mn10300, s390, x86, etc., would be affected if they enabled CONFIG_PCI_DOMAINS_GENERIC.
I also added a "Fixes:" tag for 7c674700098c, since that's the commit that added the generic code we're fixing. Backports of 7c674700098c should also backport this change.

Quoted 6 lines

That's really unfortunate, when I moved code from arm64 to generic I did not spot this issue in the original code and carried it over, you summarized the reasons in the commit log so without any further ado (and with my apologies):
Acked-by Lorenzo Pieralisi <[email protected]>

No worries, it just goes with the territory. What surprises me is that it took us so long to notice. v4.0 was released almost a year ago (April 12, 2015), so I can't figure out how nobody noticed until now.

And I don't know what happened with the problem report in the forum. That's a case where somebody *did* notice, but I guess they just gave up on v4.0 and went back to v3.18. What a shame :) I don't know if people just have low expectations of Linux, or they feel like it's too hard to report bugs, or we don't make it easy enough, or we're not approachable enough, or what. I notice that many times somebody finds a workaround, and people seem satisfied with that, and we don't get a chance to fix the real problem.

Bjorn

Quoted 59 lines

Quoted 45 lines
commit 71babd2a89fe
Author: Krzysztof =?utf-8?Q?Ha=C5=82asa?= <[email protected]>
Date:   Tue Mar 1 07:07:18 2016 +0100
PCI: Allow a NULL "parent" pointer in pci_bus_assign_domain_nr()
pci_create_root_bus() passes a "parent" pointer to pci_bus_assign_domain_nr(). When CONFIG_PCI_DOMAINS_GENERIC is defined, pci_bus_assign_domain_nr() dereferences that pointer. Many callers of pci_create_root_bus() supply a NULL "parent" pointer, which leads to a NULL pointer dereference error.
7c674700098c ("PCI: Move domain assignment from arm64 to generic code") moved the "parent" dereference from arm64 to generic code. Only arm64 used that code (because only arm64 defined CONFIG_PCI_DOMAINS_GENERIC), and it always supplied a valid "parent" pointer. Other arches supplied NULL "parent" pointers but didn't defined CONFIG_PCI_DOMAINS_GENERIC, so they used a no-op version of pci_bus_assign_domain_nr().
8c7d14746abc ("ARM/PCI: Move to generic PCI domains") defined CONFIG_PCI_DOMAINS_GENERIC on ARM, and many ARM platforms use pci_common_init(), which supplies a NULL "parent" pointer. These platforms (cns3xxx, dove, footbridge, iop13xx, etc.) crash with a NULL pointer dereference like this while probing PCI:
Unable to handle kernel NULL pointer dereference at virtual address 000000a4 PC is at pci_bus_assign_domain_nr+0x10/0x84
LR is at pci_create_root_bus+0x48/0x2e4
Kernel panic - not syncing: Attempted to kill init!
[bhelgaas: changelog, add "Reported:" and "Fixes:" tags]
Reported: http://forum.doozan.com/read.php?2,17868,22070,quote=1
Fixes 8c7d14746abc ("ARM/PCI: Move to generic PCI domains")
Fixes 7c674700098c ("PCI: Move domain assignment from arm64 to generic code")
Signed-off-by Krzysztof Ha??asa <[email protected]>
Signed-off-by Bjorn Helgaas <[email protected]>
CC stable at vger.kernel.org # v4.0+
drivers/pci/pci.c +0 -0
index 602eb42..f89db3a 100644
@@ -4772,8 +4772,10 @@ int pci_get_new_domain_nr(void)
 void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
drivers/pci/pci.c int pci_get_new_domain_nr(void) · line 4781
Quoted 10 lines
drivers/pci/pci.c +3 -1
 {
 	static int use_dt_domains = -1;
-	int domain = of_get_pci_domain_nr(parent->of_node);
+	int domain = -1;
 
+	if (parent)
+		domain = of_get_pci_domain_nr(parent->of_node);
 	/*
 	 * Check DT domain and use_dt_domains values.
 	 *
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi

2016-03-08 10:47 UTC #8 lore raw

On Mon, Mar 07, 2016 at 10:24:27PM -0600, Bjorn Helgaas wrote:

[...]

Quoted 26 lines

Quoted 14 lines
Quoted 5 lines
Actually, I did find one problem report:
http://forum.doozan.com/read.php?2,17868,22070,quote=1 from last May, but apparently it got lost in a forum and never found its way upstream.
Quoted 8 lines
I reworked the changelog because this problem will affect *any* arch that enables CONFIG_PCI_DOMAINS_GENERIC and supplies NULL "parent" pointers -- ia64, mips, mn10300, s390, x86, etc., would be affected if they enabled CONFIG_PCI_DOMAINS_GENERIC.
I also added a "Fixes:" tag for 7c674700098c, since that's the commit that added the generic code we're fixing. Backports of 7c674700098c should also backport this change.
Quoted 6 lines
That's really unfortunate, when I moved code from arm64 to generic I did not spot this issue in the original code and carried it over, you summarized the reasons in the commit log so without any further ado (and with my apologies):
Acked-by Lorenzo Pieralisi <[email protected]>
No worries, it just goes with the territory. What surprises me is that it took us so long to notice. v4.0 was released almost a year ago (April 12, 2015), so I can't figure out how nobody noticed until now.

Quoted 8 lines

And I don't know what happened with the problem report in the forum. That's a case where somebody *did* notice, but I guess they just gave up on v4.0 and went back to v3.18. What a shame :) I don't know if people just have low expectations of Linux, or they feel like it's too hard to report bugs, or we don't make it easy enough, or we're not approachable enough, or what. I notice that many times somebody finds a workaround, and people seem satisfied with that, and we don't get a chance to fix the real problem.

I agree it is a pity the problem was not reported upstream which would have solved the issue (that I should have spotted anyway while moving the code) a long time ago, unfortunately I think it has to do with how often developers/distros upgrade their kernels on these boards/socs and how they interact with upstream, which is a discussion worth having.

Thank you !
Lorenzo