From patchwork Mon Aug 8 16:28:50 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Julia Lawall X-Patchwork-Id: 108988 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 633E7B6F71 for ; Tue, 9 Aug 2011 02:29:18 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753513Ab1HHQ27 (ORCPT ); Mon, 8 Aug 2011 12:28:59 -0400 Received: from mgw2.diku.dk ([130.225.96.92]:59984 "EHLO mgw2.diku.dk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753309Ab1HHQ26 (ORCPT ); Mon, 8 Aug 2011 12:28:58 -0400 Received: from localhost (localhost [127.0.0.1]) by mgw2.diku.dk (Postfix) with ESMTP id 3C9BA19BB81; Mon, 8 Aug 2011 18:28:57 +0200 (CEST) Received: from mgw2.diku.dk ([127.0.0.1]) by localhost (mgw2.diku.dk [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 05129-08; Mon, 8 Aug 2011 18:28:51 +0200 (CEST) Received: from palace.topps.diku.dk (palace.ekstranet.diku.dk [192.38.115.202]) by mgw2.diku.dk (Postfix) with ESMTP id 1FBB419BBA6; Mon, 8 Aug 2011 18:28:51 +0200 (CEST) From: Julia Lawall To: Wolfgang Grandegger Cc: kernel-janitors@vger.kernel.org, Joe Perches , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] drivers/net/can/sja1000/plx_pci.c: eliminate double free Date: Mon, 8 Aug 2011 18:28:50 +0200 Message-Id: <1312820931-28230-1-git-send-email-julia@diku.dk> X-Mailer: git-send-email 1.7.1 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Julia Lawall In this code, the failure_cleanup label calls the function plx_pci_del_card, which frees everything in the card->net_dev array. dev is placed in this array immediately after allocation, so the two subsequent jumps to failure_cleanup should not also call free_sja1000dev, but the second one does. If plx_pci_check_sja1000 fails, then free_sja1000dev is also called on dev. Because dev is already in the card->net_dev array, this implies that when plx_pci_del_card is later called, it may get freed again. So that entry is reset to NULL after the free. Finally, if there is a problem with one channel, there will be a hole in the array. card->channels counts the number of channels that have succeeded, and does not keep track of the index of the largest element in the array that is valid. So the loop in plx_pci_del_card is changed to go up to PLX_PCI_MAX_CHAN, which is only 2. Signed-off-by: Julia Lawall --- Compiled but not tested. I'm not sure the fix is sufficient to take into account possible failures. In particular, is it safe to call unregister_sja1000dev without previously having (successfully) called register_sja1000dev? drivers/net/can/sja1000/plx_pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 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 --git a/drivers/net/can/sja1000/plx_pci.c b/drivers/net/can/sja1000/plx_pci.c index 231385b..c7f3d4e 100644 --- a/drivers/net/can/sja1000/plx_pci.c +++ b/drivers/net/can/sja1000/plx_pci.c @@ -408,7 +408,7 @@ static void plx_pci_del_card(struct pci_dev *pdev) struct sja1000_priv *priv; int i = 0; - for (i = 0; i < card->channels; i++) { + for (i = 0; i < PLX_PCI_MAX_CHAN; i++) { dev = card->net_dev[i]; if (!dev) continue; @@ -536,7 +536,6 @@ static int __devinit plx_pci_add_card(struct pci_dev *pdev, if (err) { dev_err(&pdev->dev, "Registering device failed " "(err=%d)\n", err); - free_sja1000dev(dev); goto failure_cleanup; } @@ -549,6 +548,7 @@ static int __devinit plx_pci_add_card(struct pci_dev *pdev, dev_err(&pdev->dev, "Channel #%d not detected\n", i + 1); free_sja1000dev(dev); + card->net_dev[i] = NULL; } }