Messages in this thread | | | Subject | Re: [PATCH] rapidio: fix the missed put_device() for rio_mport_add_riodev | From | Jing Xiangfeng <> | Date | Tue, 22 Sep 2020 17:19:06 +0800 |
| |
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@oracle.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 > . >
| |