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] [day] [month] [year] [list]

Message-ID: <20200922095255.GC18329@kadam>
Date:   Tue, 22 Sep 2020 12:52:55 +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 05:19:06PM +0800, Jing Xiangfeng wrote:
> 
> 
> 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;
> +                       }

To be honest, I'm not sure how the rio_get_comptag() stuff is supposed
to work...  It probably requires some thought and testing.

Anyway, your patch is straight forward enough so let's just merge that
and hope someone with the hardware can take a look at this.

regards,
dan carpenter