[PATCH] PowerNV/PCI: Fix NULL PCI controller

[PATCH] PowerNV/PCI: Fix NULL PCI controller

[Mike Qiu]

In pnv_pci_read_config() or pnv_pci_write_config(), we never check if
the PCI controller is valid before converting that into platform
dependent one, this is very dangerous. 

To avoid this potential risks, the patch check PCI controller first
before use it.

Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/pci.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index b8b8e0b..e7b7f1a 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus,
 			       int where, int size, u32 *val)
 {
 	struct pci_controller *hose = pci_bus_to_host(bus);
-	struct pnv_phb *phb = hose->private_data;
+	struct pnv_phb *phb = hose ? hose->private_data : NULL;
 	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
 	s64 rc;
 
-	if (hose == NULL)
+	if (!phb)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	switch (size) {
@@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus,
 				int where, int size, u32 val)
 {
 	struct pci_controller *hose = pci_bus_to_host(bus);
-	struct pnv_phb *phb = hose->private_data;
+	struct pnv_phb *phb = hose ? hose->private_data : NULL;
 	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
 
-	if (hose == NULL)
+	if (!phb)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",
-- 
1.7.10.1

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Mike Qiu]

2013/4/22 14:13, Mike Qiu wrote:
Resend this patch because it has been send to kernel mailling list before :)
> In pnv_pci_read_config() or pnv_pci_write_config(), we never check if
> the PCI controller is valid before converting that into platform
> dependent one, this is very dangerous. 
>
> To avoid this potential risks, the patch check PCI controller first
> before use it.
>
> Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/pci.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index b8b8e0b..e7b7f1a 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus,
>  			       int where, int size, u32 *val)
>  {
>  	struct pci_controller *hose = pci_bus_to_host(bus);
> -	struct pnv_phb *phb = hose->private_data;
> +	struct pnv_phb *phb = hose ? hose->private_data : NULL;
>  	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
>  	s64 rc;
>
> -	if (hose == NULL)
> +	if (!phb)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>
>  	switch (size) {
> @@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus,
>  				int where, int size, u32 val)
>  {
>  	struct pci_controller *hose = pci_bus_to_host(bus);
> -	struct pnv_phb *phb = hose->private_data;
> +	struct pnv_phb *phb = hose ? hose->private_data : NULL;
>  	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
>
> -	if (hose == NULL)
> +	if (!phb)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>
>  	cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Benjamin Herrenschmidt]

On Mon, 2013-04-22 at 02:13 -0400, Mike Qiu wrote:
> In pnv_pci_read_config() or pnv_pci_write_config(), we never check if
> the PCI controller is valid before converting that into platform
> dependent one, this is very dangerous. 
> 
> To avoid this potential risks, the patch check PCI controller first
> before use it.

I don't think there's any remote possibility of that happening, is
there ?

If it does, maybe it warrants a WARN_ON...

Ben.

> Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/pci.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index b8b8e0b..e7b7f1a 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus,
>  			       int where, int size, u32 *val)
>  {
>  	struct pci_controller *hose = pci_bus_to_host(bus);
> -	struct pnv_phb *phb = hose->private_data;
> +	struct pnv_phb *phb = hose ? hose->private_data : NULL;
>  	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
>  	s64 rc;
>  
> -	if (hose == NULL)
> +	if (!phb)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
>  	switch (size) {
> @@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus,
>  				int where, int size, u32 val)
>  {
>  	struct pci_controller *hose = pci_bus_to_host(bus);
> -	struct pnv_phb *phb = hose->private_data;
> +	struct pnv_phb *phb = hose ? hose->private_data : NULL;
>  	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
>  
> -	if (hose == NULL)
> +	if (!phb)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
>  	cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Mike Qiu]

于 2013/4/22 14:36, Benjamin Herrenschmidt 写道:
> On Mon, 2013-04-22 at 02:13 -0400, Mike Qiu wrote:
>> In pnv_pci_read_config() or pnv_pci_write_config(), we never check if
>> the PCI controller is valid before converting that into platform
>> dependent one, this is very dangerous.
>>
>> To avoid this potential risks, the patch check PCI controller first
>> before use it.
> I don't think there's any remote possibility of that happening, is
> there ?
Yes, I agree, I don't exactly mean that it maybe happen, but the code
try to check the pci_controller pointer and the way it try is useless,
because if this happens, the system will crash before check: try to access
the "NULL" pointer.

My patch just makes the code more stable and robust.

Anyway, I think it's better to remove the check code as it is useless, as it
will shows that this "NULL" pci_controller pointer may happen...

Thanks
Mike
> If it does, maybe it warrants a WARN_ON...
>
> Ben.
>
>> Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/platforms/powernv/pci.c |    8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
>> index b8b8e0b..e7b7f1a 100644
>> --- a/arch/powerpc/platforms/powernv/pci.c
>> +++ b/arch/powerpc/platforms/powernv/pci.c
>> @@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus,
>>   			       int where, int size, u32 *val)
>>   {
>>   	struct pci_controller *hose = pci_bus_to_host(bus);
>> -	struct pnv_phb *phb = hose->private_data;
>> +	struct pnv_phb *phb = hose ? hose->private_data : NULL;
>>   	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
>>   	s64 rc;
>>   
>> -	if (hose == NULL)
>> +	if (!phb)
>>   		return PCIBIOS_DEVICE_NOT_FOUND;
>>   
>>   	switch (size) {
>> @@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus,
>>   				int where, int size, u32 val)
>>   {
>>   	struct pci_controller *hose = pci_bus_to_host(bus);
>> -	struct pnv_phb *phb = hose->private_data;
>> +	struct pnv_phb *phb = hose ? hose->private_data : NULL;
>>   	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
>>   
>> -	if (hose == NULL)
>> +	if (!phb)
>>   		return PCIBIOS_DEVICE_NOT_FOUND;
>>   
>>   	cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",
>
> --
> 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
>
>
>

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Benjamin Herrenschmidt]

On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote:
> Anyway, I think it's better to remove the check code as it is useless,
> as it
> will shows that this "NULL" pci_controller pointer may happen...

Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host()
itself ... no big deal either way.

Cheers,
Ben.

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Mike Qiu]

