Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758628AbbLBO6i (ORCPT ); Wed, 2 Dec 2015 09:58:38 -0500 Received: from mail-yk0-f176.google.com ([209.85.160.176]:36463 "EHLO mail-yk0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755946AbbLBO6h (ORCPT ); Wed, 2 Dec 2015 09:58:37 -0500 Subject: Re: [PATCH] xen-pciback: fix up cleanup path when alloc fails To: David Vrabel , xen-devel@lists.xenproject.org References: <1448569959-7245-1-git-send-email-cardoe@cardoe.com> <565EC964.3020006@citrix.com> Cc: linux-kernel@vger.kernel.org, Bob Liu , Paul Durrant , Wei Liu , Boris Ostrovsky , Konrad Rzeszutek Wilk , Jonathan Creekmore From: Doug Goldstein X-Enigmail-Draft-Status: N1110 Message-ID: <565F0681.8070905@cardoe.com> Date: Wed, 2 Dec 2015 08:56:01 -0600 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <565EC964.3020006@citrix.com> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="DdvvmTIc9kECORdVsU756B9nqdHFbDol2" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3396 Lines: 83 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-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/