[PATCH] PCI: Fix possible NULL pointer dereference for of_pci_bus_find_domain_nr

[PATCH] PCI: Fix possible NULL pointer dereference for of_pci_bus_find_domain_nr

[Shawn Lin]

pci_register_host_bridge records bus->domain_nr from
pci_bus_find_domain_nr but not guarantee not to pass a NULL
struct device *parent to it which could be explained by the hint
of checkcing for parent device before calling set_dev_node(),
just lines after that. So of_pci_bus_find_domain_nr wisely check
the parent pointer at the very beginning, but forgot to check it
again when trying to get of_node from parent, which could causes
a NULL pointer dereference. Fix it by dumping the NULL pointer
address simply, if no parent available.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd1..ef18c48 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5612,7 +5612,7 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
 		domain = pci_get_new_domain_nr();
 	} else {
 		dev_err(parent, "Node %pOF has inconsistent \"linux,pci-domain\" property in DT\n",
-			parent->of_node);
+			parent ? parent->of_node : NULL);
 		domain = -1;
 	}
 
-- 
1.9.1

Re: [PATCH] PCI: Fix possible NULL pointer dereference for of_pci_bus_find_domain_nr

[Lorenzo Pieralisi]

On Sun, Feb 25, 2018 at 10:46:02AM +0800, Shawn Lin wrote:
> pci_register_host_bridge records bus->domain_nr from
> pci_bus_find_domain_nr but not guarantee not to pass a NULL
> struct device *parent to it which could be explained by the hint
> of checkcing for parent device before calling set_dev_node(),
> just lines after that. So of_pci_bus_find_domain_nr wisely check
> the parent pointer at the very beginning, but forgot to check it
> again when trying to get of_node from parent, which could causes
> a NULL pointer dereference. Fix it by dumping the NULL pointer
> address simply, if no parent available.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd1..ef18c48 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5612,7 +5612,7 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
>  		domain = pci_get_new_domain_nr();
>  	} else {
>  		dev_err(parent, "Node %pOF has inconsistent \"linux,pci-domain\" property in DT\n",
> -			parent->of_node);
> +			parent ? parent->of_node : NULL);

I really need to get rid of this function in its current form. In the
interim, I think that printing NULL as faulting node gives no
information whatsoever so this patch should be updated either by
avoiding to print or better by demoting the dev_err() to a pr_err(),
whatever works better for Bjorn.

Thanks,
Lorenzo

>  		domain = -1;
>  	}
>  
> -- 
> 1.9.1
> 
> 

Re: [PATCH] PCI: Fix possible NULL pointer dereference for of_pci_bus_find_domain_nr

[Shawn Lin]

Dear Lorenzo,

On 2018/2/28 1:49, Lorenzo Pieralisi wrote:
> On Sun, Feb 25, 2018 at 10:46:02AM +0800, Shawn Lin wrote:
> I really need to get rid of this function in its current form. In the
> interim, I think that printing NULL as faulting node gives no
> information whatsoever so this patch should be updated either by
> avoiding to print or better by demoting the dev_err() to a pr_err(),
> whatever works better for Bjorn.
> 

Ok, that sounds reasonable to me, so which ways is preferred, Bjorn?

Re: [PATCH] PCI: Fix possible NULL pointer dereference for of_pci_bus_find_domain_nr

[Bjorn Helgaas]

On Wed, Feb 28, 2018 at 09:42:39AM +0800, Shawn Lin wrote:
> On 2018/2/28 1:49, Lorenzo Pieralisi wrote:
> > On Sun, Feb 25, 2018 at 10:46:02AM +0800, Shawn Lin wrote:
> > I really need to get rid of this function in its current form. In the
> > interim, I think that printing NULL as faulting node gives no
> > information whatsoever so this patch should be updated either by
> > avoiding to print or better by demoting the dev_err() to a pr_err(),
> > whatever works better for Bjorn.
> 
> Ok, that sounds reasonable to me, so which ways is preferred, Bjorn?

If we get to the point of that dev_err(), I don't think we really know
what to do, so we should always print *something* as a clue to the
user because I think things are going to break if we use -1 as the
domain -- not because -1 is such an invalid value per se, but it's an
indication that the DT doesn't correctly describe the machine.  We
may make incorrect assumptions about which devices are in the same
domain.

So I'd make it a pr_err() for now.  It would be good to include the
%pOF when "parent" is not NULL.

The changelog could be simply:

  If the "parent" pointer passed to of_pci_bus_find_domain_nr() is
  NULL, don't dereference it.