From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932231AbcFCLvb (ORCPT ); Fri, 3 Jun 2016 07:51:31 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:41918 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751485AbcFCLv2 (ORCPT ); Fri, 3 Jun 2016 07:51:28 -0400 X-AuditID: cbfee68f-f79d26d0000014f6-cc-57516f3d0e40 Date: Fri, 03 Jun 2016 11:51:25 +0000 (GMT) From: Chung-Geol Kim Subject: Re: Re: Re: [PATCH] usb: core: fix a double free in the usb driver To: Alan Stern Cc: "gregkh@linuxfoundation.org" , "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" , "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: 20160603115021103@chunggeol.kim Msgkey: 20160603115021103@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: 20160603115021103@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: <487274449.448981464954680715.JavaMail.weblogic@epmlwas04d> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrMJsWRmVeSWpSXmKPExsVy+t8zHV3b/MBwg1v3zC0u75rD5sDo8XmT XABjVAOjTUpqTmZZapG+nU1iUXIGkKmQmpecn5KZl26rFBripmuhpJCRX1xiqxRtZGCsZ2Rq omdkYq5naRBrZWSqpJCXmJtqq1ShC9WrpFCUXABUm1tZDDQgJ1UPKq5XnJqX4pCVXwpyk15x Ym5xaV66XnJ+rlLCfMaMjq3n2QseJVT0L9jI3sC4IK6LkZNDSEBD4lRTNwuILSFgIvHw0yZm CFtM4sK99WxdjFxANcsYJd58mMUMU9T06goLRGIOo8S+ZwfAulkEVCR2vnrECmKzCRhKrP5z H6xBWMBb4tLXKWwgtoiAjsTiNRfB6pkFXrFInD+ZCnGFssTcO4/ZQWxeAUGJkzOfQF2kJvGj tYsVIq4usePZekaIuITErOkXWCFsXokZ7U+h6uUkpn1dA3WotMT5WRsYYb5Z/P0xVJxf4tjt HUwQtoDE1DMHoWq0JQ5uew8V55NYs/At1ExBidPXuplhdjVs/M0Oc8PWliesEL8oSkzpfsgO YWtJfPmxjw3dL7wC7hLH//eyggJOQuACh8Sivm/MExiVZiGpm4Vk1iwks5DVLGBkWcUomlqQ XFCclF5kjBzLmxghybB/B+PdA9aHGNU5GKVES/OKkxPz8hKTclLjcxNz0vKLclNTlHh4VywI CBdiTSwrrsw9xJgMjL6JzFKiyfnABJ1XEm9obGZkYWpiamxkbmmGIWxiamFhYoRDWEmcd6HU z2AhgfTEktTs1NSC1KL4otKc1OJDjEwcnFINjPNXpfy5UPZEZOGkPUVbrm/+xddo0MRQK3Sr 53V42/dPh75ZrJh7ZH/tjey1KppOeqzKJxv+bqqebaK7c+viwM2z9fi33JW7t9z2jev+XKsL 9TsO5H65cDFb7KDDPvElnzm8DRuajp7LlghwcHCL3jiZ8eunW7zifHYnOVXELy/KevHJ8bp9 srUSS3FGoqEWc1FxIgBve9bexgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrGKsWRmVeSWpSXmKPExsVy+t/tPl3b/MBwg7OXjS0u75rD5sDo8XmT XABjVJFNSmpOZllqkb6dTUZqYkpqkUJqXnJ+SmZeuq2Sd3C8c7ypmYGhrqGlhbmSQl5ibqqt kotPgK5bZg7QeCWFssScUqBQQGJxsRLQhKL80pJUhYz84hJbpWgjA2M9I1MTPSNjAz0Tg1gr QwMDI1OgqoSijI6t59kLHiVU9C/YyN7AuCCui5GTQ0hAQ+JUUzcLiC0hYCLR9OoKlC0mceHe erYuRi6gmjmMEvueHQBLsAioSOx89YgVxGYTMJRY/ec+M4gtLOAtcenrFDYQW0RAR2Lxmotg 9cwCr1gkzp9MhVimLDH3zmN2EJtXQFDi5MwnUMvUJH60drFCxNUldjxbzwgRl5CYNf0CK4TN KzGj/SlUvZzEtK9rmCFsaYnzszYwwhy9+PtjqDi/xLHbO5ggbAGJqWcOQtVoSxzc9h4qziex ZuFbqJmCEqevdTPD7GrY+Jsd5oatLU9YIX5RlJjS/ZAdwtaS+PJjHxu6X3gF3CWO/+9lncAo OwtJahaS9llI2pHVLGBkWcUomlqQXFCclF5hrFecmFtcmpeul5yfu4kRnKCeLd7B+P+89SFG dQ5GKdHSvOLkxLy8xKSc1PjcxJy0/KLc1BQlHt6IpQHhQqyJZcWVuYcYVYBWPdqw+gKjFEte fl6qkgivZ05guBBvSmJlVWpRfnxRaU5q8SFGU2DETmSWEk3OB6bgvJJ4Q2MDY0NDS3MDU0Mj CyVx3oC/68KEBNITS1KzU1MLUotg+pg4OKUaGKNfRdyxcg95dPrmS4bA53MuRrQcWeRsZnB2 RbhBxgqliQU64ewJZwyO/bvJ0vy8+tPsBa0JKzk8HmpOzFVhnH7Bp3JRQsbMurP/dWyC5m1g n3qk5FHsj2079+5c1c5q4b1118WS286Huhv52sLMP99Qm1nvvNrCWH0muwJT0NE9TOxH44/1 aiuxFGckGmoxFxUnAgBLUn8tiQMAAA== 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 u53Bpe4v029423 >On Fri, 27 May 2016, Chung-Geol Kim wrote: > >> >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. > >> 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) > >I don't understand. Why do these actions take place on two different >CPUs? Aren't the primary_hcd and the shared_hcd structures allocated >by the same thread, on the same CPU? Yes, you are right, The presentational errors in order to obtain an understanding of the process. Therefore, I will be happy to explain again the diagrammatic representation as shown below. If using usb 3.0 storage(OTG), you can see as below. ============================================== At *Insert USB(3.0) Storage sequence <1> --> <5> ============================================== VOLD =================================|============ (uevent) ______|___________ |<5> | | SCSI | |usb_get_hcd | |shared_hcd(kref=3)| |__________________| ___________________ ________|_________ |<2> | |<4> | |dwc3_otg_sm_work | |dwc3_otg_sm_work | |usb_get_hcd | |usb_get_hcd | |primary_hcd(kref=2)| |shared_hcd(kref=2)| |___________________| |__________________| _________|_________ ________|_________ |<1> | |<3> | |New USB BUS #1 | |New USB BUS #2 | |usb_create_hcd | |usb_create_hcd | |primary_hcd(kref=1)| |shared_hcd(kref=1)| | | | | |bandXX_mutex(alloc)|<-(Link)-bandXX_mutex | |___________________| |__________________| ============================================== At *remove USB(3.0) Storage sequence <1> --> <5> ((Normal Case)) ============================================== VOLD =================================|============ (uevent) ______|___________ |<1> | | SCSI | |usb_put_hcd | |shared_hcd(kref=2)| |__________________| ___________________ ________|_________ |<4> | |<2> | |dwc3_otg_sm_work | |dwc3_otg_sm_work | |usb_put_hcd | |usb_put_hcd | |primary_hcd(kref=1)| |shared_hcd(kref=1)| |___________________| |__________________| _________|_________ ________|_________ |<5> | |<3> | |New USB BUS #1 | |New USB BUS #2 | |hcd_release | |hcd_release | |primary_hcd(kref=0)| |shared_hcd(kref=0)| | | | | |bandXX_mutex(free) | -X-cut off)-bandXX_mutex| |___________________| |__________________| ---------------------------------------------- > >> --------------------------------------------------------------------------------------- >> (*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) > >The same question applies here. Aren't the shared_hcd and primary_hcd >structures released by the same thread, on the same CPU? > >The real bug here is that the shared_hcd is released after the >primary_hcd. That's what you need to fix. NO, It 's only depend on vold(scsi) release time. If the vold later released and is being released first hcd, Double free happened at <5> as below. ============================================== At *remove USB(3.0) Storage sequence <1> --> <5> ((Problem Case)) ============================================== VOLD =================================|============ (uevent) ______|___________ |<5> | | SCSI | |usb_put_hcd | |shared_hcd(kref=0)| |*hcd_release | |bandXX_mutex(free*)|<- double free |__________________| ___________________ ________|_________ |<3> | |<1> | |dwc3_otg_sm_work | |dwc3_otg_sm_work | |usb_put_hcd | |usb_put_hcd | |primary_hcd(kref=1)| |shared_hcd(kref=2)| |___________________| |__________________| _________|_________ ________|_________ |<4> | |<2> | |New USB BUS #1 | |New USB BUS #2 | |hcd_release | | | |primary_hcd(kref=0)| |shared_hcd(kref=1)| | | | | |bandXX_mutex(free) |<-(Link)-bandXX_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. > >> >> --- 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; >> } > >That's just a symptom, not the real cause of the bug. You need to fix >the real cause: the shared_hcd has to be released _before_ the >primary_hcd. > >The right way to do this is to make the shared_hcd take a reference to >the primary_hcd. This reference should be dropped when hcd_release() >is called for the shared_hcd. > >Alan Stern >