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

[Stephane Grosjean <s.grosjean@peak-system.com>]

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
--