linux-kernel - Re: [PATCH] rapidio: fix the missed put_device() for rio_mport_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200922080426.GB18329@kadam>
Date:   Tue, 22 Sep 2020 11:04:26 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jing Xiangfeng <jingxiangfeng@...wei.com>
Cc:     mporter@...nel.crashing.org, alex.bou9@...il.com,
        akpm@...ux-foundation.org, gustavoars@...nel.org,
        jhubbard@...dia.com, keescook@...omium.org,
        madhuparnabhowmik10@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rapidio: fix the missed put_device() for
 rio_mport_add_riodev

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@...cle.com>

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().

  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.

  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