sys/linux: enhance usb driver fuzzing

[fellair]

TL;DR: syzkaller needs to be tuned in a few ways to expand coverage of select usb drivers.

There are a few usb drivers that either have not been properly covered from the start or have lost coverage over time (judging from 8-month long stats derived from syzbot - I used it to identify flaws listed below). I've done a quick, incomplete audit to determine the possible reasons and their solutions. Some issues are already mentioned in #533.

1. Failed usb interface/endpoint checks (or other settings like USB speed)
Examples:
rtl8150 - lost coverage; ironically, syzkaller can't pass a check made by me...
au0828 - lost coverage; mostly breaks due to incorrectly set USB speed.
+others like radio-shark, radio-shark2, redrat3 etc.

Q: Ideally, we need to tweak generic syz* calls that deal with connecting to increase chances of correct ennumeration (better mutation and whatnot). But in the meantime, should we describe syz_usb_connect$ variants (plus others), similar to ath9k but without modified pseudo-calls, to gain coverage?

2. Failed due to a number of early CONTROL requests in both directions during probe.
Examples:
cx231xx - probe can't finish; syzkaller tries to react to READ requests but doesn't seem to succeed fully.
lan78xx - syzbot shows failure at ep check time; however, the main issue is during probe() there are tens of ctrl requests, in and out. Having experimented a bit, I've come to a conclusion that the sheer number of requests is so high that fuzzer may not want to generate essentially the same syscalls in a row, thus stoping probe() half way. It will generate a program with ~15 syz_usb_control_io$, when 30 is necessary. I've tried manually crafting a program to push it further but read requests are tricky. And that's on top of connect$ and control_io$ call variants tailored for lan78xx.
+some others.

Q: Describing syscall variants$ for drivers with such specific issues should help somewhat. Is it a good idea? As for lan78xx, I wonder if a single pseudo-call that loops for ~30 secs and sends/receives requests until probe is finished is in order?

3. r8152 device needs to handle a READ request early.
See usb_control_msg here. It's similar to ath9k case, just a different direction. In my view, this necessitates describing a separate syz_usb_connect pseudo-call for r8152 as well as changing syz_usb_connect_impl() call to take a different lookup_connect_response_in() (not _out) function as an argument.

Q: Is this solution sound?

4. Missing firmware.
Examples:
s2255 - again, usb ep checks fail, but firmware also can't be found.
go7007 - same case.

I don't remember why firmware may not be found but I imagine the fault is either with missing config options or with missing packages from the image. This can be easily remedied.

Provided my suggestions are deemed sensible, I'll start implementing them. Thank you for your time.

[a-nogikh]

Cc @xairy

[dvyukov]

Hi @fellair Thanks for looking into this!

But in the meantime, should we describe syz_usb_connect$ variants (plus others), similar to ath9k but without modified pseudo-calls, to gain coverage?

Just adding specialized versions in descriptions looks good to me.
Adding lots of special C code would be more questionable. But only descriptions seems fine.

Q: Describing syscall variants$ for drivers with such specific issues should help somewhat. Is it a good idea?

Again, some amount of descriptions looks reasonable.
Perhaps you could start with 1 driver only, so that we can assess better how exactly it will look like.

Also, have you considered adding new seeds in sys/linux/test/ (sys/linux/test/vusb*)?

As for lan78xx, I wonder if a single pseudo-call that loops for ~30 secs and sends/receives requests until probe is finished is in order?

I am out of context now to answer this. Not sure if @xairy still have any context on this.
Do you see other options? Is it better to create a background thread in e.g. syz_usb_connect? initialize_vhci creates such a thread to answer kernel requests.

Q: Is this solution sound?

Also don't have context now.

4. Missing firmware.

This sounds good.
Enabling relevant configs is always good.
For packages we can check if they are provided by buildroot, or if there are other ways to install them in the image.

