Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752754AbbL1WEy (ORCPT ); Mon, 28 Dec 2015 17:04:54 -0500 Received: from sender163-mail.zoho.com ([74.201.84.163]:25397 "EHLO sender163-mail.zoho.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752534AbbL1WEv (ORCPT ); Mon, 28 Dec 2015 17:04:51 -0500 Subject: Re: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers To: changbin.du@intel.com, balbi@ti.com, gregkh@linuxfoundation.org References: <0C18FE92A7765D4EB9EE5D38D86A563A0316D9B8@SHSMSX103.ccr.corp.intel.com> <1451283950-25724-1-git-send-email-changbin.du@intel.com> Cc: nab@linux-iscsi.org, andrzej.p@samsung.com, laurent.pinchart@ideasonboard.com, prabhakar.csengg@gmail.com, kopasiak90@gmail.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org From: Robert Baldyga Message-ID: <5681B1FD.60305@hackerion.com> Date: Mon, 28 Dec 2015 23:04:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1451283950-25724-1-git-send-email-changbin.du@intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Zoho-Virus-Status: 1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7770 Lines: 221 Hi, On 12/28/2015 07:25 AM, changbin.du@intel.com wrote: > From: "Du, Changbin" > > acm, ecm, hid, ncm, phonet, rndis and uvc functions all have double > memory free issue. Set pointers to NULL after freed to avoid this. > > Here explain how it happen on acm function, others has analogical case. > > If acm_bind fails before allocate notification and acm->notify_req is > not set to NULL after freed last time, double free will happen. > > kernel BUG at mm/slub.c:3392! > invalid opcode: 0000 [#1] PREEMPT SMP > EIP is at kfree+0x172/0x180 > Call Trace: > [<80c0e3b6>] ? usb_ep_autoconfig_ss+0x86/0x170 > [<80c13345>] gs_free_req+0x15/0x30 > [<80c12df1>] acm_bind+0x1c1/0x2d0 > [<80c0e9be>] usb_add_function+0x6e/0x120 > [<80c213cb>] acm_function_bind_config+0x2b/0x90 > Looks good to me again :) Reviewed-by: Robert Baldyga Thanks, Robert Baldyga > Signed-off-by: Du, Changbin > --- > changes from v1: > 1. add same fix for ecm, hid, ncm... fucntions > 2. patch title changed > --- > drivers/usb/gadget/function/f_acm.c | 8 ++++++-- > drivers/usb/gadget/function/f_ecm.c | 2 ++ > drivers/usb/gadget/function/f_hid.c | 5 ++++- > drivers/usb/gadget/function/f_ncm.c | 2 ++ > drivers/usb/gadget/function/f_phonet.c | 11 ++++++++--- > drivers/usb/gadget/function/f_rndis.c | 2 ++ > drivers/usb/gadget/function/f_uvc.c | 5 ++++- > 7 files changed, 28 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c > index 2fa1e80..e10c8d4 100644 > --- a/drivers/usb/gadget/function/f_acm.c > +++ b/drivers/usb/gadget/function/f_acm.c > @@ -699,8 +699,10 @@ acm_bind(struct usb_configuration *c, struct usb_function *f) > return 0; > > fail: > - if (acm->notify_req) > + if (acm->notify_req) { > gs_free_req(acm->notify, acm->notify_req); > + acm->notify_req = NULL; > + } > > ERROR(cdev, "%s/%p: can't bind, err %d\n", f->name, f, status); > > @@ -713,8 +715,10 @@ static void acm_unbind(struct usb_configuration *c, struct usb_function *f) > > acm_string_defs[0].id = 0; > usb_free_all_descriptors(f); > - if (acm->notify_req) > + if (acm->notify_req) { > gs_free_req(acm->notify, acm->notify_req); > + acm->notify_req = NULL; > + } > } > > static void acm_free_func(struct usb_function *f) > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c > index 7ad60ee..23092a2 100644 > --- a/drivers/usb/gadget/function/f_ecm.c > +++ b/drivers/usb/gadget/function/f_ecm.c > @@ -809,6 +809,7 @@ fail: > if (ecm->notify_req) { > kfree(ecm->notify_req->buf); > usb_ep_free_request(ecm->notify, ecm->notify_req); > + ecm->notify_req = NULL; > } > > ERROR(cdev, "%s: can't bind, err %d\n", f->name, status); > @@ -907,6 +908,7 @@ static void ecm_unbind(struct usb_configuration *c, struct usb_function *f) > > kfree(ecm->notify_req->buf); > usb_ep_free_request(ecm->notify, ecm->notify_req); > + ecm->notify_req = NULL; > } > > static struct usb_function *ecm_alloc(struct usb_function_instance *fi) > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c > index 99285b4..827e385 100644 > --- a/drivers/usb/gadget/function/f_hid.c > +++ b/drivers/usb/gadget/function/f_hid.c > @@ -679,8 +679,10 @@ fail: > ERROR(f->config->cdev, "hidg_bind FAILED\n"); > if (hidg->req != NULL) { > kfree(hidg->req->buf); > - if (hidg->in_ep != NULL) > + if (hidg->in_ep != NULL) { > usb_ep_free_request(hidg->in_ep, hidg->req); > + hidg->req = NULL; > + } > } > > return status; > @@ -912,6 +914,7 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f) > usb_ep_disable(hidg->in_ep); > kfree(hidg->req->buf); > usb_ep_free_request(hidg->in_ep, hidg->req); > + hidg->req = NULL; > > usb_free_all_descriptors(f); > } > diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c > index 7ad798a..510d1d7 100644 > --- a/drivers/usb/gadget/function/f_ncm.c > +++ b/drivers/usb/gadget/function/f_ncm.c > @@ -1459,6 +1459,7 @@ fail: > if (ncm->notify_req) { > kfree(ncm->notify_req->buf); > usb_ep_free_request(ncm->notify, ncm->notify_req); > + ncm->notify_req = NULL; > } > > ERROR(cdev, "%s: can't bind, err %d\n", f->name, status); > @@ -1561,6 +1562,7 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f) > > kfree(ncm->notify_req->buf); > usb_ep_free_request(ncm->notify, ncm->notify_req); > + ncm->notify_req = NULL; > } > > static struct usb_function *ncm_alloc(struct usb_function_instance *fi) > diff --git a/drivers/usb/gadget/function/f_phonet.c b/drivers/usb/gadget/function/f_phonet.c > index 157441d..5e146e1 100644 > --- a/drivers/usb/gadget/function/f_phonet.c > +++ b/drivers/usb/gadget/function/f_phonet.c > @@ -569,8 +569,10 @@ static int pn_bind(struct usb_configuration *c, struct usb_function *f) > return 0; > > err_req: > - for (i = 0; i < phonet_rxq_size && fp->out_reqv[i]; i++) > + for (i = 0; i < phonet_rxq_size && fp->out_reqv[i]; i++) { > usb_ep_free_request(fp->out_ep, fp->out_reqv[i]); > + fp->out_reqv[i] = NULL; > + } > usb_free_all_descriptors(f); > err: > ERROR(cdev, "USB CDC Phonet: cannot autoconfigure\n"); > @@ -662,9 +664,12 @@ static void pn_unbind(struct usb_configuration *c, struct usb_function *f) > /* We are already disconnected */ > if (fp->in_req) > usb_ep_free_request(fp->in_ep, fp->in_req); > - for (i = 0; i < phonet_rxq_size; i++) > - if (fp->out_reqv[i]) > + for (i = 0; i < phonet_rxq_size; i++) { > + if (fp->out_reqv[i]) { > usb_ep_free_request(fp->out_ep, fp->out_reqv[i]); > + fp->out_reqv[i] = NULL; > + } > + } > > usb_free_all_descriptors(f); > } > diff --git a/drivers/usb/gadget/function/f_rndis.c b/drivers/usb/gadget/function/f_rndis.c > index e587767..804a717 100644 > --- a/drivers/usb/gadget/function/f_rndis.c > +++ b/drivers/usb/gadget/function/f_rndis.c > @@ -821,6 +821,7 @@ fail: > if (rndis->notify_req) { > kfree(rndis->notify_req->buf); > usb_ep_free_request(rndis->notify, rndis->notify_req); > + rndis->notify_req = NULL; > } > > ERROR(cdev, "%s: can't bind, err %d\n", f->name, status); > @@ -948,6 +949,7 @@ static void rndis_unbind(struct usb_configuration *c, struct usb_function *f) > > kfree(rndis->notify_req->buf); > usb_ep_free_request(rndis->notify, rndis->notify_req); > + rndis->notify_req = NULL; > } > > static struct usb_function *rndis_alloc(struct usb_function_instance *fi) > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index 29b41b5..f3a93bb 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -736,8 +736,10 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) > error: > v4l2_device_unregister(&uvc->v4l2_dev); > > - if (uvc->control_req) > + if (uvc->control_req) { > usb_ep_free_request(cdev->gadget->ep0, uvc->control_req); > + uvc->control_req = NULL; > + } > kfree(uvc->control_buf); > > usb_free_all_descriptors(f); > @@ -864,6 +866,7 @@ static void uvc_unbind(struct usb_configuration *c, struct usb_function *f) > v4l2_device_unregister(&uvc->v4l2_dev); > > usb_ep_free_request(cdev->gadget->ep0, uvc->control_req); > + uvc->control_req = NULL; > kfree(uvc->control_buf); > > usb_free_all_descriptors(f); > -- 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/