drivers/libusb{0,1}.c: bound rdlens loop by element count, not sizeof - #3550
drivers/libusb{0,1}.c: bound rdlens loop by element count, not sizeof#35500x-a6 wants to merge 2 commits into
Conversation
…networkupstools#3136] nut_libusb_open() iterates the two candidate HID report descriptor lengths: int32_t rdlen1, rdlen2, rdlens[2]; size_t j; ... for (j = 0; j < sizeof(rdlens); j++) sizeof(rdlens) is 8 (bytes), not 2 (elements), so the loop reads rdlens[2..7] - 24 bytes past the end of the array - and uses that stack garbage as rdlen. The "ran out of candidates" check after the loop has the same bug. The out-of-range indices are only reached when neither real candidate satisfies the caller, i.e. exactly when a device is in a bad state and both descriptor reads fail. Garbage that happens to pass the rdlen sanity checks is then handed to libusb_control_transfer() and on to the HID parser callback, which is where it segfaults. Seen with usbhid-ups against an EcoFlow DELTA 3 Plus (3746:ffff) on an NVIDIA Jetson Orin Nano whose USB device had wedged; "usbreset" on the device cleared the wedge and hid the crash. Use SIZEOF_ARRAY() from common.h, which both files already include. libusb0.c carries the identical defect and is fixed the same way. Regression from aba6f43 ("if the tried 'rdlen' did not succeed, fall back to the other value we had in mind"), first released in v2.8.5. Signed-off-by: John Grant <john.grant@dronedeploy.com>
|
A ZIP file with standard source tarball and another tarball with pre-built docs for commit c736ec5 is temporarily available: NUT-tarballs-PR-3550.zip. |
|
✅ Build nut 2.8.5.4994-master completed (commit 616551d00b by @0x-a6)
|
|
My bad, thanks for the catch and fix! A news entry would be appropriate, perhaps with a new block like |
…ools#3550] Added under a new "Fix fallout of development in NUT v2.8.5" block, as suggested in review. Refers to the PR rather than the commit that introduced the regression, so no new nut.dict entries are needed. Signed-off-by: John Grant <john.grant@dronedeploy.com>
…3550] Signed-off-by: John Grant <john.grant@dronedeploy.com>
875c9df to
c736ec5
Compare
|
Added to common code, but happy to put it into it's own block. |
|
Thanks, ok for now - I've actually added such a block in #3551 so will (hopefully remember to) relocate your entry there after both PRs get merged. |
|
✅ Build nut 2.8.5.4995-master completed (commit 49b5e096be by @0x-a6)
|
|
✅ Build nut 2.8.5.4996-master completed (commit dbb3df6b8d by @0x-a6)
|
nut_libusb_open()iterates the two candidate HID report descriptor lengths:sizeof(rdlens)is the size in bytes (8), not the element count (2), so the loop readsrdlens[2..7]— 24 bytes past the end of the array — and uses that stack garbage asrdlen. The "ran out of candidates" test after the loop (if (j >= sizeof(rdlens))) has the same bug, so it never fires as intended either.The out-of-range indices are only reached when neither real candidate satisfies the caller — i.e. exactly when a device is in a bad state and both descriptor reads fail, which is when you least want undefined behaviour. Garbage that happens to survive the
rdlensanity checks is then passed tolibusb_control_transfer()and on to the HID parser callback, which is where it segfaults.How it showed up
usbhid-upssegfaulting against an EcoFlow DELTA 3 Plus (3746:ffff) on an NVIDIA Jetson Orin Nano (aarch64, NUT built fromv2.8.5) after the device's USB connection had wedged.usbreseton the device cleared the wedge, and the crash went away with it — the descriptor reads then succeeded on the first candidate and the loop never ran past index 0.Fix
Use
SIZEOF_ARRAY()frominclude/common.h, which both files already include.drivers/libusb0.ccarries the identical defect in the same two places and is fixed the same way.Regression from aba6f43 ("if the tried
rdlendid not succeed, fall back to the other value we had in mind" [#3136]), first released in v2.8.5.Happy to add a
NEWS.adocentry under the 2.8.6 section if you'd like one — I left it out to keep the diff to the fix, since the entry would want this PR's number.