Btw, IIRC we enable USB configs based on distro configs in dashboard/config/linux/distros/* (IIRC we enable only the ones used by real distros). Perhaps adding more fresh distro configs will be useful to enable more drivers as well.

[xairy]

Awesome analysis @fellair, thank you!

With regards to the drivers that fail various USB descriptor checks (speed, endpoints, etc.) — adding new syz_usb_connect$ variants is the way to go. These should fit just fine within the existing syzlang framework for describing USB devices.

One note here is that for some drivers it makes sense to add two syz_usb_connect$ variants: one that would sometimes generate proper USB descriptors to pass the driver's probing but would also sometimes fail to explore various error-checking paths; and another one that would pass the probing most of them time to allow syzkaller fuzzing the post-probing communication. But adding the first variant likely only makes sense if the probing code allows various permutations of interfaces/endpoints, which is usually only the case for class-specific drivers (e.g. UVC) and not device-specific drivers.

As for the atk9k-like cases like lan78xx and r8152 that have an unusual probing process with extra control requests: I think the best way would be to extend the existing framework to allow describing responses to these requests.

We already have a way to describe the pre-SET_CONFIGURATION requests as fields within vusb_connect_descriptors (none of the class/driver-specific descriptions use this feature at the moment though). This does not allow specifying the order of the responses if there are multiple requests of the same type, but I don't know whether we need this at all.

For the requests that must be always handled immediately after SET_CONFIGURATION, we can rework and extend syz_usb_connect_ath9k. We can rebrand it as something like syz_usb_connect_scripted that accepts another argument that specifies in syzlang the responses to post-SET_CONFIGURATION control requests and which one them is the last one (i.e., the condition for exiting the pseudo-syscall). And then we can port the hardcoded responses from the C implementation of syz_usb_connect_ath9k into syzlang and extend the functionality based on the specific new cases we encounter.

Overall, as @dvyukov suggested, if you want to work on improving USB fuzzing, you can start with adding USB descriptions for one of the simpler drivers that you analysed.

Adding new sys/linux/test/ (sys/linux/test/vusb*) seeds is an alternative approach to all this, but I think extending the syzlang descriptions and the pseudo-syscall implementations is a better approach. However, I do encourage you to add seeds based on the new USB descriptions that you add. This would allow testing their correctness and catching potential descriptions breaking changes in the future.

[fellair]

Thank you all for your insights!

With regards to the drivers that fail various USB descriptor checks (speed, endpoints, etc.) — adding new syz_usb_connect$ variants is the way to go. These should fit just fine within the existing syzlang framework for describing USB devices.

One note here is that for some drivers it makes sense to add two syz_usb_connect$ variants: one that would sometimes generate proper USB descriptors to pass the driver's probing but would also sometimes fail to explore various error-checking paths; and another one that would pass the probing most of them time to allow syzkaller fuzzing the post-probing communication. But adding the first variant likely only makes sense if the probing code allows various permutations of interfaces/endpoints, which is usually only the case for class-specific drivers (e.g. UVC) and not device-specific drivers.

Adding new sys/linux/test/ (sys/linux/test/vusb*) seeds is an alternative approach to all this, but I think extending the syzlang descriptions and the pseudo-syscall implementations is a better approach. However, I do encourage you to add seeds based on the new USB descriptions that you add. This would allow testing their correctness and catching potential descriptions breaking changes in the future.

I think I'll start small and tackle these first.

As for the atk9k-like cases like lan78xx and r8152 that have an unusual probing process with extra control requests: I think the best way would be to extend the existing framework to allow describing responses to these requests.

We already have a way to describe the pre-SET_CONFIGURATION requests as fields within vusb_connect_descriptors (none of the class/driver-specific descriptions use this feature at the moment though). This does not allow specifying the order of the responses if there are multiple requests of the same type, but I don't know whether we need this at all.

For the requests that must be always handled immediately after SET_CONFIGURATION, we can rework and extend syz_usb_connect_ath9k. We can rebrand it as something like syz_usb_connect_scripted that accepts another argument that specifies in syzlang the responses to post-SET_CONFIGURATION control requests and which one them is the last one (i.e., the condition for exiting the pseudo-syscall). And then we can port the hardcoded responses from the C implementation of syz_usb_connect_ath9k into syzlang and extend the functionality based on the specific new cases we encounter.

This is very interesting, I think it's worth exploring... I will get on that!