* Re: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers
2015-12-28 6:25 ` [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers changbin.du
@ 2015-12-28 21:13 ` Laurent Pinchart
2015-12-28 22:04 ` Robert Baldyga
1 sibling, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2015-12-28 21:13 UTC (permalink / raw)
To: changbin.du
Cc: balbi, gregkh, r.baldyga, nab, andrzej.p, prabhakar.csengg,
kopasiak90, linux-usb, linux-kernel
Hi Changbin,
Thank you for the patch.
On Monday 28 December 2015 14:25:50 changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> 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 <laurent.pinchart@ideasonboard.com>
> 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 <changbin.du@intel.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers
2015-12-28 6:25 ` [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers changbin.du
2015-12-28 21:13 ` Laurent Pinchart
@ 2015-12-28 22:04 ` Robert Baldyga
1 sibling, 0 replies; 6+ messages in thread
From: Robert Baldyga @ 2015-12-28 22:04 UTC (permalink / raw)
To: changbin.du, balbi, gregkh
Cc: nab, andrzej.p, laurent.pinchart, prabhakar.csengg, kopasiak90,
linux-usb, linux-kernel
Hi,
On 12/28/2015 07:25 AM, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> 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 <r.baldyga@hackerion.com>
Thanks,
Robert Baldyga
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread