diff mbox

[net] net: tg3: fix NULL pointer dereference in tg3_io_error_detected and tg3_io_slot_reset

Message ID 1376419513-31924-1-git-send-email-nsujir@broadcom.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Nithin Sujir Aug. 13, 2013, 6:45 p.m. UTC 
From: Daniel Borkmann <dborkman@redhat.com>

Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible
NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev)
condition is met and netdev is NULL. Then, the jump to the 'done' label
calls dev_close() with a netdevice that is NULL. Therefore, only call
dev_close() when we have a netdevice, but one that is not running.

[ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin
Nayak Sujir ]

Reported-by: Dave Jones <davej@redhat.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
Cc: Michael Chan <mchan@broadcom.com>
Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

David Miller Aug. 15, 2013, 8:07 a.m. UTC | #1 
From: "Nithin Nayak Sujir" <nsujir@broadcom.com>
Date: Tue, 13 Aug 2013 11:45:13 -0700

> From: Daniel Borkmann <dborkman@redhat.com>
> 
> Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible
> NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev)
> condition is met and netdev is NULL. Then, the jump to the 'done' label
> calls dev_close() with a netdevice that is NULL. Therefore, only call
> dev_close() when we have a netdevice, but one that is not running.
> 
> [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin
> Nayak Sujir ]
> 
> Reported-by: Dave Jones <davej@redhat.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
> Cc: Michael Chan <mchan@broadcom.com>
> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>

Can I get some reviews from the Broadcom folks?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Kravkov Aug. 15, 2013, 10:57 a.m. UTC | #2 
On Thu, Aug 15, 2013 at 11:07 AM, David Miller <davem@davemloft.net> wrote:
>
> From: "Nithin Nayak Sujir" <nsujir@broadcom.com>
> Date: Tue, 13 Aug 2013 11:45:13 -0700
>
> > From: Daniel Borkmann <dborkman@redhat.com>
> >
> > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible
> > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev)
> > condition is met and netdev is NULL. Then, the jump to the 'done' label
> > calls dev_close() with a netdevice that is NULL. Therefore, only call
> > dev_close() when we have a netdevice, but one that is not running.
> >
> > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin
> > Nayak Sujir ]
> >
> > Reported-by: Dave Jones <davej@redhat.com>
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
> > Cc: Michael Chan <mchan@broadcom.com>
> > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
>
> Can I get some reviews from the Broadcom folks?
>
> Thanks.

You already have:
Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Chan Aug. 15, 2013, 3:15 p.m. UTC | #3 
On Thu, 2013-08-15 at 01:07 -0700, David Miller wrote: 
> From: "Nithin Nayak Sujir" <nsujir@broadcom.com>
> Date: Tue, 13 Aug 2013 11:45:13 -0700
> 
> > From: Daniel Borkmann <dborkman@redhat.com>
> > 
> > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible
> > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev)
> > condition is met and netdev is NULL. Then, the jump to the 'done' label
> > calls dev_close() with a netdevice that is NULL. Therefore, only call
> > dev_close() when we have a netdevice, but one that is not running.
> > 
> > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin
> > Nayak Sujir ]
> > 
> > Reported-by: Dave Jones <davej@redhat.com>
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
> > Cc: Michael Chan <mchan@broadcom.com>
> > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
> 
> Can I get some reviews from the Broadcom folks?
> 

We worked with Daniel at Red Hat and Gavin at IBM on this patch.
Thanks.

Signed-off-by: Michael Chan <mchan@broadcom.com>


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nithin Sujir Aug. 15, 2013, 5:32 p.m. UTC | #4 
>> Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
>> Cc: Michael Chan <mchan@broadcom.com>
>> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
>
> Can I get some reviews from the Broadcom folks?
>

David,
I made the additional changes to Daniel's patch and Michael Chan reviewed it 
before I submitted.

Sorry, I forgot to add Michael's Signed-off-by. Do you want me to resend it with 
his Signed-off-by added?

Thanks,
Nithin.


> Thanks.
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 15, 2013, 8:17 p.m. UTC | #5 
From: "Nithin Nayak Sujir" <nsujir@broadcom.com>
Date: Thu, 15 Aug 2013 10:32:58 -0700

> Sorry, I forgot to add Michael's Signed-off-by. Do you want me to
> resend it with his Signed-off-by added?

That's not necessary, it gets added automatically by patchwork
since he replied and added it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 15, 2013, 8:41 p.m. UTC | #6 
From: "Michael Chan" <mchan@broadcom.com>
Date: Thu, 15 Aug 2013 08:15:20 -0700

> On Thu, 2013-08-15 at 01:07 -0700, David Miller wrote: 
>> From: "Nithin Nayak Sujir" <nsujir@broadcom.com>
>> Date: Tue, 13 Aug 2013 11:45:13 -0700
>> 
>> > From: Daniel Borkmann <dborkman@redhat.com>
>> > 
>> > Commit d8af4dfd8 ("net/tg3: Fix kernel crash") introduced a possible
>> > NULL pointer dereference in tg3 driver when !netdev || !netif_running(netdev)
>> > condition is met and netdev is NULL. Then, the jump to the 'done' label
>> > calls dev_close() with a netdevice that is NULL. Therefore, only call
>> > dev_close() when we have a netdevice, but one that is not running.
>> > 
>> > [ Add the same checks in tg3_io_slot_reset() per Gavin Shan - by Nithin
>> > Nayak Sujir ]
>> > 
>> > Reported-by: Dave Jones <davej@redhat.com>
>> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> > Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
>> > Cc: Michael Chan <mchan@broadcom.com>
>> > Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom.com>
>> 
>> Can I get some reviews from the Broadcom folks?
>> 
> 
> We worked with Daniel at Red Hat and Gavin at IBM on this patch.
> Thanks.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>

Thanks for the info, applied.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index ddebc7a..0da2214 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -17796,8 +17796,10 @@  static pci_ers_result_t tg3_io_error_detected(struct pci_dev *pdev,
 
 done:
 	if (state == pci_channel_io_perm_failure) {
-		tg3_napi_enable(tp);
-		dev_close(netdev);
+		if (netdev) {
+			tg3_napi_enable(tp);
+			dev_close(netdev);
+		}
 		err = PCI_ERS_RESULT_DISCONNECT;
 	} else {
 		pci_disable_device(pdev);
@@ -17827,7 +17829,8 @@  static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev)
 	rtnl_lock();
 
 	if (pci_enable_device(pdev)) {
-		netdev_err(netdev, "Cannot re-enable PCI device after reset.\n");
+		dev_err(&pdev->dev,
+			"Cannot re-enable PCI device after reset.\n");
 		goto done;
 	}
 
@@ -17835,7 +17838,7 @@  static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev)
 	pci_restore_state(pdev);
 	pci_save_state(pdev);
 
-	if (!netif_running(netdev)) {
+	if (!netdev || !netif_running(netdev)) {
 		rc = PCI_ERS_RESULT_RECOVERED;
 		goto done;
 	}
@@ -17847,7 +17850,7 @@  static pci_ers_result_t tg3_io_slot_reset(struct pci_dev *pdev)
 	rc = PCI_ERS_RESULT_RECOVERED;
 
 done:
-	if (rc != PCI_ERS_RESULT_RECOVERED && netif_running(netdev)) {
+	if (rc != PCI_ERS_RESULT_RECOVERED && netdev && netif_running(netdev)) {
 		tg3_napi_enable(tp);
 		dev_close(netdev);
 	}