Re: [PATCH] xen-pciback: fix up cleanup path when alloc fails

From: Doug Goldstein <cardoe@cardoe.com>
To: David Vrabel <david.vrabel@citrix.com>, xen-devel@lists.xenproject.org
Cc: Wei Liu <wei.liu2@citrix.com>,
	Jonathan Creekmore <jonathan.creekmore@gmail.com>,
	linux-kernel@vger.kernel.org,
	Paul Durrant <paul.durrant@citrix.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH] xen-pciback: fix up cleanup path when alloc fails
Date: Wed, 2 Dec 2015 08:56:01 -0600	[thread overview]
Message-ID: <565F0681.8070905__44101.3251817369$1449068401$gmane$org@cardoe.com> (raw)
In-Reply-To: <565EC964.3020006@citrix.com>

[-- Attachment #1.1: Type: text/plain, Size: 1788 bytes --]

On 12/2/15 4:35 AM, David Vrabel wrote:
> On 26/11/15 20:32, Doug Goldstein wrote:
>> When allocating a pciback device fails, avoid the possibility of a
>> use after free.
> 
> We should not require clearing drvdata for correctness.  We should
> ensure we retain drvdata for as long as it is needed.
> 
> I note that pcistub_device_release() has:
> 
> 	kfree(dev_data);
> 	pci_set_drvdata(dev, NULL);
> 
> 	/* Clean-up the device */
> 	xen_pcibk_config_free_dyn_fields(dev);
> 	xen_pcibk_config_free_dev(dev);
> 
> Which should (at a minimum) be reordered to move the kfree(dev_data) to
> after the calls that require it
> 
> David
> 

I apologize but at this point I'm confused at what action I should be
taking. Are you saying NACK to the original patch and suggesting this as
the replacement? Or saying that this should be done in addition to the
original patch?

I created the original patch when looking through the other probe()
calls and seeing that they all did pci_set_drvdata() with memory they
allocated but probe() failed they ensured that pci_set_drvdata() was
cleared. But the behavior in xen-pciback was different. It kfree()'d the
memory that passed to pci_set_drvdata() and never set that pointer to
NULL. Which could possibly result in a use after free. The use after
free doesn't occur today as Konrad pointed out but in the future its
possible should some other code changes occur. It was more of a
defensive coding patch in the end. I had planned on resubmitting the
patch with a reworded commit message after Konrad pointed out there was
currently no use after free and retaining the Reviewed-By since the code
wouldn't change but if that's not what I should be doing I will gladly
go another route.

-- 
Doug Goldstein

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel