* [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free
@ 2015-12-26 3:57 changbin.du
2015-12-27 22:05 ` Robert Baldyga
0 siblings, 1 reply; 6+ messages in thread
From: changbin.du @ 2015-12-26 3:57 UTC (permalink / raw)
To: balbi, gregkh
Cc: r.baldyga, nab, andrzej.p, linux-usb, linux-kernel, Du, Changbin
From: "Du, Changbin" <changbin.du@intel.com>
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>
---
drivers/usb/gadget/function/f_acm.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 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)
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free
2015-12-26 3:57 [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free changbin.du
@ 2015-12-27 22:05 ` Robert Baldyga
2015-12-28 6:06 ` Du, Changbin
0 siblings, 1 reply; 6+ messages in thread
From: Robert Baldyga @ 2015-12-27 22:05 UTC (permalink / raw)
To: changbin.du, balbi, gregkh
Cc: r.baldyga, nab, andrzej.p, linux-usb, linux-kernel
On 12/26/2015 04:57 AM, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> If acm_bind fails before allocate notification and acm->notify_req is
> not set to NULL after freed last time, double free will happen.
Looks good to me.
Similar problem can occur with another USB functions (at least f_ecm,
f_ncm, f_rndis and f_hid handle USB requests in analogical way). Maybe
it's worth to fix them all at once?
>
> 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
>
Reviewed-by: Robert Baldyga <r.baldyga@hackerion.com>
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
> drivers/usb/gadget/function/f_acm.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 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)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free
2015-12-27 22:05 ` Robert Baldyga
@ 2015-12-28 6:06 ` Du, Changbin
2015-12-28 6:25 ` [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers changbin.du
0 siblings, 1 reply; 6+ messages in thread
From: Du, Changbin @ 2015-12-28 6:06 UTC (permalink / raw)
To: Robert Baldyga, balbi@ti.com, gregkh@linuxfoundation.org
Cc: r.baldyga@samsung.com, nab@linux-iscsi.org, andrzej.p@samsung.com,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1246 bytes --]
> On 12/26/2015 04:57 AM, changbin.du@intel.com wrote:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > If acm_bind fails before allocate notification and acm->notify_req is
> > not set to NULL after freed last time, double free will happen.
>
> Looks good to me.
>
> Similar problem can occur with another USB functions (at least f_ecm,
> f_ncm, f_rndis and f_hid handle USB requests in analogical way). Maybe
> it's worth to fix them all at once?
>
> >
> > 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
> >
>
> Reviewed-by: Robert Baldyga <r.baldyga@hackerion.com>
>
Hmm, you are right. I checked all the fucntions, found these need to be fixed:
f_ecm.c f_hid.c f_ncm.c f_phonet.c f_rndis.c f_uvc.c
I will update patche to fix them all. thank you.
Regards,
Du, Changbin
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers
2015-12-28 6:06 ` Du, Changbin
@ 2015-12-28 6:25 ` changbin.du
2015-12-28 21:13 ` Laurent Pinchart
2015-12-28 22:04 ` Robert Baldyga
0 siblings, 2 replies; 6+ messages in thread
From: changbin.du @ 2015-12-28 6:25 UTC (permalink / raw)
To: balbi, gregkh
Cc: r.baldyga, nab, andrzej.p, laurent.pinchart, prabhakar.csengg,
kopasiak90, linux-usb, linux-kernel, Du, Changbin
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
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);
--
2.5.0
^ permalink raw reply related [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: 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
end of thread, other threads:[~2015-12-28 22:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-26 3:57 [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free changbin.du
2015-12-27 22:05 ` Robert Baldyga
2015-12-28 6:06 ` Du, Changbin
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).