On Tue, 20 Jul 2021 19:49:55 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Tue, Jul 20, 2021 at 04:01:27PM -0600, Alex Williamson wrote:
> > On Tue, 20 Jul 2021 14:42:48 -0300
> > Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> >
> > > Compared to mbochs_remove() two cases are missing from the
> > > vfio_register_group_dev() unwind. Add them in.
> > >
> > > Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()")
> > > Reported-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > samples/vfio-mdev/mbochs.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> > > index e81b875b4d87b4..501845b08c0974 100644
> > > +++ b/samples/vfio-mdev/mbochs.c
> > > @@ -553,11 +553,14 @@ static int mbochs_probe(struct mdev_device *mdev)
> > >
> > > ret = vfio_register_group_dev(&mdev_state->vdev);
> > > if (ret)
> > > - goto err_mem;
> > > + goto err_bytes;
> > > dev_set_drvdata(&mdev->dev, mdev_state);
> > > return 0;
> > >
> > > +err_bytes:
> > > + mbochs_used_mbytes -= mdev_state->type->mbytes;
> > > err_mem:
> > > + kfree(mdev_state->pages);
> > > kfree(mdev_state->vconfig);
> > > kfree(mdev_state);
> > > return ret;
> > > @@ -567,8 +570,8 @@ static void mbochs_remove(struct mdev_device *mdev)
> > > {
> > > struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
> > >
> > > - mbochs_used_mbytes -= mdev_state->type->mbytes;
> > > vfio_unregister_group_dev(&mdev_state->vdev);
> > > + mbochs_used_mbytes -= mdev_state->type->mbytes;
> > > kfree(mdev_state->pages);
> > > kfree(mdev_state->vconfig);
> > > kfree(mdev_state);
> >
> > Hmm, doesn't this suggest we need another atomic conversion? (untested)
>
> Sure why not, I can add this as another patch
>
> > @@ -567,11 +573,11 @@ static void mbochs_remove(struct mdev_device *mdev)
> > {
> > struct mdev_state *mdev_state = dev_get_drvdata(&mdev->dev);
> >
> > - mbochs_used_mbytes -= mdev_state->type->mbytes;
> > vfio_unregister_group_dev(&mdev_state->vdev);
> > kfree(mdev_state->pages);
> > kfree(mdev_state->vconfig);
> > kfree(mdev_state);
> > + atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes);
>
> This should be up after the vfio_unregister_group_dev(), it is a use after free?
Oops, yep. That or get the mbochs_type so we can mirror the _probe
setup. Same on the _probe unwind, but we've already got type->mbytes
there. Thanks,
Alex
