Conversation
An endpoint's wMaxPacketSize was stored exactly as the device reported it. For a hub's status-change endpoint that value becomes the buffer length the SIE is told it may write into the 64-byte DPRAM window at INT_EP_BUF_OFFSET. A descriptor claiming more than 64 therefore lets the controller run past the end of that window. A value too large for the 10-bit length field also collides with the control bits sharing the register. Keep only the size field. Bits 12:11 are the high-speed transaction count, not part of the size. Cap the result at the 64-byte full-speed maximum this controller is wired for, and leave a zero-length endpoint unrecorded so the existing completeness checks reject the device instead of arming a pipe that carries nothing. hcd_int_ep_install caps the length as well, so the hardware boundary does not rely on its caller. usb_parse.c had no host coverage. tests/test_usb_parse.c adds it: descriptor walking for the mass-storage and hub paths, alternate settings, truncated and malformed TLVs, the packet-size bounds above, and CBW/CSW round-trips.
ir_decode_nec, ir_decode_samsung32, ir_decode_rca and ir_decode_kaseikyo checked a minimum edge count and then read only the entries their protocol uses. A capture carrying a good frame plus extra edges, which is what a second burst, an echo or a noise glitch inside the same capture window looks like, decoded as a clean result and the extra edges were never examined. The framer drops the terminating space, so a captured frame has exactly the edge count its protocol defines: 67 for NEC and Samsung32, 51 for RCA, 99 for Kaseikyo. Compare against that count instead. ir_decode_sirc and ir_decode_rc5 already did, and RC6 self-limits through its half-bit accounting, so this brings the remaining four in line. The NEC repeat frame is matched before the data-frame check and is unaffected. tests/test_ir_strict_length.c covers all four: a valid frame still decodes, one extra edge or a spurious appended burst does not, a frame missing its stop mark does not, and the NEC repeat still decodes while a padded repeat does not. The dispatcher and ir_decode_all are checked too, since a padded frame previously produced a confident match there.
check_app_uf2 unpacked the family ID out of every block header and then discarded it, so the only thing standing between a foreign image and the SD card was the address windows. Those do not separate chips: RP2040 SRAM also starts at 0x20000000, so an RP2040 build lands inside the SRAM app window and passed both the POST_BUILD gate and the pre-mount check in fw install-app. make_app_uf2.py stamps every block it emits with the family-ID flag and the RP2350 Arm-secure family, so when a block declares a family it can be held to that one. Blocks that do not set the flag are left alone: the field is not a family there, so it carries no claim to check. Covered in tools/tests/test_fw.py: an RP2040 image inside the SRAM window is rejected, an RP2350 one is accepted, and a block without the flag still parses as before.
A CIC integrator is supposed to wrap. The comb stage subtracts the wrapped values back out, which is why the filter stays correct across the boundary. The accumulators were int32_t, so that wrap was signed overflow, and signed overflow is undefined rather than modular. This is not a corner case on this board. pdm_capture starts the filters once and free-runs them for the life of the session, and any real microphone signal carries enough bias to walk the integrators to the end of their range within seconds. Built with -fsanitize=undefined and fed a biased PDM stream, the first integrator trips the sanitizer well under a second of audio. Holding the accumulators in uint32_t makes the wrap defined and leaves the arithmetic identical: the same stream over three million bits produces the same samples before and after, and the sanitizer stays quiet.
Nothing ran the tests automatically, so a regression in the host tree only surfaced when someone remembered to run fw test locally. The tree needs no Pico SDK and no board, so a stock Linux runner covers all of it. Two gaps in what fw test itself reached, fixed here so the workflow and a local run cover the same ground: tools/tests was only partly wired in. test_uf2_info and test_make_app_uf2 ran; test_fw and test_check_app_repo did not, which left the fw CLI's flash-protection and install-app guards out of the suite. Those guards decide whether a UF2 may reach the card at all, so fw test should be the thing that says they still hold. They now run as python_tools alongside test_no_private_refs. test_no_private_refs also skipped skills/. Those files ship in the repo and are written for agents to follow, which is the same reason the scanner covers docs/. AGENTS.md stays out: a test already pins it as a non-public file.
The README pinned the host tree at 26 green binaries, which stopped being true as tests were added. It now points at fw test rather than carrying a number that has to be maintained by hand. fw2kb_chord_for's comment says five hops where the loop runs six. The bound is fine either way, since every append is guarded against FW2KB_CHORD_MAX, but the comment is what a reader checks that headroom against.
ft6336_poll promises screen coordinates inside 480x320, and a real touch gets them because ft6336_map_point clamps on the way through. An injected point skipped that: agentio parses TCH into a uint16_t and handed it straight to ft6336_inject_set, so a command naming 60000 50000 reported a touch two orders of magnitude off the panel to whatever was polling. No current consumer turns that into a bad access. st7796_fill_rect clips its own rectangle and retrochat range-checks before indexing, so this is a broken promise rather than a live crash. It is the kind a later consumer inherits silently, though, since the header says the range is already bounded. The clamp moves into ft6336_clamp_point so both paths share it, and injection now goes through it. tests/test_ft6336_map.c is new: the mapping had no host test despite the driver comment describing it as host-tested logic. It covers the rotation, both clamped edges, and the widest value a TCH command can carry. The injection call itself is by inspection, since ft6336.c needs the Pico SDK and does not build on the host.
audio_i2s_duplex_play_stream_loop requires a length that is a non-zero multiple of 8192 and at least 16384. The second DMA channel is armed one chunk into the buffer and the refill advances by whole chunks modulo the length, so a shorter or unaligned buffer points a chunk-sized read past the end of it, and a zero length divides by zero in the interrupt. Nothing violates that today: the one caller pads up to a multiple of 8192 and says so in a comment at the call site. The requirement was only recorded there, though, while the sibling play_loop documents its own alignment rule in this header. Stating it here is where the next caller will look.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I went looking for things the host tree could prove, since I have no FreeWili 2
and no probe here. Everything below is verified with
fw teston Linux. Nothingin it has been near a board, so the hardware-facing parts are reasoned from the
code and I have said so where that matters.
Four fixes, each with the test that catches it:
wMaxPacketSizewas taken from the device as-is (bsp/usbhost). For a hub'sstatus-change endpoint that value becomes the buffer length the SIE may write
into the 64-byte window at
INT_EP_BUF_OFFSET, so a descriptor asking for morethan 64 runs past it, and a value too wide for the 10-bit length field lands on
the control bits next to it. Now masked to the size field, capped at the
full-speed maximum, with a zero-length endpoint left unrecorded so the existing
completeness checks reject the device.
hcd_int_ep_installcaps it too.usb_parse.chad no host coverage at all, so there is a new test file for it.Four IR decoders accepted frames longer than the protocol (
bsp/ir). NEC,Samsung32, RCA and Kaseikyo checked a minimum edge count and read only the
entries they needed, so a capture holding a good frame plus a second burst or a
noise glitch decoded as clean.
ir_framedrops the terminating space, so thecount is exact and they can compare against it. SIRC and RC5 already did, RC6
self-limits. The NEC repeat frame is matched earlier and still decodes.
A UF2 built for another chip passed
check_app_uf2(tools/fw.py). Thefamily ID was unpacked and dropped, and the address windows do not separate
chips: RP2040 SRAM also starts at
0x20000000, so an RP2040 image sits insidethe SRAM app window. Blocks that declare a family are now held to the RP2350
Arm-secure one. Blocks without the flag are untouched.
The CIC accumulators were signed (
bsp/dsp). The filter wants theintegrators to wrap and the comb stage takes the wrap back out, but wrapping a
signed type is undefined rather than modular.
pdm_capturefree-runs these forthe session and any real mic bias walks them to the end of their range in
seconds. Under
-fsanitize=undefinedwith a biased PDM stream the firstintegrator trips well inside a second of audio. Holding them in
uint32_tmakesthe wrap defined and the arithmetic identical: same samples over three million
bits, sanitizer quiet.
Also here:
TCH 60000 50000reported a point far off the panel. Nothing currently turns that into a bad
access, so it is a broken promise rather than a crash. The clamp is shared
now, and the mapping has the host test the driver comment already described.
it on push and PR. I left Windows out rather than add a job I cannot try.
fw testwas only running part oftools/tests.test_fwandtest_check_app_repowere outside it, which left the flash-protection andinstall-app guards out of the suite. They run now.
test_no_private_refsdid not scanskills/. AGENTS.md stays out, a testpins it as non-public.
fw2kbsays five hops where the loop runs six.
audio_i2s_duplex_play_stream_loopneeds a length that is a non-zeromultiple of 8192 and at least 16384, or a chunk-sized read points past the
buffer. Nothing violates it today, and the one caller pads, but the rule
only existed in a comment at the call site while the sibling
play_loopdocuments its own rule in the header. Documented, not guarded, since I
cannot build the target here to check a guard.
One thing I found and did not fix, because it needs a board to confirm and to
not make worse:
fw2_recovery_sd_sendcalls straight intouart_write_blockingon a CTS/RTS link, withfw2_app_recovery_task()onlyafter it returns. The read side deliberately slices into short polls for exactly
this reason. If MAIN stops asserting CTS mid-write, a HOME hold looks like it
should reboot and does not. Happy to open it as an issue instead if that is
easier to track.
fw test: 49/49 green, from a clean tree.