netdev - Re: [PATCH net-next v6 0/3] The huawei_cdc

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: <877g7tm1an.fsf@nemi.mork.no>
Date:	Mon, 17 Mar 2014 13:56:16 +0100
From:	Bjørn Mork <bjorn@...k.no>
To:	Pasi Kärkkäinen <pasik@....fi>
Cc:	Dan Williams <dcbw@...hat.com>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org, Enrico Mioso <mrkiko.rs@...il.com>,
	Oliver Neukum <oliver@...kum.org>
Subject: Re: [PATCH net-next v6 0/3] The huawei_cdc_ncm driver / E3276 problem

Bjørn Mork <bjorn@...k.no> writes:

> But here we have a device which does not comform to spec (that's OK,
> Huawei doesn't claim it does - this is a vendor specific function after
> all), and which seems to be locked to 32 bit mode?  Either it requires
> the 32 bit variant, or we are doing something "wrong" during setup to
> make the device go into this mode.

Yuck.  Looking at the code, we do definitely do something wrong here.
We do this during setup:

        /* set NTB format, if both formats are supported */
        if (ntb_fmt_supported & USB_CDC_NCM_NTH32_SIGN) {
                err = usbnet_write_cmd(dev, USB_CDC_SET_NTB_FORMAT,
                                       USB_TYPE_CLASS | USB_DIR_OUT
                                       | USB_RECIP_INTERFACE,
                                       USB_CDC_NCM_NTB16_FORMAT,
                                       iface_no, NULL, 0);
                if (err < 0)
                        dev_dbg(&dev->intf->dev, "Setting NTB format to 16-bit failed\n");
        }

But USB_CDC_NCM_NTH32_SIGN isn't a flag mask.  It is a constant matching
the the header signature.  I.e. the 'ncmh' string, or 0x686D636E as the
little endian 32 bit number the macro expands to. The ntb_fmt_supported
variable is the bmNtbFormatsSupported bitmap in CPU endianness.  Bit 0
indicates 16 bit support, bit 1 indicates 32 bit support and the rest is
reserved (zero/ignored).  So the test does actually work because it will
return true of bit 1 is set, but it is fundamentally wrong because it
also inspects a number of bits which the host is required to ignore.

And looking at your dump, I see that the GET_NTB_PARAMETERS response is:

Frame 151: 92 bytes on wire (736 bits), 92 bytes captured (736 bits) on interface 0
[..]
0040  1c 00 03 00 00 00 04 00 04 00 02 00 04 00 00 00   ................
0050  00 80 00 00 04 00 02 00 04 00 00 00               ............

This is the structure:

struct usb_cdc_ncm_ntb_parameters {
        __le16  wLength;
        __le16  bmNtbFormatsSupported;
        __le32  dwNtbInMaxSize;
        __le16  wNdpInDivisor;
        __le16  wNdpInPayloadRemainder;
        __le16  wNdpInAlignment;
        __le16  wPadding1;
        __le32  dwNtbOutMaxSize;
        __le16  wNdpOutDivisor;
        __le16  wNdpOutPayloadRemainder;
        __le16  wNdpOutAlignment;
        __le16  wNtbOutMaxDatagrams;
} __attribute__ ((packed));

So bmNtbFormatsSupported is 0x00000003 as expected and we should execute
the SET_NTB_FORMAT above.  And we do.  I can see these NCM control
requests:

frame 150: in GET_NTB_PARAMETERS
frame 152: out SET_NTB_INPUT_SIZE
frame 154: out SET_CRC_MODE
frame 156: out SET_NTB_FORMAT
frame 158: in GET_MAX_DATAGRAM_SIZE

But looking closer, I see that the SET_NTB_FORMAT returned -EPIPE,
i.e. stall.  The driver currently ignores this error, which is why we
end up with different device and host settings. 

Anyone know what the proper driver action is? Note that this error
cannot happen with a spec conforming device, because SET_NTB_FORMAT
support is required for devices claiming 32 bit support.  So the spec
doesn't tell us what to do.

We do of course want to support all devices.  Is the proper action to
add 32 bit support and fall back to setting that?  What if that fails as
well?  Just silently accept 32 bit NTBs?  I have played with that
thought.  It is possible.  But it's likely not more successful unless we
make it automacally switch to sending such NTBs as well, and I'm not
sure that's a good idea.

Just fail?  Well, that doesn't make this device work...

Wonder what happens if you comment out the SET_NTB_FORMAT command?
Maybe the firmware have some odd logic which unconditionally switches
to 32 bit assuming that the host will only send this command for that
case?  And it fails because the "set 16 bit" parameter is unexpected
given this assumption?

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