> -----Original Message----- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Tuesday, January 08, 2019 12:13 AM > To: liujian (CE) <liujia...@huawei.com> > Cc: michal.si...@xilinx.com; hamish.mar...@alliedtelesis.co.nz; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] driver: uio: fix possible memory leak and > use-after-free > in __uio_register_device > > On Fri, Jan 04, 2019 at 10:19:08PM +0800, liujian wrote: > > 'idev' is malloced in __uio_register_device() and leak free it before > > leaving from the uio_get_minor() error handing case, it will cause > > memory leak. > > > > Also, in uio_dev_add_attributes() error handing case, idev is used > > after device_unregister(), in which 'idev' has been released, touch > > idev cause use-after-free. > > > > Fixes: a93e7b331568 ("uio: Prevent device destruction while fds are > > open") > > Fixes: e6789cd3dfb5 ("uio: Simplify uio error path by using devres > > functions") > > Signed-off-by: liujian <liujia...@huawei.com> > > Reviewed-by: Hamish Martin <hamish.mar...@alliedtelesis.co.nz> > > --- > > v1->v2: > > change git log and fix code > > > > drivers/uio/uio.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index > > 1313422..be2a943 100644 > > --- a/drivers/uio/uio.c > > +++ b/drivers/uio/uio.c > > @@ -940,9 +940,12 @@ int __uio_register_device(struct module *owner, > > atomic_set(&idev->event, 0); > > > > ret = uio_get_minor(idev); > > - if (ret) > > + if (ret) { > > + kfree(idev); > > return ret; > > + } > > > > + device_initialize(&idev->dev); > > idev->dev.devt = MKDEV(uio_major, idev->minor); > > idev->dev.class = &uio_class; > > idev->dev.parent = parent; > > @@ -953,7 +956,7 @@ int __uio_register_device(struct module *owner, > > if (ret) > > goto err_device_create; > > > > - ret = device_register(&idev->dev); > > + ret = device_add(&idev->dev); > > if (ret) > > goto err_device_create; > > > > @@ -985,9 +988,10 @@ int __uio_register_device(struct module *owner, > > err_request_irq: > > uio_dev_del_attributes(idev); > > err_uio_dev_add_attributes: > > - device_unregister(&idev->dev); > > + device_del(&idev->dev); > > err_device_create: > > uio_free_minor(idev); > > + put_device(&idev->dev); > > device_del() and then put_device()? I don't think that's a correct error > cleanup path do you? > In err_uio_dev_add_attributes error handling case, device_unregister() will call put_device(), and idev will be released in put_device() Then in err_device_create error handling case, uio_free_minor() will trigger use-after-free
Now, divide device_register() into device_initialize() and device_add(), and divide device_unregister() into device_del() and put_device(). Call uio_free_minor() and then call put_device() to avoid use-after-free > Please fix one thing at a time here also, this should be a a patch series, > right? > Yes, I will do, thanks~ Best Regards, Liujian > thanks, > > greg k-h
