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: <5F69C18A.2070800@huawei.com>
Date:   Tue, 22 Sep 2020 17:19:06 +0800
From:   Jing Xiangfeng <jingxiangfeng@...wei.com>
To:     Dan Carpenter <dan.carpenter@...cle.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 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@...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().
  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
> .
>