all: introduce syz_usb_finish_probe to improve usb driver fuzzing
CC: @xairy
This change is another step closer to addressing open issues with current state of usb driver fuzzing. Some problems were voiced in issue https://github.com/google/syzkaller/issues/6206, as well as PR: https://github.com/google/syzkaller/pull/6354. There are a few new ones, I'll update the issue soon.
In short, use a modified copy of a generic syz_usb_connect that takes another argument - a list of responses to usb control requests in a form of already existing vusb_responses struct. That way syzkaller knows how to react to non-standard CTRL requests (provided there are necessary descriptions of said requests in syzlang) during syz_usb_connect_scripted. Also, we can specify the number of times each request should be processed to finish driver probe() - syzkaller will keep syz_usb_connect_scripted running until all requests have been accounted for (or something breaks, timeout is hit etc.). Add a couple descriptions as an example, seeds as well.
As this new syscall is just a stepping stone, I've settled on a few compromises:
- keep the new
syz_usb_connect_scriptedseparate: all new functionality may be merged into generic in the future, once all usb driver quirks are figured out. - new syscall and new helper functions are not even remotely great, as they will be subject to change. I think that's somewhat fine, after all the call will have only a limited use as a variant to target specific drivers.
- the only small change that affects existing framework is an extra field in
vusb_response.expectedholds the number of times a request needs to be processed. It is zero by default, does not affect classicsyz_usb_XXXcalls. - no docs just yet. When/if we get coverage from syzbot and everything works, I'll upgrade
syz_usb_connect_ath9kto_scriptedand update the docs. But I'll refer to maintainers on this.
Apart from obvious additions, I've also:
- fixed broken seeds for generic
syz_usb_XXX. - included
linux-firmwarepackage intotools/create-image.shscript. This is important as multiple drivers are not fuzzed solely due to missing firmware binaries. Comedi usb drivers, such as usbdux, are part of this problem and are addressed by this series as well.
Thank you for working on this @fellair!
Keeping syz_usb_connect_scripted separate on the syzlang level for now is fine, but I think the C implementations should be merged. With the current implementation, there's too much C code duplication. Just merge the extra parts into syz_usb_connect_impl and lookup_connect_response_in — they don't seem to be very invasive, so I think they should fit just fine. And I don't see a problem with changing them in the future if we need to.
However, please first see the comment I just left in #6206 about a different pseudo-syscall design idea. Sorry about not thinking of that approach before. But I like it considerably more than extending syz_usb_connect.
If you think that other approach makes sense and have the energy to rework this pull request — please do.
Otherwise, since you already wrote the code and the syzlang description to implement syz_usb_connect_scripted, I think we can keep it (but please merge the C implementations). But splitting out the _scripted part (optionally, with the support for handling non-control requests) into syz_usb_finish/driver_probe is something that I think would make sense to do next.
Per conversation in https://github.com/google/syzkaller/issues/6206#issuecomment-3330916129, I'll rework this change to adopt @xairy's suggestion over current version of syz_usb_connect_scripted.
I think, if no one objects, I'll do the transition in this PR instead of creating a new one.
Cc: @xairy
2nd version of the approach to deal with select usb drivers that require numerous CTRL-request during probe.
- use
syz_usb_finish_probeinstead ofsyz_usb_connect_scriptedintroduced earlier. It can take over aftersyz_usb_connectsetups initial steps and goes through required CTRL-requests until all of them are done or the call times out. Less duplicate code (although there is still some - that is by design FOR NOW), more lightweight. The goal and method is still very much the same. Only control requests for now, I am currently working on BULK/INT. - remove
syz_usb_connect_ath9kand switch to variant ofsyz_usb_finish_probeinstead. Same result. - fix seeds, add a couple of examples for syz_usb_finish_probe.
I'd prefer to update docs AFTER this PR is pushed to see that coverage is improved similarly to my setup. Or I'll leave it to @xairy, whichever is best. Maybe even after non-control requests are accounted for as well...
P.S. Unified diff for https://github.com/google/syzkaller/pull/6372/commits/30cf8eba2a8f7273471ad7161a5bd73884cb7ab0 is a bit chaotic, apologies for that.
Looks much better! I left some comments/questions.
Less duplicate code (although there is still some - that is by design FOR NOW)
Yes, definitely some duplication between the new code and parts of syz_usb_connect and syz_usb_control_io. Addressing that would be a good next change.
I'd prefer to update docs AFTER this PR is pushed to see that coverage is improved similarly to my setup
Yeah, sure, but please still add a note to the documentation saying that this PR introduces new feature, which will be documented later.