fix possible null pointer access in acpi_pci_irq_{enable, disable}

PATCH In review Kenji Kaneshige 4 participants 8 messages Aug 1, 2005 – Aug 8, 2005 6 messages not in the archive

Subsystems (2)

ACPI PCI

lore

Share & tools

lore

Kenji Kaneshige

author

2005-08-01 03:32 UTC #0 lore raw

cover fix possible null pointer access in acpi_pci_irq_{enable, disable}

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Kenji Kaneshige

author

2005-08-01 03:34 UTC #2 lore raw

PATCH 1/2 fix possible null pointer access - acpi_pci_irq_enable

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 <[email protected]>

---

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

drivers/acpi/pci_irq.c renamed from drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable +5 -5

diff -puN drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable drivers/acpi/pci_irq.c
@@ -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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pavel Machek

2005-08-03 11:36 UTC #4 lore raw

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

Hi!

Quoted 5 lines

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

Quoted 46 lines

Signed-off-by Kenji Kaneshige <[email protected]>
---
 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
drivers/acpi/pci_irq.c renamed from drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable +5 -5
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/acpi-devel

Signature · 8 lines

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

A message that is not in the archive (sent off-list or not yet received): 20050803113637.GB4038-I/[email protected]

Kenji Kaneshige

author

2005-08-05 01:34 UTC #6 lore raw

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

Hi,

Quoted 4 lines

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

Quoted 57 lines · Pavel Machek

Hi!
Quoted 5 lines
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
Quoted 37 lines
Signed-off-by Kenji Kaneshige <[email protected]>
---
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
drivers/acpi/pci_irq.c renamed from drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_enable +5 -5
@@ -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.
_

Quoted 11 lines

Quoted 9 lines
-------------------------------------------------------
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 [email protected]
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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pavel Machek

2005-08-05 07:19 UTC #8 lore raw

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

Hi!

Quoted 9 lines

Quoted 4 lines
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

Signature · 8 lines

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

A message that is not in the archive (sent off-list or not yet received): 20050805071918.GG1780-I/[email protected]

Kenji Kaneshige

author

2005-08-08 04:24 UTC #10 lore raw

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

Hi,

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
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...

Quoted 8 lines

Quoted 4 lines
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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andrew Morton

2005-08-08 05:07 UTC #12 lore raw

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

Quoted 17 lines · Kenji Kaneshige

Hi,
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
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...
Quoted 8 lines
Quoted 4 lines
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.

Quoted 7 lines

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.

Kenji Kaneshige

author

2005-08-01 03:35 UTC #13 lore raw

PATCH 2/2 fix possible null pointer access - acpi_pci_irq_disable

In reply to a message not in the archive

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 <[email protected]>

---

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

drivers/acpi/pci_irq.c renamed from drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_disable +1 -4

diff -puN drivers/acpi/pci_irq.c~fix-possible-null-pointer-access-acpi_pci_irq_disable drivers/acpi/pci_irq.c
@@ -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.
 	 */