netdev - Re: [PATCH] net/cdc_ncm: fix null pointer panic at usbnet_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87y55bduh1.fsf@nemi.mork.no>
Date:	Wed, 30 Oct 2013 09:38:18 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	David Miller <davem@...emloft.net>
Cc:	changbinx.du@...el.com, oliver@...kum.org,
	linux-usb@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net/cdc_ncm: fix null pointer panic at usbnet_link_change

David Miller <davem@...emloft.net> writes:

> The problem is in cdc_ncm_bind_common().
>
> It seems to leave dangling interface data pointers in some cases, and
> then branches just to "error" so that they don't get cleared back out.

Sorry, but I fail to see this as well.  I see one "return" and two "goto
error", but all are well before any intfdata pointer is set:

    364         ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
    365         if (!ctx)
    366                 return -ENOMEM;
    367 
[..]
    453         /* check if we got everything */
    454         if ((ctx->control == NULL) || (ctx->data == NULL) ||
    455             ((!ctx->mbim_desc) && ((ctx->ether_desc == NULL) || (ctx->control != intf))))
    456                 goto error;
    457 
    458         /* claim data interface, if different from control */
    459         if (ctx->data != ctx->control) {
    460                 temp = usb_driver_claim_interface(driver, ctx->data, dev);
    461                 if (temp)
    462                         goto error;
    463         }
[..]
    490         usb_set_intfdata(ctx->data, dev);
    491         usb_set_intfdata(ctx->control, dev);
    492         usb_set_intfdata(ctx->intf, dev);
[..]
    510         return 0;
    511 
    512 error2:
    513         usb_set_intfdata(ctx->control, NULL);
    514         usb_set_intfdata(ctx->data, NULL);
    515         if (ctx->data != ctx->control)
    516                 usb_driver_release_interface(driver, ctx->data);
    517 error:
    518         cdc_ncm_free((struct cdc_ncm_ctx *)dev->data[0]);
    519         dev->data[0] = 0;
    520         dev_info(&dev->udev->dev, "bind() failure\n");
    521         return -ENODEV;
    522 }
 

This could (and given this thread, probably should) certainly be
refactored and cleaned up a bit.  But I do not see how it leaves any
dangling pointers.  The pointers are only set near the end of the
function, and the only exit points after that are either success or
through the "error2" label.

Side note: It is definitely confusing that we set 3 pointers, but only
clean up 2.  The reason is that there are never more than 2 interfaces
involved here. We always have ctx->intf == ctx->control.  I'd really
like to get rid of that redundant ctx->intf pointer.  That's one issue
to cleanup throughout this driver.



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html