linux-kernel - RE: [PATCH] vfio/mdev: fix possible memory leak in module init funcs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN9PR11MB5276BC0B7E656465950E3A558C149@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Thu, 1 Dec 2022 02:00:57 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Alex Williamson <alex.williamson@...hat.com>,
        ruanjinjie <ruanjinjie@...wei.com>
CC:     "kwankhede@...dia.com" <kwankhede@...dia.com>,
        "kraxel@...hat.com" <kraxel@...hat.com>,
        "cjia@...dia.com" <cjia@...dia.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "akrowiak@...ux.ibm.com" <akrowiak@...ux.ibm.com>,
        "pasic@...ux.ibm.com" <pasic@...ux.ibm.com>,
        "jjherne@...ux.ibm.com" <jjherne@...ux.ibm.com>,
        "farman@...ux.ibm.com" <farman@...ux.ibm.com>,
        "mjrosato@...ux.ibm.com" <mjrosato@...ux.ibm.com>
Subject: RE: [PATCH] vfio/mdev: fix possible memory leak in module init funcs

> From: Alex Williamson <alex.williamson@...hat.com>
> Sent: Thursday, December 1, 2022 7:06 AM
> 
> [Cc +vfio-ap, vfio-ccw]
> 
> On Fri, 18 Nov 2022 11:28:27 +0800
> ruanjinjie <ruanjinjie@...wei.com> wrote:
> 
> > Inject fault while probing module, if device_register() fails,
> > but the refcount of kobject is not decreased to 0, the name
> > allocated in dev_set_name() is leaked. Fix this by calling
> > put_device(), so that name can be freed in callback function
> > kobject_cleanup().

It's not just about the name. The problem of kboject not being
released is a bigger one.

put_device() is always required no matter device_register()
succeeds or not:

* NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.
 */
int device_register(struct device *dev)

> > @@ -1430,8 +1430,10 @@ static int __init mbochs_dev_init(void)
> >  	dev_set_name(&mbochs_dev, "%s", MBOCHS_NAME);
> >
> >  	ret = device_register(&mbochs_dev);
> > -	if (ret)
> > +	if (ret) {
> > +		put_device(&mbochs_dev);
> >  		goto err_class;
> > +	}
> >
> >  	ret = mdev_register_parent(&mbochs_parent, &mbochs_dev,
> &mbochs_driver,
> >  				   mbochs_mdev_types,
> 
> 
> vfio-ap has a similar unwind as the sample drivers, but actually makes
> an attempt to catch this ex:
> 
> 	...
>         ret = device_register(&matrix_dev->device);
>         if (ret)
>                 goto matrix_reg_err;
> 
>         ret = driver_register(&matrix_driver);
>         if (ret)
>                 goto matrix_drv_err;
> 
>         return 0;
> 
> matrix_drv_err:
>         device_unregister(&matrix_dev->device);
> matrix_reg_err:
>         put_device(&matrix_dev->device);
> 	...
> 
> So of the vfio drivers calling device_register(), vfio-ap is the only
> one that does a put_device() if device_register() fails, but it also
> seems sketchy to call both device_unregister() and put_device() in the
> case that we exit via matrix_drv_err.
> 
> I wonder if all of these shouldn't adopt a flow like:
> 
> 	ret = device_register(&dev);
> 	if (ret)
> 		goto err1;
> 
> 	....
> 
> 	return 0;
> 
> err2:
> 	device_del(&dev);
> err1:
> 	put_device(&dev);
> 

It's kind of a mixed model.

With above unwind it's clearer to use device_initialize() and device_add() instead.

Otherwise what this patch does looks better IMHO:

	ret = device_register(&dev);
	if (ret) {
		put_device(&dev);
		goto err1;
	}

	...

	return 0;

err2:
	device_unregister(&dev);
err1:
	earlier_unwind();