AHCI: fix Null pointer dereference in achi_host_active()

PATCH In review Xiaotian Feng 3 participants 7 messages Jul 16, 2013 – Jul 23, 2013 libata

Share & tools

Xiaotian Feng

author

2013-07-16 05:18 UTC #0 lore raw

PATCH 1/1 AHCI: fix Null pointer dereference in achi_host_active()

commit b29900e6 introuded a regression, which resulted Null pointer dereference for achi host with dummy ports. For ahci ports, when the port is dummy port, its private_data will be NULL, as ata_dummy_port_ops doesn't support ->port_start.

Reported-and-tested-by Alex Williamson <[email protected]>

Signed-off-by Xiaotian Feng <[email protected]>

Cc Alexander Gordeev <[email protected]>

Cc Tejun Heo <[email protected]>

Cc [email protected]

---
 drivers/ata/ahci.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/ata/ahci.c +7 -1

index 5064f3e..f1de689 100644
@@ -1147,10 +1147,16 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis)
 
 	for (i = 0; i < host->n_ports; i++) {
 		struct ahci_port_priv *pp = host->ports[i]->private_data;
+		const char *desc;
 
+		if (ata_port_is_dummy(host->ports[i]))
+			desc = dev_driver_string(host->dev);
+		else
+			desc = pp->irq_desc;
+ 
 		rc = devm_request_threaded_irq(host->dev,
 			irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED,
-			pp->irq_desc, host->ports[i]);
+			desc, host->ports[i]);
 		if (rc)
 			goto out_free_irqs;
 	}

Signature · 1 line

1.7.9.6 (Apple Git-31.1)

Sergei Shtylyov

2013-07-16 11:48 UTC #1 lore raw

Hello.

Quoted 1 line · Xiaotian Feng

commit b29900e6 introuded a regression, which resulted Null pointer

Please also specify that commit's summary in parens.

Quoted 10 lines

dereference for achi host with dummy ports. For ahci ports, when the port is dummy port, its private_data will be NULL, as ata_dummy_port_ops doesn't support ->port_start.
Reported-and-tested-by Alex Williamson <[email protected]>
Signed-off-by Xiaotian Feng <[email protected]>
Cc Alexander Gordeev <[email protected]>
Cc Tejun Heo <[email protected]>
Cc [email protected]
Cc [email protected]

MBR, Sergei

Xiaotian Feng

author

2013-07-17 06:10 UTC #2 lore raw

PATCH 1/1 AHCI: fix Null pointer dereference in achi_host_active()

commit b29900e6 (AHCI: Make distinct names for ports in /proc/interrupts) introuded a regression, which resulted Null pointer dereference for achi host with dummy ports. For ahci ports, when the port is dummy port, its private_data will be NULL, as ata_dummy_port_ops doesn't support ->port_start.

Reported-and-tested-by Alex Williamson <[email protected]>

Signed-off-by Xiaotian Feng <[email protected]>

Cc Alexander Gordeev <[email protected]>

Cc Tejun Heo <[email protected]>

Cc [email protected]

---
 drivers/ata/ahci.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/ata/ahci.c +7 -1

index 5064f3e..f1de689 100644
@@ -1147,10 +1147,16 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis)
 
 	for (i = 0; i < host->n_ports; i++) {
 		struct ahci_port_priv *pp = host->ports[i]->private_data;
+		const char *desc;
 
+		if (ata_port_is_dummy(host->ports[i]))
+			desc = dev_driver_string(host->dev);
+		else
+			desc = pp->irq_desc;
+ 
 		rc = devm_request_threaded_irq(host->dev,
 			irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED,
-			pp->irq_desc, host->ports[i]);
+			desc, host->ports[i]);
 		if (rc)
 			goto out_free_irqs;
 	}

Signature · 1 line

1.7.9.6 (Apple Git-31.1)

Tejun Heo

2013-07-22 21:28 UTC #3 lore raw

