syzkaller icon indicating copy to clipboard operation
syzkaller copied to clipboard

all: introduce syz_usb_finish_probe to improve usb driver fuzzing

Open fellair opened this issue 3 months ago • 4 comments

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_scripted separate: 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. expected holds the number of times a request needs to be processed. It is zero by default, does not affect classic syz_usb_XXX calls.
  • no docs just yet. When/if we get coverage from syzbot and everything works, I'll upgrade syz_usb_connect_ath9k to _scripted and 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-firmware package into tools/create-image.sh script. 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.

fellair avatar Sep 22 '25 16:09 fellair

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.

xairy avatar Sep 24 '25 22:09 xairy

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.

fellair avatar Sep 25 '25 13:09 fellair

Cc: @xairy

2nd version of the approach to deal with select usb drivers that require numerous CTRL-request during probe.

  • use syz_usb_finish_probe instead of syz_usb_connect_scripted introduced earlier. It can take over after syz_usb_connect setups 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_ath9k and switch to variant of syz_usb_finish_probe instead. 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.

fellair avatar Nov 14 '25 14:11 fellair

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.

xairy avatar Nov 26 '25 16:11 xairy