Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsoundness issue in function function set_latency_timer #80

Open
Farmadupe opened this issue Sep 1, 2024 · 3 comments
Open

Unsoundness issue in function function set_latency_timer #80

Farmadupe opened this issue Sep 1, 2024 · 3 comments

Comments

@Farmadupe
Copy link
Contributor

Farmadupe commented Sep 1, 2024

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:

  • Reintroduce range checks (would suggest full assertions rather than debug assertions, as perf is probably not an issue)
  • Mark this function as unsafe
  • Add an unsafe_set_latency_timer function to allow <2ms timeouts.
@ftdi-rs ftdi-rs deleted a comment Sep 1, 2024
@newAM
Copy link
Member

newAM commented Sep 1, 2024

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
Copy link
Contributor Author

Farmadupe commented Sep 2, 2024

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
Copy link
Member

newAM commented Sep 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants