ftdi-rs / libftd2xx

Rust safe wrappers for the libftd2xx drivers
MIT License
32 stars 15 forks source link

Unsoundness issue in function function set_latency_timer #80

Open Farmadupe opened 2 weeks ago

Farmadupe commented 2 weeks ago

In PR #71, a change was merged to relax the valid input range in function set_latency_timer from 2-255ms to 0-255 ms, as the datasheet for device FT2232H says this is permissible.

Unfortunately this conflicts with the following words in the libd2xx API docs:

In the FT8U232AM and FT8U245AM devices, the receive buffer timeout that is used to flush remaining
data from the receive buffer was fixed at 16 ms. In all other FTDI devices, this timeout is programmable
and can be set at 1 ms intervals between 2ms and 255 ms. This allows the device to be better optimized
for protocols requiring faster response times from short data packets.

I think the change introduced by #71 must be considered unsound, as the API docs explicitly forbid values below 2 ms, and since those docs apply to every libtd2xx device, they probably cannot be overruled by the datasheet for a single device.

Possible solutions may be:

newAM commented 2 weeks ago

When documentation for closed source software contains contractions I err on the side of what works experimentally. I tested with a FT4232H and a FT2232H, both work with 0ms latency.

If there's a device for which 0ms does not work then this should be fixed, otherwise there's insufficient evidence to declare this unsound.

Farmadupe commented 2 weeks ago

Page 64 of the FTDI D2XX docs does actually explicitly state that the valid range of values is 2-255. Values 0-1 are therefore explicitly invalid. Thus the existing rust is unsound, as with the removal of boundary checking, it is now possible to use it to enter libd2xx with invalid inputs. This holds regardless of any experimental result.

I admit that the hole seems inoccuous, but the question of soundness is not actually in doubt.

FTDI could change their documentation tomorrow to say 0-255, without changing their opaque binaries, and the rust would instantly become sound. This is the power of documentation.

newAM commented 2 weeks ago

It's not just experimental, the FT2232H datasheet page 26 explicitly calls out a latency timer value of 0-255 as valid.