Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1298159 > unrolled thread

[PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free

Started bychangbin.du@intel.com
First post2015-12-26 05:10 +0100
Last post2015-12-28 23:10 +0100
Articles 6 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free changbin.du@intel.com - 2015-12-26 05:10 +0100
    Re: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to  avoid double free Robert Baldyga <r.baldyga@hackerion.com> - 2015-12-28 03:50 +0100
      RE: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to  avoid double free "Du, Changbin" <changbin.du@intel.com> - 2015-12-28 07:10 +0100
        [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers changbin.du@intel.com - 2015-12-28 07:40 +0100
          Re: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2015-12-28 22:20 +0100
          Re: [PATCH v2] usb: gadget: fix double mem free for usb_request  caused by wild pointers Robert Baldyga <r.baldyga@hackerion.com> - 2015-12-28 23:10 +0100

#1298159 — [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free

Fromchangbin.du@intel.com
Date2015-12-26 05:10 +0100
Subject[PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free
Message-ID<qJOl4-4um-9@gated-at.bofh.it>
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

--
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/

[toc] | [next] | [standalone]

#1298457 — Re: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free

FromRobert Baldyga <r.baldyga@hackerion.com>
Date2015-12-28 03:50 +0100
SubjectRe: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free
Message-ID<qKw3c-3Io-633@gated-at.bofh.it>
In reply to#1298159
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)
> 


--
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/

[toc] | [prev] | [next] | [standalone]

#1298519 — RE: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free

From"Du, Changbin" <changbin.du@intel.com>
Date2015-12-28 07:10 +0100
SubjectRE: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free
Message-ID<qKzah-6rU-1@gated-at.bofh.it>
In reply to#1298457
PiBPbiAxMi8yNi8yMDE1IDA0OjU3IEFNLCBjaGFuZ2Jpbi5kdUBpbnRlbC5jb20gd3JvdGU6DQo+
ID4gRnJvbTogIkR1LCBDaGFuZ2JpbiIgPGNoYW5nYmluLmR1QGludGVsLmNvbT4NCj4gPg0KPiA+
IElmIGFjbV9iaW5kIGZhaWxzIGJlZm9yZSBhbGxvY2F0ZSBub3RpZmljYXRpb24gYW5kIGFjbS0+
bm90aWZ5X3JlcSBpcw0KPiA+IG5vdCBzZXQgdG8gTlVMTCBhZnRlciBmcmVlZCBsYXN0IHRpbWUs
IGRvdWJsZSBmcmVlIHdpbGwgaGFwcGVuLg0KPiANCj4gTG9va3MgZ29vZCB0byBtZS4NCj4gDQo+
IFNpbWlsYXIgcHJvYmxlbSBjYW4gb2NjdXIgd2l0aCBhbm90aGVyIFVTQiBmdW5jdGlvbnMgKGF0
IGxlYXN0IGZfZWNtLA0KPiBmX25jbSwgZl9ybmRpcyBhbmQgZl9oaWQgaGFuZGxlIFVTQiByZXF1
ZXN0cyBpbiBhbmFsb2dpY2FsIHdheSkuIE1heWJlDQo+IGl0J3Mgd29ydGggdG8gZml4IHRoZW0g
YWxsIGF0IG9uY2U/DQo+IA0KPiA+DQo+ID4ga2VybmVsIEJVRyBhdCBtbS9zbHViLmM6MzM5MiEN
Cj4gPiBpbnZhbGlkIG9wY29kZTogMDAwMCBbIzFdIFBSRUVNUFQgU01QDQo+ID4gRUlQIGlzIGF0
IGtmcmVlKzB4MTcyLzB4MTgwDQo+ID4gQ2FsbCBUcmFjZToNCj4gPiBbPDgwYzBlM2I2Pl0gPyB1
c2JfZXBfYXV0b2NvbmZpZ19zcysweDg2LzB4MTcwDQo+ID4gWzw4MGMxMzM0NT5dIGdzX2ZyZWVf
cmVxKzB4MTUvMHgzMA0KPiA+IFs8ODBjMTJkZjE+XSBhY21fYmluZCsweDFjMS8weDJkMA0KPiA+
IFs8ODBjMGU5YmU+XSB1c2JfYWRkX2Z1bmN0aW9uKzB4NmUvMHgxMjANCj4gPiBbPDgwYzIxM2Ni
Pl0gYWNtX2Z1bmN0aW9uX2JpbmRfY29uZmlnKzB4MmIvMHg5MA0KPiA+DQo+IA0KPiBSZXZpZXdl
ZC1ieTogUm9iZXJ0IEJhbGR5Z2EgPHIuYmFsZHlnYUBoYWNrZXJpb24uY29tPg0KPg0KSG1tLCB5
b3UgYXJlIHJpZ2h0LiBJIGNoZWNrZWQgYWxsIHRoZSBmdWNudGlvbnMsIGZvdW5kIHRoZXNlIG5l
ZWQgdG8gYmUgZml4ZWQ6DQpmX2VjbS5jIGZfaGlkLmMgZl9uY20uYyBmX3Bob25ldC5jIGZfcm5k
aXMuYyBmX3V2Yy5jDQoNCkkgd2lsbCB1cGRhdGUgcGF0Y2hlIHRvIGZpeCB0aGVtIGFsbC4gdGhh
bmsgeW91Lg0KDQpSZWdhcmRzLA0KRHUsIENoYW5nYmluDQoNCg==
--
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/

[toc] | [prev] | [next] | [standalone]

#1298520 — [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers

Fromchangbin.du@intel.com
Date2015-12-28 07:40 +0100
Subject[PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers
Message-ID<qKzDk-6D7-1@gated-at.bofh.it>
In reply to#1298519
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

--
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/

[toc] | [prev] | [next] | [standalone]

#1298760 — Re: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers

FromLaurent Pinchart <laurent.pinchart@ideasonboard.com>
Date2015-12-28 22:20 +0100
SubjectRe: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers
Message-ID<qKNmW-7GX-19@gated-at.bofh.it>
In reply to#1298520
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

--
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/

[toc] | [prev] | [next] | [standalone]

#1298771 — Re: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers

FromRobert Baldyga <r.baldyga@hackerion.com>
Date2015-12-28 23:10 +0100
SubjectRe: [PATCH v2] usb: gadget: fix double mem free for usb_request caused by wild pointers
Message-ID<qKO9k-8f0-9@gated-at.bofh.it>
In reply to#1298520
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);
> 


--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web