On Fri, Oct 17, 2014 at 6:09 PM, Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
> * pavi1729 | 2014-10-15 14:47:53 [+0530]:
>
>>Removed usb_free_all_descriptors in the bind functions, which
>>results in double-free corruption of the descriptors on error path.
>>The usb descriptors are allocated by usb_assign_descriptors.
>>
>>Signed-off-by: Pavitrakumar Managutte <pavitra1729@xxxxxxxxx>
>>Reviewed-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
>
> You can add
>
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
>
> *but* the patch is badly formated, you need to fix this.
Agreed. Will send a proper patch with the additions.
>
>>diff --git a/drivers/usb/gadget/function/f_ncm.c
>>b/drivers/usb/gadget/function/f_ncm.c
>>index bcdc882..38a9279 100644
>>--- a/drivers/usb/gadget/function/f_ncm.c
>>+++ b/drivers/usb/gadget/function/f_ncm.c
>>@@ -1453,7 +1453,6 @@ static int ncm_bind(struct usb_configuration *c,
>>struct usb_function *f)
>> return 0;
>>
>> fail:
>>- usb_free_all_descriptors(f);
>
> This is okay, but if you scroll up, you will notice that
> usb_assign_descriptors() isn't checked. Could you please add this as a
> separate patch?
Agreed. Will do that.
>
>> if (ncm->notify_req) {
>> kfree(ncm->notify_req->buf);
>> usb_ep_free_request(ncm->notify, ncm->notify_req);
>>diff --git a/drivers/usb/gadget/function/f_rndis.c
>>b/drivers/usb/gadget/function/f_rndis.c
>>index ddb09dc..2f0517f 100644
>>--- a/drivers/usb/gadget/function/f_rndis.c
>>+++ b/drivers/usb/gadget/function/f_rndis.c
>>@@ -803,7 +803,7 @@ rndis_bind(struct usb_configuration *c, struct
>>usb_function *f)
>> if (rndis->manufacturer && rndis->vendorID &&
>> rndis_set_param_vendor(rndis->config, rndis->vendorID,
>> rndis->manufacturer))
>>- goto fail;
>>+ goto fail_free_descs;
>>
>> /* NOTE: all that is done without knowing or caring about
>> * the network link ... which is unavailable to this code
>>@@ -817,10 +817,11 @@ rndis_bind(struct usb_configuration *c, struct
>>usb_function *f)
>> rndis->notify->name);
>> return 0;
>>
>>+fail_free_descs:
>>+ usb_free_all_descriptors(f);
>> fail:
>> kfree(f->os_desc_table);
>> f->os_desc_n = 0;
>>- usb_free_all_descriptors(f);
>>
>> if (rndis->notify_req) {
>> kfree(rndis->notify_req->buf);
>
> The remaining issue is that if you get here via fail_free_descs you
> return the error path with status == 0 and so the caller does not notice
> that somthing went wrong. Would you please fix this (in a separate
> patch)?
Agreed. Will do that.
>
> Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
