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.

Sign up for GitHub

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

Jump to bottom

Replaced parse_descriptor() function, fixed some overruns #1460

Closed
wants to merge 6 commits into from
Closed

Replaced parse_descriptor() function, fixed some overruns #1460

wants to merge 6 commits into from

Commits on May 27, 2024

  1. descriptor: Replace parse_descriptor() function 

    This function had a few problems:

- it takes two buffers as parameters but knows nothing about their length, making it easy to overrun them.
- callers make unwarranted assumptions about the alignment of structures that are passed to it (it assumes there's no padding)
- it has tricky pointer arithmetic and masking

With this new formulation, it's easier to see what's being read/written, especially the destination. It's now very clear that the destination is not being overrun because we are simply assigning to struct fields.

Also converted byte swapping macros to inline functions for more type safety.
    @seanm
    seanm committed May 27, 2024
    Configuration menu
    Copy the full SHA
    50a56f8 View commit details
    Browse the repository at this point in the history

  2. descriptor: Defer potentially truncating cast to last minute

    @seanm
    seanm committed May 27, 2024
    Configuration menu
    Copy the full SHA
    9cfdddd View commit details
    Browse the repository at this point in the history

  3. descriptor: Restore implicitly casted-away const

    @seanm
    seanm committed May 27, 2024
    Configuration menu
    Copy the full SHA
    58b86fb View commit details
    Browse the repository at this point in the history

  4. descriptor: Fixe potential offsetting of pointer by too much 

    This was checking that `size` is at least `LIBUSB_DT_CONFIG_SIZE` (9) bytes long, but then increments the pointer with `buf += header.bLength`. That could end up pointing past of the end of the buffer. There is a subsequest check that would prevent dereferencing it, but it's still UB to even create such a pointer.

Added a check with a similar pattern as elsewhere in this file.
    @seanm
    seanm committed May 27, 2024
    Configuration menu
    Copy the full SHA
    aa2b85a View commit details
    Browse the repository at this point in the history

  5. descriptor: Small clarifications with no behaviour change 

    All the right hand side is `dev_cap`, changed one outlier to match.

Also clarified the relationships between some magic numbers.

No change in behaviour here.
    @seanm
    seanm committed May 27, 2024
    Configuration menu
    Copy the full SHA
    a50adb0 View commit details
    Browse the repository at this point in the history

  6. descriptor: Fix buffer overincrement in parse_iad_array function 

    The first iteration of this loop was safe because the beginning of the function checked that `size` is at least LIBUSB_DT_CONFIG_SIZE (9) bytes long.

But for subsequent iterations, it could advance the pointer too far (which is undefined behaviour) depending on the content of the buffer itself.
    @seanm
    seanm committed May 27, 2024
    Configuration menu
    Copy the full SHA
    83b6ab1 View commit details
    Browse the repository at this point in the history