于 2013/4/22 16:04, Benjamin Herrenschmidt 写道:
> On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote:
>> Anyway, I think it's better to remove the check code as it is useless,
>> as it
>> will shows that this "NULL" pci_controller pointer may happen...
> Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host()
> itself ... no big deal either way.
OK, you mean the code will remain the same and my patch is worthless?

Anyway, it will be OK for me. But I hope it can be accept for more stable
reason.

Thanks
Mike
> Cheers,
> Ben.
>
>
> --
> 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
>
>
>

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Bjorn Helgaas]

On Mon, Apr 22, 2013 at 3:44 AM, Mike Qiu <qiudayu@linux.vnet.ibm.com> wrote:
> 于 2013/4/22 16:04, Benjamin Herrenschmidt 写道:
>
>> On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote:
>>>
>>> Anyway, I think it's better to remove the check code as it is useless,
>>> as it
>>> will shows that this "NULL" pci_controller pointer may happen...
>>
>> Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host()
>> itself ... no big deal either way.
>
> OK, you mean the code will remain the same and my patch is worthless?
>
> Anyway, it will be OK for me. But I hope it can be accept for more stable
> reason.

This is powerpc code, so I'm kibitzing here, but it was cc'd to linux-pci :)

There should be no way to get a struct pci_bus * where bus->sysdata
(== "hose") is NULL.  The sysdata pointer is always supplied to
pci_create_root_bus() (or a similar interface that calls
pci_create_root_bus()), so every root bus has a valid sysdata pointer.
 And every child bus inherits the sysdata pointer of its parent (in
pci_alloc_child_bus()).  Therefore, every pci_bus should have a valid
sysdata pointer.

So I think you should just remove even the existing "if (hose ==
NULL)" check.  That way, if bus->sysdata actually *does* turn out to
be NULL, we'll oops on the null pointer dereference, get a nice
backtrace, and have a chance to fix the problem.  Testing and
returning an error means whatever bug or memory corruption caused the
null pointer will most likely be ignored.

