diff mbox

Fix NULL ptr dereference in pci_bus_assign_domain_nr() on ARM

Message ID m337sapw89.fsf@t19.piap.pl
State Accepted
Headers show

Commit Message

Krzysztof Hałasa March 1, 2016, 6:07 a.m. UTC
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 <khalasa@piap.pl>
Cc: stable@vger.kernel.org

Comments

Bjorn Helgaas March 3, 2016, 5:35 p.m. UTC | #1
Hi Krzysztof,

On Tue, Mar 01, 2016 at 07:07:18AM +0100, Krzysztof Hałasa wrote:
> 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.

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

> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> Cc: stable@vger.kernel.org
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 602eb42..f89db3a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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.
>  	 *
> 
> -- 
> Krzysztof Halasa
> 
> Industrial Research Institute for Automation and Measurements PIAP
> Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Hałasa March 4, 2016, 6:13 a.m. UTC | #2
Hi Bjorn,

Bjorn Helgaas <helgaas@kernel.org> writes:

> On Tue, Mar 01, 2016 at 07:07:18AM +0100, Krzysztof Hałasa wrote:
>> 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.
>
> 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.
Bjorn Helgaas March 4, 2016, 4:27 p.m. UTC | #3
On Fri, Mar 04, 2016 at 07:13:49AM +0100, Krzysztof Hałasa wrote:
> Hi Bjorn,
> 
> Bjorn Helgaas <helgaas@kernel.org> writes:
> 
> > On Tue, Mar 01, 2016 at 07:07:18AM +0100, Krzysztof Hałasa wrote:
> >> 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.
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Hałasa March 7, 2016, 1:54 p.m. UTC | #4
Bjorn Helgaas <helgaas@kernel.org> writes:

>> 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.
diff mbox

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 602eb42..f89db3a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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.
 	 *