search

trezor / trezor-mcu

:lock: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
GNU Lesser General Public License v3.0
318 stars 255 forks source link

webusb - can't open USB connection if previous connection has pending data #444

Closed MelbourneDeveloper closed 5 years ago

MelbourneDeveloper commented 5 years ago

This was an issue in 1.7.1 but it is also and issue in 1.7.3. Perhaps I'm doing something wrong, but this is an issue with three different libraries including LibUsb. If I'm doing something wrong, please let me know. But, I'm sending the same data as I would for 1.6.x, and 1.6.x works fine.

To recreate the issue I am having with firmware 1.7.3 you can see the problem with LibUsbDotNet. The problem is NOT specific to LibUsb. This problem is occurring with any WinUSB API calls. But, it is a problem with any LibUsb based code on Windows.

In this USB library, I get an error code of 121 on this line: https://github.com/MelbourneDeveloper/Device.Net/blob/41166e4aa25384a0cb5849c549540218b3a22b91/src/Usb.Net.WindowsSample/Program.cs#L50. You can clone the sample here: https://github.com/MelbourneDeveloper/Device.Net.git

Notice that if you unplug the device and reconnect it, it will usually work fine.

Now repeat the process but use a 1.6.x device, and change VendorId to 0x534C. It works fine again and again.

Sorry for copying #443 but I could not reopen the issue. @prusnak

MelbourneDeveloper commented 5 years ago

@prusnak, @matejcik have you guys had a look at this at all?

It's quite possible that all the C# libraries are doing something wrong... But, several standard WinUSB C# libraries are having the same problem so it makes me suspect that it may be a firmware issue. This problem doesn't occur with the old firmware via Hid.dll (Windows Hid C library).

The basic process for talking to the Trezor is (WinUSB calls):

Am I missing something here? Does this look right?

matejcik commented 5 years ago

You're not reading the full response.

On my testing Trezor, the length of Features response to your Initialize message is 152 bytes, which splits into 3 64-byte packets. You're reading the first one and leaving the subsequent ones hanging. Repeating the read 3 times fixed the problem for me.

What seems to be happening is that Trezor will be unable to respond to USB opening sequence while it still has data to send - unlike TT, which will continue to send buffered data after USB close and reopen. This might be worth fixing, but could as well be a wontfix-problem, what do you think @prusnak ?

prusnak commented 5 years ago

I'd rather not change anything in our firmware unless it cannot be fixed on the client side. And it seems the fix is pretty straightforward.

matejcik commented 5 years ago

Closing. @MelbourneDeveloper, please ping here if you need to reopen.