[PATCH] peak_pci: fix use after free in netdev teardown

[PATCH] peak_pci: fix use after free in netdev teardown

[Christopher R. Baker]

[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]

Hi All,

In the course of tracking down (and eventually backporting) a fix to one
of my systems that is still running a 3.2 kernel, I noticed what I
believe to be a pair of use-after-free bugs in peak_pci.c pertaining to
the linked list of netdevs that is maintained for multi-port cards.
These bugs persist in 3.14, so I thought I should send along a patch for
review.

Basically, the "prev_dev" pointer that is used for this list lives in
memory that is allocated by alloc_sja1000dev(), but it is referenced
after the call to free_sja1000dev() when walking the list during
teardown, both in the failure case of peak_pci_probe and in
peak_pci_remove.  Unless I'm missing something, these are toes waiting
to be stubbed...

Caveats:
 - This is a growing blob of copy-pasta that should probably be
refactored to a common location.  For example, peak_pci_remove could be
restructured to incrementally check and free allocated resources,
allowing the "failure_remove_channels" label to delegate the cleanup to
peak_pci_remove.  I didn't want to bite off too much this time, though,
so I left that alone.
 - I don't have an expresscard adapter to check the placement of the
pciec_remove stanzas.  By inspection, unregister_sja1000dev does not
appear to have a path back to the pciec stuff, but I may have missed
something.

-ChrisR

Signed-of-by: Christopher R. Baker <cbaker@rec.ri.cmu.edu>



