all: introduce syz_usb_finish_probe to improve usb driver fuzzing #6372
Conversation
|
Thank you for working on this @fellair! Keeping 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 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 |
|
Per conversation in #6206 (comment), 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. |
Add a new pseudo-syscall syz_usb_finish_probe in order to increase coverage in usb drivers that require processing of driver-specific control requests between drivers and devices. Also, change syz_usb_connect_ath9k to new pseudo-call format. This is another step in addressing open problems partially discussed in issue [1]. The idea is to describe variants of this call in syzlang for drivers that lack in coverage to all but ensure a successful probe. Once syz_usb_connect performs initial steps towards usb device setup, including dealing with SET_CONFIGURATION request, syz_usb_finish_probe takes over to process a set of driver-specific CTRL-requests. These requests are described in syzlang's vusb_responses_XXX struct. In essence, syz_usb_finish_probe acts as a more consistent version of syz_usb_control_io call, except the former can also take both in/out requests and keeps going until it either times out, or all the reqs have been processed. Moreover, some requests deemed important enough (for instance, those that are necessary to be processed in order to properly finish probing) can be affixed with a number of times they should be dealt with by syzkaller before finishing device setup and configuraion. This is quite useful for dealing with drivers that send/receive multiple similar requests. Note: as stated before, this is a transitionary change on the way to implementing a universal approach that takes into account all possible usb driver quirks, of which are many. Note: to avoid duplicating too much code between multiple pseudo-calls I've made some changes to generic usb functions like lookup_connect_response_in. That required non-functional changes to generic calls like syz_usb_connect in common_usb_[linux|netbsd].h. [1] google#6206
Update select seeds that have broken due to changes to vusb_responses struct. This is caused by introduction of a new field 'expected' to struct vusb_response. This field holds the number of times syzkaller is required to process a specific request before finishing syz_usb_finish_probe and, in turn, driver probe itself. No functional changes were made to the way generic syz_usb_connect works.
Add descriptions for variants of generic syz_usb_connect and new syz_usb_finish_probe pseudo-syscalls to target 3 underperforming (coverage-wise) usb drivers. Specifically, concentrate on these modules: - drivers/net/usb/r8152.c Interestingly, there are CTRL-requests before *and* after SET_CONFIGURATION directive. - drivers/usb/misc/legousbtower.c Basic example of a driver that needs help to pass early interface and endpoint checks, as well as a mandatory control request. - drivers/comedi/drivers/usbdux.c Of particular interest is a mandatory standard USB_REQ_SET_INTERFACE request. Requires firmware from linux-firmware package. Also, add seeds for these descriptions. Focus on syz_usb_finish_probe calls, rather than syz_usb_control_io, as only they are capable of properly broadening code coverage in these cases.
Multiple usb drivers (among others) are missing firmware thus
stopping short any probe() attempts.
A couple examples are:
1) comedi usb drivers: usbdux, usbduxfast etc.
2) drivers/media/usb/{s2255,go7007}
and a few others at the very least.
Add the required module 'firmware-linux' (carries both free and
nonfree batches of built firmware) to the list of default packages.
This is not an ideal solution considering the goal of a *minimal*
distribution. Still, this will increase code coverage.
fellair
force-pushed
the
fellair_vusb_add_new_pseudocall_v2
branch
from
75d0a7e to
67e268c
Compare
fellair
changed the title
|
Cc: @xairy 2nd version of the approach to deal with select usb drivers that require numerous CTRL-request during 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 30cf8eb is a bit chaotic, apologies for that. |
| default: | ||
| break; | ||
| } | ||
| if (!resps || !done) |
There was a problem hiding this comment.
I think this part of code belongs to a new function, named e.g. lookup_probe_response_in. The code above does not use resps or done in any way.
And then in syz_usb_connect you just call lookup_connect_response_in like before, and in syz_usb_finish_probe you call both sequentially (but do you actually need the lookup_connect_response_in part in syz_usb_finish_probe? why?).
| if (!resps) | ||
| break; | ||
|
|
||
| resps_num = (resps->len - offsetof(struct vusb_responses, resps)) / sizeof(resps->resps[0]); |
There was a problem hiding this comment.
Same here, put this part into lookup_probe_response_out.
| syz_usb_control_io$cdc_ncm(r0, 0x0, 0x0) | ||
| syz_usb_control_io$cdc_ncm(r0, 0x0, 0x0) | ||
| syz_usb_control_io$cdc_ncm(r0, 0x0, &(0x7f0000000340)={0x44, 0x0, 0x0, 0x0, &(0x7f0000000200)={0x20, 0x80, 0x1c, {0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x10}}, 0x0, 0x0, 0x0, 0x0}) | ||
| syz_usb_control_io$cdc_ncm(r0, 0x0, &(0x7f0000000340)={0x44, 0x0, 0x0, 0x0, &(0x7f0000000200)={0x20, 0x80, 0x1c, 0x0, {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}}, 0x0, 0x0, 0x0, 0x0}) |
There was a problem hiding this comment.
Why were 0x10s changed to 0x0s?
|
Looks much better! I left some comments/questions.
Yes, definitely some duplication between the new code and parts of
Yeah, sure, but please still add a note to the documentation saying that this PR introduces new feature, which will be documented later. |
2 participants
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 #6206, as well as PR: #6354. There are a few new ones, I'll update the issue soon.
In short, use a modified copy of a generic
syz_usb_connectthat takes another argument - a list of responses to usb control requests in a form of already existingvusb_responsesstruct. That way syzkaller knows how to react to non-standard CTRL requests (provided there are necessary descriptions of said requests in syzlang) duringsyz_usb_connect_scripted. Also, we can specify the number of times each request should be processed to finish driverprobe()- syzkaller will keepsyz_usb_connect_scriptedrunning 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:
syz_usb_connect_scriptedseparate: all new functionality may be merged into generic in the future, once all usb driver quirks are figured out.vusb_response.expectedholds the number of times a request needs to be processed. It is zero by default, does not affect classicsyz_usb_XXXcalls.syz_usb_connect_ath9kto_scriptedand update the docs. But I'll refer to maintainers on this.Apart from obvious additions, I've also:
syz_usb_XXX.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.