Bjorn

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Mike Qiu]

On Mon, 2013-04-22 at 11:35 -0600, Bjorn Helgaas wrote:
> On Mon, Apr 22, 2013 at 3:44 AM, Mike Qiu <qiudayu@linux.vnet.ibm.com> wrote:
> > 于 2013/4/22 16:04, Benjamin Herrenschmidt 写道:
> >
> >> On Mon, 2013-04-22 at 15:41 +0800, Mike Qiu wrote:
> >>>
> >>> Anyway, I think it's better to remove the check code as it is useless,
> >>> as it
> >>> will shows that this "NULL" pci_controller pointer may happen...
> >>
> >> Ok. It *might* still be worth adding a BUG_ON then in pci_bus_to_host()
> >> itself ... no big deal either way.
> >
> > OK, you mean the code will remain the same and my patch is worthless?
> >
> > Anyway, it will be OK for me. But I hope it can be accept for more stable
> > reason.
> 
> This is powerpc code, so I'm kibitzing here, but it was cc'd to linux-pci :)
> 
> There should be no way to get a struct pci_bus * where bus->sysdata
> (== "hose") is NULL.  The sysdata pointer is always supplied to
> pci_create_root_bus() (or a similar interface that calls
> pci_create_root_bus()), so every root bus has a valid sysdata pointer.
>  And every child bus inherits the sysdata pointer of its parent (in
> pci_alloc_child_bus()).  Therefore, every pci_bus should have a valid
> sysdata pointer.
And the kernel has checked whether if sysdata is NULL after call
pci_create_root_bus().
> 
> So I think you should just remove even the existing "if (hose ==
> NULL)" check.  That way, if bus->sysdata actually *does* turn out to
> be NULL, we'll oops on the null pointer dereference, get a nice
> backtrace, and have a chance to fix the problem.  Testing and
> returning an error means whatever bug or memory corruption caused the
> null pointer will most likely be ignored.
> 
Yes, I agree with you, because it is useless, and meaningless. Also
it never achieve its goals even if the "hose==NULL". 

So I suggest either use my patch or remove that check code.
If the second case, I will write v2 patch to remove that check code.
I don't know what's to go for next step, it depends on the
maintainer :)

Thanks
Mike
 
> 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
> 

Re: [PATCH] PowerNV/PCI: Fix NULL PCI controller

[Benjamin Herrenschmidt]

On Mon, 2013-04-22 at 21:46 -0400, Mike Qiu wrote:
> 
> So I suggest either use my patch or remove that check code.
> If the second case, I will write v2 patch to remove that check code.
> I don't know what's to go for next step, it depends on the
> maintainer :)

Please do.

Cheers,
Ben.

[PATCH] PowerNV/PCI: Fix NULL PCI controller

[Mike Qiu]

In pnv_pci_read_config() or pnv_pci_write_config(), we never check if
the PCI controller is valid before converting that into platform
dependent one, this is very dangerous. 

To avoid this potential risks, the patch check PCI controller first
before use it.

Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/pci.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index b8b8e0b..e7b7f1a 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -286,11 +286,11 @@ static int pnv_pci_read_config(struct pci_bus *bus,
 			       int where, int size, u32 *val)
 {
 	struct pci_controller *hose = pci_bus_to_host(bus);
-	struct pnv_phb *phb = hose->private_data;
+	struct pnv_phb *phb = hose ? hose->private_data : NULL;
 	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
 	s64 rc;
 
-	if (hose == NULL)
+	if (!phb)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	switch (size) {
@@ -330,10 +330,10 @@ static int pnv_pci_write_config(struct pci_bus *bus,
 				int where, int size, u32 val)
 {
 	struct pci_controller *hose = pci_bus_to_host(bus);
-	struct pnv_phb *phb = hose->private_data;
+	struct pnv_phb *phb = hose ? hose->private_data : NULL;
 	u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
 
-	if (hose == NULL)
+	if (!phb)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",
-- 
1.7.10.1