From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932988AbcE0J56 (ORCPT ); Fri, 27 May 2016 05:57:58 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:56838 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932873AbcE0J55 (ORCPT ); Fri, 27 May 2016 05:57:57 -0400 X-AuditID: cbfee691-f79196d000001483-77-57481a20ff1c Date: Fri, 27 May 2016 09:57:52 +0000 (GMT) From: Chung-Geol Kim Subject: Re: Re: [PATCH] usb: core: fix a double free in the usb driver To: "gregkh@linuxfoundation.org" Cc: "mathias.nyman@linux.intel.com" , "stefan.koch10@gmail.com" , "hkallweit1@gmail.com" , "sergei.shtylyov@cogentembedded.com" , "dan.j.williams@intel.com" , "sarah.a.sharp@linux.intel.com" , "stern@rowland.harvard.edu" , "chris.bainbridge@gmail.com" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Reply-to: chunggeol.kim@samsung.com MIME-version: 1.0 X-MTR: 20160527094817267@chunggeol.kim Msgkey: 20160527094817267@chunggeol.kim X-EPLocale: ko_KR.euc-kr X-Priority: 3 X-EPWebmail-Msg-Type: personal X-EPWebmail-Reply-Demand: 0 X-EPApproval-Locale: X-EPHeader: ML X-MLAttribute: X-RootMTR: 20160527094817267@chunggeol.kim X-ParentMTR: X-ArchiveUser: EV X-CPGSPASS: Y X-ConfirmMail: N,general Content-type: text/plain; charset=euc-kr MIME-version: 1.0 Message-id: <1893879755.217681464343067404.JavaMail.weblogic@epmlwas08d> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrIJsWRmVeSWpSXmKPExsVy+t8zPV0FKY9wg40dahaXd81hc2D0+LxJ LoAxqoHRJiU1J7MstUjfziaxKDkDyFRIzUvOT8nMS7dVCg1x07VQUsjILy6xVYo2MjDWMzI1 0TMyMdezNIi1MjJVUshLzE21VarQhepVUihKLgCqza0sBhqQk6oHFdcrTs1LccjKLwW5Sa84 Mbe4NC9dLzk/VylhPmNGz+4LbAUr7CvmfvJuYLxi28XIySEkoCFxqqmbpYuRg0NCwESi82sh SFhCQEziwr31bF2MXEAlyxglzl9YzwyRMJH4cuQ9I0TvHEaJGb80QWwWAVWJlkfPweJsAoYS q//cB6sXFnCXuPHzLRuILSJgKzFxWTczyFBmgRcsEn3nbrJADFKWmHvnMTuIzSsgKHFy5hMW iGVqEv+XTmaBiKtLfDvcxQQRl5CYNf0CK4TNKzGj/SlUvZzEtK9roA6Vljg/awMjzDeLvz+G ivNLHLu9A2qOgMTUMweharQl7r1+yg5h80msWfgWaqagxOlr3cwwuxo2/maHuWFryxOwG5gF FCWmdD9kh7C1JL782MeG7hdeAQ+JbXdB9nIB9Z7hkPj3bDvjBEalWUjqZiGZNQvJLGQ1CxhZ VjGKphYkFxQnpReZIsfxJkZIIpy4g/H+AetDjOocjFKipXnFyYl5eYlJOanxuYk5aflFuakp Sjy8DoIe4UKsiWXFlbmHGJOB8TeRWUo0OR+YnPNK4g2NzYwsTE1MjY3MLc0whE1MLSxMjHAI K4nz6kj/DBYSSE8sSc1OTS1ILYovKs1JLT7EyMTBKdXAaMRsGZ+fdG09p/kdjZMJV39lR71I Opo4IUIxx7nDoJ7pQFOBB++ip70Si3UmJRps1Lksvm/LlTjFZze2CHsy9qe5P4ooFjW7Jcjh ICP95PR1gXdTLprMtHZQsfeqLP58oPeV5Vbp0x537aV4/eK9t/xSlit6YZzNsI9JIc1L/rty yvVZW48qsRRnJBpqMRcVJwIAPE43bMQDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrKKsWRmVeSWpSXmKPExsVy+t/tPl0FKY9wg2M/FC0u75rD5sDo8XmT XABjVJFNSmpOZllqkb6dTUZqYkpqkUJqXnJ+SmZeuq2Sd3C8c7ypmYGhrqGlhbmSQl5ibqqt kotPgK5bZg7QeCWFssScUqBQQGJxsRLQhKL80pJUhYz84hJbpWgjA2M9I1MTPSNjAz0Tg1gr QwMDI1OgqoSijJ7dF9gKVthXzP3k3cB4xbaLkZNDSEBD4lRTNwuILSFgIvHlyHtGCFtM4sK9 9WwQNXMYJWb80gSxWQRUJVoePQerYRMwlFj95z4ziC0s4C5x4+dbsHoRAVuJicu6geJcHMwC L1gk+s7dZIEYpCwx985jdhCbV0BQ4uTMJ1CL1ST+L53MAhFXl/h2uIsJIi4hMWv6BVYIm1di RvtTqHo5iWlf1zBD2NIS52dtgDt68ffHUHF+iWO3d0DNEZCYeuYgVI22xL3XT9khbD6JNQvf Qs0UlDh9rZsZZlfDxt/sMDdsbXkCdgOzgKLElO6H7BC2lsSXH/vY0P3CK+Ahse3uDqYJjLKz kKRmIWmfhaQdWc0CRpZVjKKpBckFxUnpFSZ6xYm5xaV56XrJ+bmbGMHp6dmSHYwNF6wPMapz MEqJluYVJyfm5SUm5aTG5ybmpOUX5aamKPHwcjxwDxdiTSwrrsw9xKgCtOrRhtUXGKVY8vLz UpVEeCNFPcKFeFMSK6tSi/Lji0pzUosPMZoCY3Yis5Rocj4wAeeVxBsaGxgbGlqaG5gaGlko ifMG/F0XJiSQnliSmp2aWpBaBNPHxMEp1cC43Pv7Jhemt2nLs9iusTGf+Jr1Ujd6vXdeQcKv qc9EYza7/V09P7Aj9KPkjl+m51cYLzvVaZc2bXoMt5v6zP2GbrFGc7nbpq5vUzL6e27C59ef Xz748D3c4OIqRr3zNYdbK5lOBU1d8knD+sD3CfMeOyorFM7/t+Wu2reG35pN7x/rXotqFJEp VWIpzkg01GIuKk4EAGfJXQCIAwAA DLP-Filter: Pass X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u4R9w5p7015381 >On Fri, May 27, 2016 at 01:38:17AM +0000, Chung-Geol Kim wrote: >> There is a double free problem in the usb driver. > >Which driver? When I using the USB OTG Storage, this issue happened. When remove the OTG Storage, it reproduced sometimes. > >> This is caused by delayed deregister for scsi device. >> <*> at Insert USB Storage >> - USB bus #1 register >> usb_create_hcd (primary-kref==1) >> * primary-bandwidth_mutex(alloc)) >> usb_get_hcd (primary-kref==2) >> - USB bus #2 register >> usb_create_hcd (second-kref==1) >> * second-bandwidth_mutex==primary-bandwidth_mutex >> usb_get_hcd (second-kref==2) >> - scsi_device_get >> usb_get_hcd (second-kref==3) >> >> <*> at remove USB Storage (Normal) >> - scsi_device_put >> usb_put_hcd (second-kref==2) >> - USB bus #2 deregister >> usb_release_dev(second-kref==1) >> usb_release_dev(second-kref==0) -> hcd_release() >> - USB bus #1 deregister >> usb_release_dev(primary-kref==1) >> usb_release_dev(primary-kref==0) -> hcd_release() >> *(primary-bandwidth_mutex free) >> >> at remove USB Storage >> - USB bus #2 deregister >> usb_release_dev(second-kref==2) >> usb_release_dev(second-kref==1) >> - USB bus #1 deregister >> usb_release_dev(primary-kref==1) >> usb_release_dev(primary-kref==0) -> hcd_release() >> *(primary-bandwidth_mutex free) >> - scsi_device_put >> usb_put_hcd (second-kref==0) -> hcd_release(*) >> * at this, second->primary==0 therefore try to >> free the primary-bandwidth_mutex.(already freed) > >The formatting for this is all confused, can you fix it up? Sorry to confuse you, Let me change as below. cpu 0 cpu 1 ---------------------------------------------------------------------------------------- (*Insert USB Storage) usb_create_shared_hcd() kmalloc(primary_hcd) kmalloc(primary_hcd->bandwidth_mutex) ->(primary_hcd->kref==1) usb_get_hcd() ->(primary_hcd->kref==2) usb_create_shared_hcd() kmalloc(hcd->shared_hcd) ->hcd->shared_hcd->bandwidth_mutex=primary->bandwidth_mutex ->primary_hcd->primary_hcd = primary_hcd ->hcd->shared_hcd->primary_hcd = primary_hcd ->(hcd->shared_hcd->kref==1) usb_get_hcd() ->(hcd->shared_hcd->kref==2) usb_get_hcd() ->(hcd->shared_hcd->kref==3) --------------------------------------------------------------------------------------- (*remove USB Storage) usb_release_dev() ->(hcd->shared_hcd-kref==2) usb_release_dev() ->(hcd->shared_hcd-kref==1) usb_release_dev() -> (primary_hcd-kref==1) usb_release_dev() -> (primary_hcd-kref==0) hcd_release() -> kfree(primary_hcd->bandwidth_mutex) -> hcd->shared_hcd->primary_hcd = NULL -> kfree(primary_hcd) usb_release_dev() -> (hcd->shared_hcd-kref==0) hcd_release() -> usb_hcd_is_primary_hcd(hcd->shared_hcd) -> hcd->shared_hcd->primary_hcd already NULL, return 1 -> try to double kfree(primary_hcd->bandwidth_mutex) Since hcd->shared_hcd->priary_hcd was Null it didn't reach (hcd == hcd->primary_hcd) in usb_hcd_is_primary_hcd(). It returned 1 at since condition !hcd->primary_hcd is met. > >> >> To fix this problem kfree(hcd->bandwidth_mutex); >> should be executed at only (hcd->primary_hcd==hcd). >> >> Signed-off-by: Chunggeol Kim > >We need an email address at the end of this line, look at how the >commits in the kernel git history look like for examples. sorry, maybe deleted during transfer the patch. Signed-off-by: Chunggeol Kim > >> --- >> drivers/usb/core/hcd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> index 34b837a..60077f3 100644 >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -2608,7 +2608,7 @@ static void hcd_release(struct kref *kref) >> struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref); >> >> mutex_lock(&usb_port_peer_mutex); >> - if (usb_hcd_is_primary_hcd(hcd)) { >> + if (hcd == hcd->primary_hcd) { > >That doesn't make sense, usb_hcd_is_primary_hcd() is the same as this >check, what are you changing here? Since hcd->priary_hcd was Null it didn't reach (hcd == hcd->primary_hcd). It returned 1 at since condition !hcd->primary_hcd is met. int usb_hcd_is_primary_hcd(struct usb_hcd *hcd) { if (!hcd->primary_hcd) return 1; return hcd == hcd->primary_hcd; } > >> kfree(hcd->address0_mutex); >> kfree(hcd->bandwidth_mutex); >> } > >Your patch itself is also corrupted, and can't be applied, can you also >resolve this and resend? > >thanks, > >greg k-h >