[-- Attachment #2: peak_pci_cleanup_use_after_free.diff --]
[-- Type: text/x-patch, Size: 2794 bytes --]

diff --git a/drivers/net/can/sja1000/peak_pci.c b/drivers/net/can/sja1000/peak_pci.c
index 065ca49..4ff33df 100644
--- a/drivers/net/can/sja1000/peak_pci.c
+++ b/drivers/net/can/sja1000/peak_pci.c
@@ -686,17 +686,28 @@ failure_remove_channels:
 	/* Disable interrupts */
 	writew(0x0, cfg_base + PITA_ICR + 2);
 
-	chan = NULL;
-	for (dev = pci_get_drvdata(pdev); dev; dev = chan->prev_dev) {
-		unregister_sja1000dev(dev);
-		free_sja1000dev(dev);
+	/* Dealloc netdevs: notably replicated near-verbatim in peak_pci_remove */
+	dev = pci_get_drvdata(pdev);
+	while(dev)
+	{
+		/* priv and chan look into memory that is allocated by alloc_sja1000dev,
+		   so be careful to cache prev_dev before free_sja1000dev */
+		struct net_device *prev_dev;
 		priv = netdev_priv(dev);
 		chan = priv->priv;
+		prev_dev = chan->prev_dev;
+
+		/* free PCIeC resources for the first card, if present, and before free_sja1000dev. */
+		if (!prev_dev && chan->pciec_card)
+			peak_pciec_remove(chan->pciec_card);
+
+		unregister_sja1000dev(dev);
+		free_sja1000dev(dev);
+
+		dev = prev_dev;
 	}
+	pci_set_drvdata(pdev,NULL);
 
-	/* free any PCIeC resources too */
-	if (chan && chan->pciec_card)
-		peak_pciec_remove(chan->pciec_card);
 
 	pci_iounmap(pdev, reg_base);
 
@@ -717,28 +728,35 @@ static void peak_pci_remove(struct pci_dev *pdev)
 	struct net_device *dev = pci_get_drvdata(pdev); /* Last device */
 	struct sja1000_priv *priv = netdev_priv(dev);
 	struct peak_pci_chan *chan = priv->priv;
+
 	void __iomem *cfg_base = chan->cfg_base;
 	void __iomem *reg_base = priv->reg_base;
 
 	/* Disable interrupts */
 	writew(0x0, cfg_base + PITA_ICR + 2);
 
-	/* Loop over all registered devices */
-	while (1) {
-		dev_info(&pdev->dev, "removing device %s\n", dev->name);
+	/* Dealloc netdevs: notably replicated from above */
+	while(dev)
+	{
+		/* priv and chan look into memory that is allocated by alloc_sja1000dev,
+		   so be careful to cache prev_dev before free_sja1000dev */
+		struct net_device *prev_dev;
+		priv = netdev_priv(dev);
+		chan = priv->priv;
+		prev_dev = chan->prev_dev;
+		dev_info(&pdev->dev, "removing device '%s' at %p (prev_dev = %p, chan->pciec_card = %p)\n",
+						dev->name, dev, prev_dev,chan->pciec_card);
+
+		/* free PCIeC resources for the first card, if present, and before free_sja1000dev. */
+		if (!prev_dev && chan->pciec_card)
+			peak_pciec_remove(chan->pciec_card);
+
 		unregister_sja1000dev(dev);
 		free_sja1000dev(dev);
-		dev = chan->prev_dev;
 
-		if (!dev) {
-			/* do that only for first channel */
-			if (chan->pciec_card)
-				peak_pciec_remove(chan->pciec_card);
-			break;
-		}
-		priv = netdev_priv(dev);
-		chan = priv->priv;
+		dev = prev_dev;
 	}
+	pci_set_drvdata(pdev,NULL);
 
 	pci_iounmap(pdev, reg_base);
 	pci_iounmap(pdev, cfg_base);

Re: [PATCH] peak_pci: fix use after free in netdev teardown

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

On 04/09/2014 09:45 PM, Christopher R. Baker wrote:
> Hi All,
> 
> In the course of tracking down (and eventually backporting) a fix to one
> of my systems that is still running a 3.2 kernel, I noticed what I
> believe to be a pair of use-after-free bugs in peak_pci.c pertaining to
> the linked list of netdevs that is maintained for multi-port cards.
> These bugs persist in 3.14, so I thought I should send along a patch for
> review.
> 
> Basically, the "prev_dev" pointer that is used for this list lives in
> memory that is allocated by alloc_sja1000dev(), but it is referenced
> after the call to free_sja1000dev() when walking the list during
> teardown, both in the failure case of peak_pci_probe and in
> peak_pci_remove.  Unless I'm missing something, these are toes waiting
> to be stubbed...
> 
> Caveats:
>  - This is a growing blob of copy-pasta that should probably be
> refactored to a common location.  For example, peak_pci_remove could be
> restructured to incrementally check and free allocated resources,
> allowing the "failure_remove_channels" label to delegate the cleanup to
> peak_pci_remove.  I didn't want to bite off too much this time, though,
> so I left that alone.
>  - I don't have an expresscard adapter to check the placement of the
> pciec_remove stanzas.  By inspection, unregister_sja1000dev does not
> appear to have a path back to the pciec stuff, but I may have missed
> something.
> 
> -ChrisR
> 
> Signed-of-by: Christopher R. Baker <cbaker@rec.ri.cmu.edu>

Stephane, can you please have a look at the patch.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH] peak_pci: fix use after free in netdev teardown

[Stephane Grosjean]

Hi Marc,

Certainly I'd like... but didn't get that patch at all ! :-) Could you 
please forward it to me ?

Thanks and regards,

Stéphane

Le 19/05/2014 09:11, Marc Kleine-Budde a écrit :
> On 04/09/2014 09:45 PM, Christopher R. Baker wrote:
>> Hi All,
>>
>> In the course of tracking down (and eventually backporting) a fix to one
>> of my systems that is still running a 3.2 kernel, I noticed what I
>> believe to be a pair of use-after-free bugs in peak_pci.c pertaining to
>> the linked list of netdevs that is maintained for multi-port cards.
>> These bugs persist in 3.14, so I thought I should send along a patch for
>> review.
>>
>> Basically, the "prev_dev" pointer that is used for this list lives in
>> memory that is allocated by alloc_sja1000dev(), but it is referenced
>> after the call to free_sja1000dev() when walking the list during
>> teardown, both in the failure case of peak_pci_probe and in
>> peak_pci_remove.  Unless I'm missing something, these are toes waiting
>> to be stubbed...
>>
>> Caveats:
>>   - This is a growing blob of copy-pasta that should probably be
>> refactored to a common location.  For example, peak_pci_remove could be
>> restructured to incrementally check and free allocated resources,
>> allowing the "failure_remove_channels" label to delegate the cleanup to
>> peak_pci_remove.  I didn't want to bite off too much this time, though,
>> so I left that alone.
>>   - I don't have an expresscard adapter to check the placement of the
>> pciec_remove stanzas.  By inspection, unregister_sja1000dev does not
>> appear to have a path back to the pciec stuff, but I may have missed
>> something.
>>
>> -ChrisR
>>
>> Signed-of-by: Christopher R. Baker <cbaker@rec.ri.cmu.edu>
> Stephane, can you please have a look at the patch.
>
> Marc
>

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com
--

Re: [PATCH] peak_pci: fix use after free in netdev teardown

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

On 05/19/2014 09:16 AM, Stephane Grosjean wrote:
> Hi Marc,
> 
> Certainly I'd like... but didn't get that patch at all ! :-) Could you
> please forward it to me ?

I've bounced the original mail (including the patch). It's from
04/09/2014 09:45 PM.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH] peak_pci: fix use after free in netdev teardown

[Stephane Grosjean]

Hi All,

So I finally had a look to the diff file. It clearly does what 
Christopher says, that is, it fixes some memory access order issues when 
the CAN devices have to be removed from the system, either when probing 
has failed or when the driver is unloaded from the system.

The diff file has to be re-written into a Linux-coding style patch, and 
yes, maybe all this removing stuff could be put into a (new) single 
function made for that.

So, how do we proceed, please ?

Regards,

Stéphane

