[PATCH 2.6.13-rc4 0/2] fix possible null pointer access in acpi_pci_irq_{enable, disable}

[PATCH 2.6.13-rc4 0/2] fix possible null pointer access in acpi_pci_irq_{enable, disable}

[Kenji Kaneshige]

Hi,

This series of patches fixes possible null pointer access in
acpi_pci_irq_enable() and acpi_pci_irq_disable() respectively.

This is reposting of the patches I posted to acpi-devel ML before at:

    http://sourceforge.net/mailarchive/message.php?msg_id=11322751
    http://sourceforge.net/mailarchive/message.php?msg_id=11322752

Thanks,
Kenji Kaneshige



-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

[PATCH 2.6.13-rc4 1/2] fix possible null pointer access - acpi_pci_irq_enable

[Kenji Kaneshige]

This patch fixes possible null pointer access in acpi_pci_irq_enable.

The 'bus' field in pci_dev structure should be checked before calling
pci_read_config_byte() because pci_bus_read_config_byte() called by
pci_read_config_byte() refers to 'bus' field.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---

 drivers/acpi/pci_irq.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff -puN drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable drivers/acpi/pci_irq.c
--- linux-2.6.13-rc4/drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable	2005-08-01 12:20:26.000000000 +0900
+++ linux-2.6.13-rc4-kanesige/drivers/acpi/pci_irq.c	2005-08-01 12:20:26.000000000 +0900
@@ -397,6 +397,11 @@ acpi_pci_irq_enable (
 
 	if (!dev)
 		return_VALUE(-EINVAL);
+
+	if (!dev->bus) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid (NULL) 'bus' field\n"));
+		return_VALUE(-ENODEV);
+	}
 	
 	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
 	if (!pin) {
@@ -405,11 +410,6 @@ acpi_pci_irq_enable (
 	}
 	pin--;
 
-	if (!dev->bus) {
-		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid (NULL) 'bus' field\n"));
-		return_VALUE(-ENODEV);
-	}
-
 	/* 
 	 * First we check the PCI IRQ routing table (PRT) for an IRQ.  PRT
 	 * values override any BIOS-assigned IRQs set during boot.
_



-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: [PATCH 2.6.13-rc4 1/2] fix possible null pointer access - acpi_pci_irq_enable

[Pavel Machek]

Hi!

> This patch fixes possible null pointer access in acpi_pci_irq_enable.
> 
> The 'bus' field in pci_dev structure should be checked before calling
> pci_read_config_byte() because pci_bus_read_config_byte() called by
> pci_read_config_byte() refers to 'bus' field.

Should people be passing NULLs here, anyway? Is not the right fix to
remove the check, and remove the (!dev) check, too, and just fix the
callers? People are going to fix the oops, but noone is going to see
that ACPI_DEBUG_PRINT...


								Pavel


> Signed-off-by: Kenji Kaneshige <kaneshige.kenji-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> ---
> 
>  drivers/acpi/pci_irq.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff -puN drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable drivers/acpi/pci_irq.c
> --- linux-2.6.13-rc4/drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable	2005-08-01 12:20:26.000000000 +0900
> +++ linux-2.6.13-rc4-kanesige/drivers/acpi/pci_irq.c	2005-08-01 12:20:26.000000000 +0900
> @@ -397,6 +397,11 @@ acpi_pci_irq_enable (
>  
>  	if (!dev)
>  		return_VALUE(-EINVAL);
> +
> +	if (!dev->bus) {
> +		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid (NULL) 'bus' field\n"));
> +		return_VALUE(-ENODEV);
> +	}
>  	
>  	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
>  	if (!pin) {
> @@ -405,11 +410,6 @@ acpi_pci_irq_enable (
>  	}
>  	pin--;
>  
> -	if (!dev->bus) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid (NULL) 'bus' field\n"));
> -		return_VALUE(-ENODEV);
> -	}
> -
>  	/* 
>  	 * First we check the PCI IRQ routing table (PRT) for an IRQ.  PRT
>  	 * values override any BIOS-assigned IRQs set during boot.
> _
> 
> 
> 
> -------------------------------------------------------
> SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
> from IBM. Find simple to follow Roadmaps, straightforward articles,
> informative Webcasts and more! Get everything you need to get up to
> speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
> _______________________________________________
> Acpi-devel mailing list
> Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/acpi-devel

-- 
teflon -- maybe it is a trademark, but it should not be.


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH 2.6.13-rc4 1/2] fix possible null pointer access - acpi_pci_irq_enable

[Kenji Kaneshige]

Hi,

> Should people be passing NULLs here, anyway? Is not the right fix to
> remove the check, and remove the (!dev) check, too, and just fix the
> callers? People are going to fix the oops, but noone is going to see
> that ACPI_DEBUG_PRINT...

As you said, I think people should not pass NULLs here, and we should
fix the caller. But I think it is not bad to check (!dev) and (!dev->bus)
in these functions instead of panic. How about using WARN_ON here to
notify people of badness?

	if (!dev) {
		WARN_ON(1);
		return_VALUE(-EINVAL);
	}

	if (!dev->bus) {
		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid (NULL) 'bus' field\n"));
		WARN_ON(1);
		return_VALUE(-ENODEV);
	}

Thanks,
Kenji Kaneshige


Pavel Machek wrote:
> Hi!
> 
> 
>>This patch fixes possible null pointer access in acpi_pci_irq_enable.
>>
>>The 'bus' field in pci_dev structure should be checked before calling
>>pci_read_config_byte() because pci_bus_read_config_byte() called by
>>pci_read_config_byte() refers to 'bus' field.
> 
> 
> Should people be passing NULLs here, anyway? Is not the right fix to
> remove the check, and remove the (!dev) check, too, and just fix the
> callers? People are going to fix the oops, but noone is going to see
> that ACPI_DEBUG_PRINT...
> 
> 
> 								Pavel
> 
> 
> 
>>Signed-off-by: Kenji Kaneshige <kaneshige.kenji-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>>---
>>
>> drivers/acpi/pci_irq.c |   10 +++++-----
>> 1 files changed, 5 insertions(+), 5 deletions(-)
>>
>>diff -puN drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable drivers/acpi/pci_irq.c
>>--- linux-2.6.13-rc4/drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable	2005-08-01 12:20:26.000000000 +0900
>>+++ linux-2.6.13-rc4-kanesige/drivers/acpi/pci_irq.c	2005-08-01 12:20:26.000000000 +0900
>>@@ -397,6 +397,11 @@ acpi_pci_irq_enable (
>> 
>> 	if (!dev)
>> 		return_VALUE(-EINVAL);
>>+
>>+	if (!dev->bus) {
>>+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid (NULL) 'bus' field\n"));
>>+		return_VALUE(-ENODEV);
>>+	}
>> 	
>> 	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
>> 	if (!pin) {
>>@@ -405,11 +410,6 @@ acpi_pci_irq_enable (
>> 	}
>> 	pin--;
>> 
>>-	if (!dev->bus) {
>>-		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid (NULL) 'bus' field\n"));
>>-		return_VALUE(-ENODEV);
>>-	}
>>-
>> 	/* 
>> 	 * First we check the PCI IRQ routing table (PRT) for an IRQ.  PRT
>> 	 * values override any BIOS-assigned IRQs set during boot.
>>_
>>
>>
>>
>>-------------------------------------------------------
>>SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
>>from IBM. Find simple to follow Roadmaps, straightforward articles,
>>informative Webcasts and more! Get everything you need to get up to
>>speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
>>_______________________________________________
>>Acpi-devel mailing list
>>Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>https://lists.sourceforge.net/lists/listinfo/acpi-devel
> 
> 



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH 2.6.13-rc4 1/2] fix possible null pointer access - acpi_pci_irq_enable

[Pavel Machek]

Hi!

> >Should people be passing NULLs here, anyway? Is not the right fix to
> >remove the check, and remove the (!dev) check, too, and just fix the
> >callers? People are going to fix the oops, but noone is going to see
> >that ACPI_DEBUG_PRINT...
> 
> As you said, I think people should not pass NULLs here, and we should
> fix the caller. But I think it is not bad to check (!dev) and (!dev->bus)
> in these functions instead of panic. How about using WARN_ON here to
> notify people of badness?

No, just read l-k archives. Just dereference it. It wil oops in a nice
way, and developer *will* see and fix it.

								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH 2.6.13-rc4 1/2] fix possible null pointer access - acpi_pci_irq_enable

[Kenji Kaneshige]

Hi,

>>>Should people be passing NULLs here, anyway? Is not the right fix to
>>>remove the check, and remove the (!dev) check, too, and just fix the
>>>callers? People are going to fix the oops, but noone is going to see
>>>that ACPI_DEBUG_PRINT...
>>
>>As you said, I think people should not pass NULLs here, and we should
>>fix the caller. But I think it is not bad to check (!dev) and (!dev->bus)
>>in these functions instead of panic. How about using WARN_ON here to
>>notify people of badness?
> 
> 
> No, just read l-k archives. Just dereference it. It wil oops in a nice
> way, and developer *will* see and fix it.

OK. I'll make a new patch that removes the check.

Andrew, could you please drop the following two patches from -mm tree?

    o fix-possible-null-pointer-access-acpi_pci_irq_enable.patch
    o fix-possible-null-pointer-access-acpi_pci_irq_disable.patch

Thanks,
Kenji Kaneshige


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: [PATCH 2.6.13-rc4 1/2] fix possible null pointer access - acpi_pci_irq_enable

[Andrew Morton]

Kenji Kaneshige <kaneshige.kenji-+CUm20s59erQFUHtdCDX3A@public.gmane.org> wrote:
>
> Hi,
> 
> >>>Should people be passing NULLs here, anyway? Is not the right fix to
> >>>remove the check, and remove the (!dev) check, too, and just fix the
> >>>callers? People are going to fix the oops, but noone is going to see
> >>>that ACPI_DEBUG_PRINT...
> >>
> >>As you said, I think people should not pass NULLs here, and we should
> >>fix the caller. But I think it is not bad to check (!dev) and (!dev->bus)
> >>in these functions instead of panic. How about using WARN_ON here to
> >>notify people of badness?
> > 
> > 
> > No, just read l-k archives. Just dereference it. It wil oops in a nice
> > way, and developer *will* see and fix it.
> 
> OK. I'll make a new patch that removes the check.
> 
> Andrew, could you please drop the following two patches from -mm tree?
> 
>     o fix-possible-null-pointer-access-acpi_pci_irq_enable.patch
>     o fix-possible-null-pointer-access-acpi_pci_irq_disable.patch
> 

They're not in -mm any more.  Either they've been merged into Len's tree or
I accidentally dropped them, assuming they were in Len's tree due to them
throwing rejects.



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

[PATCH 2.6.13-rc4 2/2] fix possible null pointer access - acpi_pci_irq_disable

[Kenji Kaneshige]

This patch fixes possible null pointer access in acpi_pci_irq_disable.

The 'bus' field in pci_dev structure should be checked before calling
pci_read_config_byte() because pci_bus_read_config_byte() called by
pci_read_config_byte() refers to 'bus' field.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
---

 drivers/acpi/pci_irq.c |    5 +----
 1 files changed, 1 insertion(+), 4 deletions(-)

diff -puN drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_disable drivers/acpi/pci_irq.c
--- linux-2.6.13-rc4/drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_disable	2005-08-01 12:20:31.000000000 +0900
+++ linux-2.6.13-rc4-kanesige/drivers/acpi/pci_irq.c	2005-08-01 12:20:31.000000000 +0900
@@ -474,7 +474,7 @@ acpi_pci_irq_disable (
 
 	ACPI_FUNCTION_TRACE("acpi_pci_irq_disable");
 
-	if (!dev)
+	if (!dev || !dev->bus)
 		return_VOID;
 
 	pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
@@ -482,9 +482,6 @@ acpi_pci_irq_disable (
 		return_VOID;
 	pin--;
 
-	if (!dev->bus)
-		return_VOID;
-
 	/*
 	 * First we check the PCI IRQ routing table (PRT) for an IRQ.
 	 */
_



-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click