netdev - Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

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: <87r4ley6g9.fsf@nemi.mork.no>
Date:	Mon, 21 Jan 2013 15:08:22 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Alexey Orishko <alexey.orishko@...il.com>
Cc:	Oliver Neukum <oliver@...kum.org>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org, Greg Suarez <gsuarez@...thmicro.com>,
	Alexey Orishko <alexey.orishko@...ricsson.com>
Subject: Re: [PATCH net] net: cdc_ncm: workaround for missing CDC Union

Hello Alexey,

I have another issue with the Sierra firmware which I hope you can help
me with: The MC7710 device requires at ZLP even if we send
dwNtbOutMaxSize sized NTBs. This is a problem because the current code
explicitly prevents this.

The following code in the v3.8 cdc-ncm was written to keep existing
behaviour from the pre-v3.8 driver:

	if (((skb_out->len % le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0) &&
	    (skb_out->len < le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)) && skb_tailroom(skb_out))
		*skb_put(skb_out, 1) = 0;	/* force short packet */

The previous implementaion looked like this:

6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  832)    if (((last_offset < ctx->tx_max) && ((last_offset %
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  833)                    le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0)) ||
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  834)        (((last_offset == ctx->tx_max) && ((ctx->tx_max %
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  835)            le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0)) &&
6c60408e (Alexey Orishko     2011-05-06 03:01:30 +0000  836)            (ctx->tx_max < le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize)))) {
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  837)            /* force short packet */
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  838)            *(((u8 *)skb_out->data) + last_offset) = 0;
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  839)            last_offset++;
900d495a (Alexey Orishko     2010-11-29 23:23:28 +0000  840)    }

The effect is the same: We add a 0 byte if the NTB length is a multiplum
of wMaxPacketSize *except* if the length is equal to dwNtbOutMaxSize.
This exception will happen very often because of the way we pad NTBs.

Now, I have tried to find what the above code was based on, and my guess
is that it is this note in table 3-1 in the CDC NCM spec:

   If wBlockLength = 0x0000, the block is terminated by a
   short packet. In this case, the USB transfer must still
   be shorter than dwNtbInMaxSize or dwNtbOutMax-
   Size. If exactly dwNtbInMaxSize or dwNtbOutMaxSize
   bytes are sent, and the size is a multiple of wMax-
   PacketSize for the given pipe, then no ZLP shall be
   sent.

Is that correct?  I cannot find any special ZLP handling mentioned
anywhere else in the standard.

If so, then I believe it is a misinterpretation.  The above text deals
only with the exceptional case of wBlockLength = 0x0000, which we do not
do.  As long as wBlockLength > 0 then I believe the device is in its
full right to expect a ZLP if wBlockLength % wMaxPacketSize is 0.

Would you feel comfortable dropping the additional condition and going
with

	if (((skb_out->len % le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0) &&
	    skb_tailroom(skb_out))
		*skb_put(skb_out, 1) = 0;	/* force short packet */

? I have verified that this is sufficient to make the Sierra device
work.  I will of course test it with the other NCM and MBIM devices I've
got, but that is a very limited set...

The other option I see is making a device specific quirk for this.  But
I suspect that Sierra is using the current Qualcomm MBIM implemetation
here, and if so then we are likely to see a large number of similar
devices in the near future.  I'd really like to avoid having device
specific quirks for all of them if at all possible.

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