Le 19/05/2014 09:11, Marc Kleine-Budde a écrit :
> On 04/09/2014 09:45 PM, Christopher R. Baker wrote:
>> Hi All,
>>
>> In the course of tracking down (and eventually backporting) a fix to one
>> of my systems that is still running a 3.2 kernel, I noticed what I
>> believe to be a pair of use-after-free bugs in peak_pci.c pertaining to
>> the linked list of netdevs that is maintained for multi-port cards.
>> These bugs persist in 3.14, so I thought I should send along a patch for
>> review.
>>
>> Basically, the "prev_dev" pointer that is used for this list lives in
>> memory that is allocated by alloc_sja1000dev(), but it is referenced
>> after the call to free_sja1000dev() when walking the list during
>> teardown, both in the failure case of peak_pci_probe and in
>> peak_pci_remove.  Unless I'm missing something, these are toes waiting
>> to be stubbed...
>>
>> Caveats:
>>   - This is a growing blob of copy-pasta that should probably be
>> refactored to a common location.  For example, peak_pci_remove could be
>> restructured to incrementally check and free allocated resources,
>> allowing the "failure_remove_channels" label to delegate the cleanup to
>> peak_pci_remove.  I didn't want to bite off too much this time, though,
>> so I left that alone.
>>   - I don't have an expresscard adapter to check the placement of the
>> pciec_remove stanzas.  By inspection, unregister_sja1000dev does not
>> appear to have a path back to the pciec stuff, but I may have missed
>> something.
>>
>> -ChrisR
>>
>> Signed-of-by: Christopher R. Baker <cbaker@rec.ri.cmu.edu>
> Stephane, can you please have a look at the patch.
>
> Marc
>

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com
--

Re: [PATCH] peak_pci: fix use after free in netdev teardown

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On 05/19/2014 11:14 AM, Stephane Grosjean wrote:
> Hi All,
> 
> So I finally had a look to the diff file. It clearly does what
> Christopher says, that is, it fixes some memory access order issues when
> the CAN devices have to be removed from the system, either when probing
> has failed or when the driver is unloaded from the system.
> 
> The diff file has to be re-written into a Linux-coding style patch, and
> yes, maybe all this removing stuff could be put into a (new) single
> function made for that.
> 
> So, how do we proceed, please ?

Make it so.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH] peak_pci: fix use after free in netdev teardown

[Stephane Grosjean]

Hi again,

After having a deeper look to the code, it looks like we don't actually 
need all of these modifications.

It's ok that some pointers have to be saved BEFORE calling 
free_sja1000dev(), but I checked the other things more deeply:

- peak_pciec_remove() calls don't need to be moved: this function 
clearly doesn't deal with any netdev neither candev stuff. So, it 
doesn't actually need to be called before free_sja1000dev().

- IMHO, no need to restore "pci_set_drvdata(pdev,NULL);" before 
returning an errno status from the _probe() function (neither from 
peak_pci_remove()): the pci device object won't be used next (AFAIK).

- same in peak_pci_remove(): the prev link has to be saved before 
freeing the current candev object.

So I assume that Christopher's fix could be summarized to:

--- a/drivers/net/can/sja1000/peak_pci.c
+++ b/drivers/net/can/sja1000/peak_pci.c
@@ -551,7 +551,7 @@ static int peak_pci_probe(struct pci_dev *pdev, 
const struct
  {
         struct sja1000_priv *priv;
         struct peak_pci_chan *chan;
-       struct net_device *dev;
+       struct net_device *dev, *prev_dev;
         void __iomem *cfg_base, *reg_base;
         u16 sub_sys_id, icr;
         int i, err, channels;
@@ -688,11 +688,13 @@ failure_remove_channels:
         writew(0x0, cfg_base + PITA_ICR + 2);

         chan = NULL;
-       for (dev = pci_get_drvdata(pdev); dev; dev = chan->prev_dev) {
-               unregister_sja1000dev(dev);
-               free_sja1000dev(dev);
+       for (dev = pci_get_drvdata(pdev); dev; dev = prev_dev) {
                 priv = netdev_priv(dev);
                 chan = priv->priv;
+               prev_dev = chan->prev_dev;
+
+               unregister_sja1000dev(dev);
+               free_sja1000dev(dev);
         }

         /* free any PCIeC resources too */
@@ -726,10 +728,12 @@ static void peak_pci_remove(struct pci_dev *pdev)

         /* Loop over all registered devices */
         while (1) {
+               struct net_device *prev_dev = chan->prev_dev;
+
                 dev_info(&pdev->dev, "removing device %s\n", dev->name);
                 unregister_sja1000dev(dev);
                 free_sja1000dev(dev);
-               dev = chan->prev_dev;
+               dev = prev_dev;

                 if (!dev) {
                         /* do that only for first channel */

If ok, I'll push a well formatted patch asap, as soon as I have finished 
with building my local linux-can-next and have tested these changes.

Regards,

Stéphane

Le 19/05/2014 11:20, Marc Kleine-Budde a écrit :
> On 05/19/2014 11:14 AM, Stephane Grosjean wrote:
>> Hi All,
>>
>> So I finally had a look to the diff file. It clearly does what
>> Christopher says, that is, it fixes some memory access order issues when
>> the CAN devices have to be removed from the system, either when probing
>> has failed or when the driver is unloaded from the system.
>>
>> The diff file has to be re-written into a Linux-coding style patch, and
>> yes, maybe all this removing stuff could be put into a (new) single
>> function made for that.
>>
>> So, how do we proceed, please ?
> Make it so.
>
> Marc
>

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com
--