Hello, Xiaotian.

Thanks for the fix. A couple comments below.

drivers/ata/ahci.c int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) · line 1155

Quoted 8 lines · Xiaotian Feng

drivers/ata/ahci.c +5 -0

 	for (i = 0; i < host->n_ports; i++) {
 		struct ahci_port_priv *pp = host->ports[i]->private_data;
+		const char *desc;
 
+		if (ata_port_is_dummy(host->ports[i]))
+			desc = dev_driver_string(host->dev);
+		else
+			desc = pp->irq_desc;

I think it'd be better to branch on pp. ie. do "if (pp) desc = pp->... " instead and then add a comment saying "pp is NULL for dummies".

Thanks!

Signature · 1 line

tejun

Xiaotian Feng

author

2013-07-22 21:31 UTC #4 lore raw

Quoted 9 lines · Tejun Heo

Hello, Xiaotian.
Thanks for the fix. A couple comments below.
Quoted 4 lines · Xiaotian Feng
      for (i = 0; i < host->n_ports; i++) {
              struct ahci_port_priv *pp = host->ports[i]->private_data;
+ const char *desc;

Quoted 9 lines

drivers/ata/ahci.c int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis) · line 1155
Quoted 4 lines
drivers/ata/ahci.c +4 -0
+             if (ata_port_is_dummy(host->ports[i]))
+                     desc = dev_driver_string(host->dev);
+             else
+                     desc = pp->irq_desc;
I think it'd be better to branch on pp. ie. do "if (pp) desc = pp->... " instead and then add a comment saying "pp is NULL for dummies".

Okay, I'll update v2 patch, thanks :)

Quoted 4 lines

Thanks!
--
tejun

Xiaotian Feng

author

2013-07-23 03:54 UTC #5 lore raw

PATCH v2 1/1 AHCI: fix Null pointer dereference in achi_host_active()

In reply to Tejun Heo

changes in v2: use pp to check dummy ports, update comments

Reported-and-tested-by Alex Williamson <[email protected]>

Signed-off-by Xiaotian Feng <[email protected]>

Cc Alexander Gordeev <[email protected]>

Cc Tejun Heo <[email protected]>

Cc [email protected]

---
 drivers/ata/ahci.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/ata/ahci.c +8 -1

index 5064f3e..db4380d 100644
@@ -1146,11 +1146,18 @@ int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis)
 		return rc;
 
 	for (i = 0; i < host->n_ports; i++) {
+		const char* desc;
 		struct ahci_port_priv *pp = host->ports[i]->private_data;
 
+		/* pp is NULL for dummy ports */
+		if (pp)
+			desc = pp->irq_desc;
+		else
+			desc = dev_driver_string(host->dev);
+
 		rc = devm_request_threaded_irq(host->dev,
 			irq + i, ahci_hw_interrupt, ahci_thread_fn, IRQF_SHARED,
-			pp->irq_desc, host->ports[i]);
+			desc, host->ports[i]);
 		if (rc)
 			goto out_free_irqs;
 	}

Signature · 1 line

1.7.9.6 (Apple Git-31.1)

Tejun Heo

Applied

2013-07-23 14:26 UTC #6 lore raw

Quoted 5 lines · Xiaotian Feng

commit b29900e6 (AHCI: Make distinct names for ports in /proc/interrupts) introuded a regression, which resulted Null pointer dereference for achi host with dummy ports. For ahci ports, when the port is dummy port, its private_data will be NULL, as ata_dummy_port_ops doesn't support ->port_start.

Quoted 8 lines

changes in v2: use pp to check dummy ports, update comments
Reported-and-tested-by Alex Williamson <[email protected]>
Signed-off-by Xiaotian Feng <[email protected]>
Cc Alexander Gordeev <[email protected]>
Cc Tejun Heo <[email protected]>
Cc [email protected]
Cc [email protected]

Applied to libata/for-3.11-fixes.

Thanks.

Signature · 1 line

tejun