| Message ID | 20220909102247.67324-7-kevin.tian@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tidy up vfio_device life cycle | expand |
On Fri, 9 Sep 2022 18:22:38 +0800 Kevin Tian <kevin.tian@intel.com> wrote: > From: Yi Liu <yi.l.liu@intel.com> > > and manage available ports inside @init/@release. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Kevin Tian <kevin.tian@intel.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- > samples/vfio-mdev/mtty.c | 67 +++++++++++++++++++++++----------------- > 1 file changed, 39 insertions(+), 28 deletions(-) > > diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c > index f42a59ed2e3f..41301d50b247 100644 > --- a/samples/vfio-mdev/mtty.c > +++ b/samples/vfio-mdev/mtty.c ... > +static int mtty_probe(struct mdev_device *mdev) > +{ > + struct mdev_state *mdev_state; > + int ret; > + > + mdev_state = vfio_alloc_device(mdev_state, vdev, &mdev->dev, > + &mtty_dev_ops); > + if (IS_ERR(mdev_state)) > + return PTR_ERR(mdev_state); > > ret = vfio_register_emulated_iommu_dev(&mdev_state->vdev); > if (ret) > - goto err_vconfig; > + goto err_put_vdev; > dev_set_drvdata(&mdev->dev, mdev_state); > return 0; > > -err_vconfig: > - kfree(mdev_state->vconfig); > -err_state: > - vfio_uninit_group_dev(&mdev_state->vdev); > - kfree(mdev_state); > -err_nr_ports: > - atomic_add(nr_ports, &mdev_avail_ports); > +err_put_vdev: > + vfio_put_device(&mdev_state->vdev); > return ret; > } > > +static void mtty_release_dev(struct vfio_device *vdev) > +{ > + struct mdev_state *mdev_state = > + container_of(vdev, struct mdev_state, vdev); > + > + kfree(mdev_state->vconfig); > + vfio_free_device(vdev); > + atomic_add(mdev_state->nr_ports, &mdev_avail_ports); I must be missing something, isn't this a use-after-free? mdev_state is allocated via vfio_alloc_device(), where vdev is the first entry in that structure, so this is equivalent to kvfree(mdev_state). mbochs has the same issue. mdpy and vfio-ap adjust global counters after vfio_free_device(), which I think muddies the situation. Shouldn't we look suspiciously at any .release callback where vfio_free_device() isn't the last thing executed? Thanks, Alex
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Wednesday, September 21, 2022 3:17 AM > > On Fri, 9 Sep 2022 18:22:38 +0800 > Kevin Tian <kevin.tian@intel.com> wrote: > > > From: Yi Liu <yi.l.liu@intel.com> > > > > and manage available ports inside @init/@release. > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > Signed-off-by: Kevin Tian <kevin.tian@intel.com> > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > samples/vfio-mdev/mtty.c | 67 +++++++++++++++++++++++----------------- > > 1 file changed, 39 insertions(+), 28 deletions(-) > > > > diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c > > index f42a59ed2e3f..41301d50b247 100644 > > --- a/samples/vfio-mdev/mtty.c > > +++ b/samples/vfio-mdev/mtty.c > ... > > +static int mtty_probe(struct mdev_device *mdev) > > +{ > > + struct mdev_state *mdev_state; > > + int ret; > > + > > + mdev_state = vfio_alloc_device(mdev_state, vdev, &mdev->dev, > > + &mtty_dev_ops); > > + if (IS_ERR(mdev_state)) > > + return PTR_ERR(mdev_state); > > > > ret = vfio_register_emulated_iommu_dev(&mdev_state->vdev); > > if (ret) > > - goto err_vconfig; > > + goto err_put_vdev; > > dev_set_drvdata(&mdev->dev, mdev_state); > > return 0; > > > > -err_vconfig: > > - kfree(mdev_state->vconfig); > > -err_state: > > - vfio_uninit_group_dev(&mdev_state->vdev); > > - kfree(mdev_state); > > -err_nr_ports: > > - atomic_add(nr_ports, &mdev_avail_ports); > > +err_put_vdev: > > + vfio_put_device(&mdev_state->vdev); > > return ret; > > } > > > > +static void mtty_release_dev(struct vfio_device *vdev) > > +{ > > + struct mdev_state *mdev_state = > > + container_of(vdev, struct mdev_state, vdev); > > + > > + kfree(mdev_state->vconfig); > > + vfio_free_device(vdev); > > + atomic_add(mdev_state->nr_ports, &mdev_avail_ports); > > I must be missing something, isn't this a use-after-free? Yes, it's a use-after-free indeed. Thanks for catching it! > > mdev_state is allocated via vfio_alloc_device(), where vdev is the > first entry in that structure, so this is equivalent to > kvfree(mdev_state). mbochs has the same issue. mdpy and vfio-ap > adjust global counters after vfio_free_device(), which I think muddies > the situation. Shouldn't we look suspiciously at any .release callback > where vfio_free_device() isn't the last thing executed? Thanks, > Yes. I'll scrutinize it again. To be consistent I'll make sure the free is the last line in all .release callbacks though not required for some (e.g. mdpy and vfio-ap).
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index f42a59ed2e3f..41301d50b247 100644 --- a/samples/vfio-mdev/mtty.c +++ b/samples/vfio-mdev/mtty.c @@ -703,9 +703,11 @@ static ssize_t mdev_access(struct mdev_state *mdev_state, u8 *buf, size_t count, return ret; } -static int mtty_probe(struct mdev_device *mdev) +static int mtty_init_dev(struct vfio_device *vdev) { - struct mdev_state *mdev_state; + struct mdev_state *mdev_state = + container_of(vdev, struct mdev_state, vdev); + struct mdev_device *mdev = to_mdev_device(vdev->dev); int nr_ports = mdev_get_type_group_id(mdev) + 1; int avail_ports = atomic_read(&mdev_avail_ports); int ret; @@ -716,58 +718,65 @@ static int mtty_probe(struct mdev_device *mdev) } while (!atomic_try_cmpxchg(&mdev_avail_ports, &avail_ports, avail_ports - nr_ports)); - mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL); - if (mdev_state == NULL) { - ret = -ENOMEM; - goto err_nr_ports; - } - - vfio_init_group_dev(&mdev_state->vdev, &mdev->dev, &mtty_dev_ops); - mdev_state->nr_ports = nr_ports; mdev_state->irq_index = -1; mdev_state->s[0].max_fifo_size = MAX_FIFO_SIZE; mdev_state->s[1].max_fifo_size = MAX_FIFO_SIZE; mutex_init(&mdev_state->rxtx_lock); - mdev_state->vconfig = kzalloc(MTTY_CONFIG_SPACE_SIZE, GFP_KERNEL); - if (mdev_state->vconfig == NULL) { + mdev_state->vconfig = kzalloc(MTTY_CONFIG_SPACE_SIZE, GFP_KERNEL); + if (!mdev_state->vconfig) { ret = -ENOMEM; - goto err_state; + goto err_nr_ports; } mutex_init(&mdev_state->ops_lock); mdev_state->mdev = mdev; - mtty_create_config_space(mdev_state); + return 0; + +err_nr_ports: + atomic_add(nr_ports, &mdev_avail_ports); + return ret; +} + +static int mtty_probe(struct mdev_device *mdev) +{ + struct mdev_state *mdev_state; + int ret; + + mdev_state = vfio_alloc_device(mdev_state, vdev, &mdev->dev, + &mtty_dev_ops); + if (IS_ERR(mdev_state)) + return PTR_ERR(mdev_state); ret = vfio_register_emulated_iommu_dev(&mdev_state->vdev); if (ret) - goto err_vconfig; + goto err_put_vdev; dev_set_drvdata(&mdev->dev, mdev_state); return 0; -err_vconfig: - kfree(mdev_state->vconfig); -err_state: - vfio_uninit_group_dev(&mdev_state->vdev); - kfree(mdev_state); -err_nr_ports: - atomic_add(nr_ports, &mdev_avail_ports); +err_put_vdev: + vfio_put_device(&mdev_state->vdev); return ret; } +static void mtty_release_dev(struct vfio_device *vdev) +{ + struct mdev_state *mdev_state = + container_of(vdev, struct mdev_state, vdev); + + kfree(mdev_state->vconfig); + vfio_free_device(vdev); + atomic_add(mdev_state->nr_ports, &mdev_avail_ports); +} + static void mtty_remove(struct mdev_device *mdev) { struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev); - int nr_ports = mdev_state->nr_ports; vfio_unregister_group_dev(&mdev_state->vdev); - - kfree(mdev_state->vconfig); - vfio_uninit_group_dev(&mdev_state->vdev); - kfree(mdev_state); - atomic_add(nr_ports, &mdev_avail_ports); + vfio_put_device(&mdev_state->vdev); } static int mtty_reset(struct mdev_state *mdev_state) @@ -1287,6 +1296,8 @@ static struct attribute_group *mdev_type_groups[] = { static const struct vfio_device_ops mtty_dev_ops = { .name = "vfio-mtty", + .init = mtty_init_dev, + .release = mtty_release_dev, .read = mtty_read, .write = mtty_write, .ioctl = mtty_ioctl,