Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752854AbbL1VOC (ORCPT ); Mon, 28 Dec 2015 16:14:02 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:58623 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752483AbbL1VN5 (ORCPT ); Mon, 28 Dec 2015 16:13:57 -0500 From: Laurent Pinchart To: changbin.du@intel.com Cc: balbi@ti.com, gregkh@linuxfoundation.org, r.baldyga@hackerion.com, nab@linux-iscsi.org, andrzej.p@samsung.com, prabhakar.csengg@gmail.com, kopasiak90@gmail.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers Date: Mon, 28 Dec 2015 23:13:55 +0200 Message-ID: <2479692.ZVCoTVT39n@avalon> User-Agent: KMail/4.14.8 (Linux/4.1.12-gentoo; KDE/4.14.8; x86_64; ; ) In-Reply-To: <1451283950-25724-1-git-send-email-changbin.du@intel.com> References: <0C18FE92A7765D4EB9EE5D38D86A563A0316D9B8@SHSMSX103.ccr.corp.intel.com> <1451283950-25724-1-git-send-email-changbin.du@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7819 Lines: 226 Hi Changbin, Thank you for the patch. On Monday 28 December 2015 14:25:50 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. For drivers/usb/gadget/function/f_uvc.c, Reviewed-by: Laurent Pinchart > 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 > > 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); -- Regards, Laurent Pinchart -- 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/