netdev - Re: [PATCH v2 2/3] net: usb: cdc_enc: New driver exposing USB CDC encapsulated protocols

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: <87lip9dp11.fsf@nemi.mork.no>
Date:	Sun, 15 Jan 2012 15:22:02 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Oliver Neukum <oliver@...kum.org>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Dan Williams <dcbw@...hat.com>,
	Thomas Schäfer <tschaefer@...nline.de>
Subject: Re: [PATCH v2 2/3] net: usb: cdc_enc: New driver exposing USB CDC encapsulated protocols

Oliver Neukum <oliver@...kum.org> writes:
> Am Sonntag, 15. Januar 2012, 07:40:36 schrieb Bjørn Mork:
>> USB CDC Ethernet devices may encapsulate vendor specific protocols inside
>> USB_CDC_SEND_ENCAPSULATED_COMMAND +USB_CDC_GET_ENCAPSULATED_RESPONSE
>> control messages. Devices based on Qualcomm MSM
>> chipsets are known to use this for exporting the Qualcomm
>> MSM Interface (QMI) protocol. Examples of such devices are
>> the Huawei E392 and E398 LTE modems.
>
> The problem is that this is another version of the code already
> in cdc-wdm, which is tested well.

Huh?  I thought that was something entirely different.  Isn't it?

Anyway, I see that I should look thoroughly at it as it does these
things in a much nicer way.  Thanks for the pointer.

> I'd really like to unify this stuff.

That makes sense.  

[snipped a number of detailed issues which I obviously need to fix]

>> +
>> +/* per CDC_ENC interface state */
>> +struct cdc_enc_state {
>> +	struct usb_interface *intf;
>> +	struct urb *urb;		/* receive urb */
>> +	struct urb *interrupt;		/* interrupt urb */
>> +	unsigned char rcvbuf[CDC_ENC_BUFLEN];     /* receive buffer */
>
> This is a violation of the DMA requirements. You must use kmalloc.

This probably only demonstrates my lack of understanding these
requirements.  I thought this was OK as long as I kmalloc'ed the whole
struct, but I see now that that isn't necessarily true.

>
> I had a not very detailed look at this driver, but I would really prefer
> if you could have a look at making unified code for this, cdc-wdm and cdc-acm.

Will do.  Thanks a lot for the fast review.


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