rio_mport_add_riodev() misses to call put_device() when the device
already exists. Add the missed function call to fix it.
Fixes: e8de370188d0 ("rapidio: add mport char device driver")
Signed-off-by: Jing Xiangfeng <[email protected]>
---
drivers/rapidio/devices/rio_mport_cdev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c
index a30342942e26..829fe2b7c437 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -1679,6 +1679,7 @@ static int rio_mport_add_riodev(struct mport_cdev_priv *priv,
struct rio_dev *rdev;
struct rio_switch *rswitch = NULL;
struct rio_mport *mport;
+ struct device *dev;
size_t size;
u32 rval;
u32 swpinfo = 0;
@@ -1693,8 +1694,10 @@ static int rio_mport_add_riodev(struct mport_cdev_priv *priv,
rmcd_debug(RDEV, "name:%s ct:0x%x did:0x%x hc:0x%x", dev_info.name,
dev_info.comptag, dev_info.destid, dev_info.hopcount);
- if (bus_find_device_by_name(&rio_bus_type, NULL, dev_info.name)) {
+ dev = bus_find_device_by_name(&rio_bus_type, NULL, dev_info.name);
+ if (dev) {
rmcd_debug(RDEV, "device %s already exists", dev_info.name);
+ put_device(dev);
return -EEXIST;
}
--
2.17.1
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 <[email protected]>
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
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 <[email protected]>
>
> 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
> .
>
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 <[email protected]>
> >
> > 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