Re: [PATCH] rapidio: fix the missed put_device() for rio_mport_add_riodev

[Jing Xiangfeng]

On 2020/9/22 16:04, Dan Carpenter wrote:
> On Tue, Sep 22, 2020 at 03:25:25PM +0800, Jing Xiangfeng wrote:
> > rio_mport_add_riodev() misses to call put_device() when the device
> > already exists. Add the missed function call to fix it.
> Looks good.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> I notice that rio_mport_del_riodev() has a related bug.
>
>    1802          err = rio_add_device(rdev);
>    1803          if (err)
>    1804                  goto cleanup;
>    1805          rio_dev_get(rdev);
>                  ^^^^^^^^^^^^^^^^^
> This calls get_device(&rdev->dev);
>
>    1806
>    1807          return 0;
>    1808  cleanup:
>    1809          kfree(rdev);
>    1810          return err;
>    1811  }
>    1812
>    1813  static int rio_mport_del_riodev(struct mport_cdev_priv *priv, void __user *arg)
>    1814  {
>    1815          struct rio_rdev_info dev_info;
>    1816          struct rio_dev *rdev = NULL;
>    1817          struct device  *dev;
>    1818          struct rio_mport *mport;
>    1819          struct rio_net *net;
>    1820
>    1821          if (copy_from_user(&dev_info, arg, sizeof(dev_info)))
>    1822                  return -EFAULT;
>    1823          dev_info.name[sizeof(dev_info.name) - 1] = '\0';
>    1824
>    1825          mport = priv->md->mport;
>    1826
>    1827          /* If device name is specified, removal by name has priority */
>    1828          if (strlen(dev_info.name)) {
>    1829                  dev = bus_find_device_by_name(&rio_bus_type, NULL,
>    1830                                                dev_info.name);
>    1831                  if (dev)
>    1832                          rdev = to_rio_dev(dev);
>
> This path takes a second get_device(&rdev->dev);
>
>    1833          } else {
>    1834                  do {
>    1835                          rdev = rio_get_comptag(dev_info.comptag, rdev);
>    1836                          if (rdev && rdev->dev.parent == &mport->net->dev &&
>    1837                              rdev->destid == dev_info.destid &&
>    1838                              rdev->hopcount == dev_info.hopcount)
>    1839                                  break;
>
> This path does not call get_device().
 Add  calling rio_dev_get()  in this path? like the following changes:

 static int rio_mport_del_riodev(struct mport_cdev_priv *priv, void 
__user *arg)
                        rdev = rio_get_comptag(dev_info.comptag, rdev);
                        if (rdev && rdev->dev.parent == &mport->net->dev &&
                            rdev->destid == dev_info.destid &&
-                           rdev->hopcount == dev_info.hopcount)
+                           rdev->hopcount == dev_info.hopcount) {
+                               rio_dev_get(rdev);
                                break;
+                       }
                } while (rdev);
        }

>    1840                  } while (rdev);
>    1841          }
>    1842
>    1843          if (!rdev) {
>    1844                  rmcd_debug(RDEV,
>    1845                          "device name:%s ct:0x%x did:0x%x hc:0x%x not found",
>    1846                          dev_info.name, dev_info.comptag, dev_info.destid,
>    1847                          dev_info.hopcount);
>    1848                  return -ENODEV;
>    1849          }
>    1850
>    1851          net = rdev->net;
>    1852          rio_dev_put(rdev);
>
> This drops a reference.
>
>    1853          rio_del_device(rdev, RIO_DEVICE_SHUTDOWN);
>
> This drops a second reference.  So presumably deleting by component tag
> will lead to a use after free.
Indeed,  it  is a mismatched reference.
>    1854
>    1855          if (list_empty(&net->devices)) {
>    1856                  rio_free_net(net);
>    1857                  mport->net = NULL;
>    1858          }
>    1859
>    1860          return 0;
>    1861  }
>
> regards,
> dan carpenter
> .