From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doug Goldstein Subject: Re: [PATCH] xen-pciback: fix up cleanup path when alloc fails Date: Wed, 2 Dec 2015 08:56:01 -0600 Message-ID: <565F0681.8070905__44101.3251817369$1449068401$gmane$org@cardoe.com> References: <1448569959-7245-1-git-send-email-cardoe@cardoe.com> <565EC964.3020006@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6470696982656521601==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a48rf-0006NR-9V for xen-devel@lists.xenproject.org; Wed, 02 Dec 2015 14:58:39 +0000 Received: by ykdr82 with SMTP id r82so48998831ykd.3 for ; Wed, 02 Dec 2015 06:58:36 -0800 (PST) In-Reply-To: <565EC964.3020006@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , xen-devel@lists.xenproject.org Cc: Wei Liu , Jonathan Creekmore , linux-kernel@vger.kernel.org, Paul Durrant , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============6470696982656521601== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="DdvvmTIc9kECORdVsU756B9nqdHFbDol2" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --DdvvmTIc9kECORdVsU756B9nqdHFbDol2 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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. >=20 > We should not require clearing drvdata for correctness. We should > ensure we retain drvdata for as long as it is needed. >=20 > I note that pcistub_device_release() has: >=20 > kfree(dev_data); > pci_set_drvdata(dev, NULL); >=20 > /* Clean-up the device */ > xen_pcibk_config_free_dyn_fields(dev); > xen_pcibk_config_free_dev(dev); >=20 > Which should (at a minimum) be reordered to move the kfree(dev_data) to= > after the calls that require it >=20 > David >=20 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. --=20 Doug Goldstein --DdvvmTIc9kECORdVsU756B9nqdHFbDol2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0 iQJ8BAEBCgBmBQJWXwaEXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRBNTM5MEQ2RTNFMTkyNzlCNzVDMzIwOTVB MkJDMDNEQzg3RUQxQkQ0AAoJEKK8A9yH7RvU0CkP/RSlBy5Mw94bvIjVwIyD5TGP Sq7qyxa5XspH8mXtdd0xUSlg7P2ClwwrQSdiW8pLEqwQiZrlAQJzziBYTkucbetq lm1MAQcOIXNfZTYars+RHdrqVnJAoyHrWkGP0SmIDvjIOE7ZTyjKSKuuhlrROB2/ S8qbZ3huu9b34itMXknXdtGZEKR3ChI8znA4zlk0NhFc4KLyn6RH1xbYcw9kPkcI zYs1nBaJ9cc4P9hRC4Ry5oEfa0p+KVF0vtlrpYUNqNl6/bZzvYzyZluZRyufgPN9 V3UdUVx61uRchF5S9XbbN3Fiogl7Uz7jU5DYpba2dtOFjYwUwQ3fxlvHbL41QgR5 I2zS2gj9Foo0hzHlt/LYV/VkRQY9//cIzig5pUrVXTbs4WYuq/D4ZxonmDLjfjtG HmLavHLVCZd7IpUwsUc1WPrvJdvBvE7GLAYRrTMPXMGoTud6ghR5efSfX39yYVrv bXw1d7z3YDgISmdx7CcfhCM7WLiUz7ypuVo1Vb0475/WkudO7dTAo7fHXbVlK61M gkl97cnKMxHKk9hjm9QHRCEV1qmciICIdUx24RMZ1WkiqNdDZSLzGTYSwBnDU+rf 6vQ5sWnzzM7Co9t1QMDoEvwFQ0kzQol/HPrOo8KEODQf/p85Eee/nCUOpXMldTy+ 4A9F+R2V38xUoCHAH151 =Em02 -----END PGP SIGNATURE----- --DdvvmTIc9kECORdVsU756B9nqdHFbDol2-- --===============6470696982656521601== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6470696982656521601==--