Kafl/fuzz 5.15 4 add virtio cache

[williamcroberts]

Updated and ready for review

[ereshetova]

@williamcroberts thank you! I have taken a look on the code and wondering if doing it this way would limit the fuzzing options considerably. You set a single u64 variable in per device cache, which is reset upon swiotlb bounce event. However, even the same device can consume more than u64 of input from swiotlb buffer to be used for different purposes. See here for example virtio-console handling: https://elixir.bootlin.com/linux/latest/source/drivers/char/virtio_console.c#L1578: it would first read the cpkt->event and then later on cpkt->id (this is all part of the same bounce buffer) and then does some processing on these values. But with the current cache implementation it would get the same fuzz value for both event and id (ok not the same exactly given that they are u16 and u32, but you get the idea). However, i dont know how to fix this easily: making bigger cache with filled fuzz input wont be an issue, but it would also require tracking what value has been asked from this cache....

[ereshetova]

One silly idea I got maybe in tdx_fuzz_virtio_cache_get_64 keep the mapping orig_v --> index into fuzz buffer of u64s, i.e. allocate big enough fuzz buffer on switlb bounce and then every time a value is asked from this fuzz buffer use orig_v as index into it? I think it might work without requiring of keeping track of proper swiotlb buffer indexes

[williamcroberts]

One silly idea I got maybe in tdx_fuzz_virtio_cache_get_64 keep the mapping orig_v --> index into fuzz buffer of u64s, i.e. allocate big enough fuzz buffer on switlb bounce and then every time a value is asked from this fuzz buffer use orig_v as index into it? I think it might work without requiring of keeping track of proper swiotlb buffer indexes

Yes I was thinking about how this limits granularity of the fuzzing, we could even do a get16, get32, get64 so we have 3 values per virtio buffer and then we could map from there as well.

A initial goal was to avoid locking as I was unsure of the locking model in this device tree, so a simple atomic64 avoided that. i'd have to take a look deeper into the locking mechanism.

[williamcroberts]

@ereshetova that was a good thought, not silly. So now I see better fuzz granularity and it throwing more virtio errors:

virtio_console: probe of virtio2 failed with error -2
tail: /var/run/shm/kafl_bill/hprintf_00.log: file truncated
BILL virtio fuzz cache: get u32 offset: 512.
virtio_console: probe of virtio1 failed with error -22
tail: /var/run/shm/kafl_bill/hprintf_00.log: file truncated

Let me do some cleanups and ill update the PR.

[williamcroberts]

So in test BPH_VIRTIO_CONSOLE_INIT, I see it init the cache three times but the buffer never gets refreshed through swiotlb_bounce so our cache never updates which, is what folks say should be correct, but you'll definitely lose the amount of stuff you can actually fuzz per VM start.

I also don't think we need any locking as init is guaranteed to run before anything else because it happens in virtio device creation, and since swiotlb_bounce and endian converting (reads) from the buffer have to mutually exclusive, I think we're safe.

[ereshetova]

The new code looks great to me! I think it provides both correct way of synching cache and also enough fuzzing granularity.
About the virtio-console. I looked into the code now (and want to write this down here before I forget myself how it works) and I am not convinced that BPH_VIRTIO_CONSOLE_INIT is the best way of testing this. virtio-console has two types of data it exchanges with the (untrusted host): normal data (chars on console, result back) and control data. The normal data is transmitted in terms of two virtques (send & recv) per each configured port (up to 16 ports). In our virtio configuration, on the guest side the incoming data from each configured virtque is read via virtqueue_get_buf_ctx_split(). There, it reads two control values (index i & len) and then the actual pointer on data buffer is returned all the way up to the fill_readbuf and get_chars that is used by hvc console driver on the guest. So it looks to me that the proper function to kprobe for fuzzing the runtime exchange of data for virtio console is get_chars and then apply the heavy load on virsh console traffic. This should force the bounce buffer regular refreshing/bouncing due to console interaction. The other type of exchanged data is control data and processing of control messages happens in handle_control_message, but control data is only exchanged when some configuration is changed or set up, i.e. data ports added or removed, resizing, etc. So it looks to me that in order to force a new copy of bounce buffer here, it might not be enough to just run through virtio console initialization, but would need to trigger some control messages from the host. @sirmc, you were looking into virtio previously, did I get this right?

[ereshetova]

@sirmc, one more thing I noticed now while reading virtio-console code in details is that in some cases (like for that control message handling), we already populate the virtqueue_get_buf with fuzz input and then this buffer arrives to handle_control_message, some of its parts like id and event are fuzz injected again via virtioxx_to wrappers. This feels like another problem with our virtio fuzzing: such thing cannot happen in real life interaction with the host. The correct way seems to be fuzzing virtqueue_get_buf (and it takes care of double fetches itself because virtio driver code doesnt seem to be reading the buffers twice, too heavy on its own) only for virtio drivers themselves, but then fuzzing virtioxx_to* wrappers for virtio ring itself (using proper cache that Bill implemented now to avoid double fetches). Or did I misunderstand smth on why we were simultaneously fuzzing both virtqueue_get_buf & virtioxx_to_* in virtio drivers?

[ereshetova]

And even one more issue, now about accessing virtio config space for virtio devices. When a virtio device (like virtio-console) uses virtio_cread to get data from its config space, it will go down to virtio core and eventually invoke config->get which is mapped to vp_get and eventually ends up in different type of ioread. We already fuzz ioreads from a non-virtio hooks, so the fuzz input will be consumed correctly here. But when this fuzz input propagates up all the way to virtio_cread macro, it will get substituted by another fuzz input from virtio_to_cpu as the last thing before returning this input to virtio device. Here the substitute itself was not a problem previously since we always consumed a new input on each invocation of virtio_to_cpu (it was just silly i guess that we substituted one fuzz input with another one, but harmless), but now when we limit the fuzz input via the swiotlb cache it becomes not correct imo, because ioreads down are not done via swiotlb but straight with tdvmcalls, so we loose some fuzzing granularity here. In scenarios when host will be constantly updating config space for a given virtio driver but not using swiotlb for any other activity, we will see that virtio fuzz cache wont refresh and we will end up using the same buffer for fuzzing. On the positive side, since the ioreads underneath will be feeding different fuzz values, we will be indexing different offsets in virtio fuzz cache, so we will be somewhat varied here nonetheless. That said, I guess the best would not to fuzz the virtioxx_to_ wrappers for config space access anyhow.

[sirmc]

@ereshetova I agree with your points. I really think the virtio_to_XX interception is not the ideal way to fuzz this, but we added it a a simple, low-effort way. The proper way would be to mark certain memory ranges as DMA/ host-shared and then trap on any access to those memory ranges (i.e., enforce a page fault by unmapping the region). But at the time we deemed that too much work for something that we might not need to use.

Re control messages, a similar setup to the VSOCK fuzzing (as described here https://intel.github.io/ccc-linux-guest-hardening-docs/tdx-guest-hardening.html#perform-driver-fuzzing), should be able to generate some control messages from the host. However, this fuzzing setup is a bit finnicky, since it requires an external stimulus in the form of a client/ server program on the host and guest.

I added support for virtqueue_get_buf fuzzing, since there were a bunch of parts not really covered by the virtioxx_to_ wrappers. It might make sense to run harnesses with these hooks separately, individually enabled. I think our previous assumption was that virtioxx_to_ wrappers always accessed shared/ host memory, so could be changed at any time, but since this assumption is not true we should probably be a bit more conservative and use caches as you suggested above.

[ereshetova]

@sirmc thank you for confirming my understanding! I think the assumption that virtioxx_to* wrappers access shared/dma memory mostly holds, the only clear exception i can see so far are config space access. So, i think when we enable boot style harnesses and want to cover various virtio devices probe functions, the best way is probably disabling input injection via virtioxx_to_* wrappers fully and rely on config space being injected via ioreads and virtqueue_get_buf for the rest (exchange of control messages during initialization and actual buffer data). But then we want to fuzz some virtio runtime interaction, then we should enable imo only virtioxx_to* wrappers (with Bill's cache) for higher fuzzing efficiency because both virtio core and virtio drivers seem to be using only the parts that they take out of these drivers and convert using these wrappers. Rest goes as payload towards upper layers and it is pointless to fuzz it since it should be protected by other means and no subject to host malicious input. I will double check this logic with rest of virtio drivers we use and then create a jira to adjust virtio harnesses accordingly.

[ereshetova]

But the main bottom line is that both virtioxx_to* and virtqueue_get_fuzz injection should not be enabled at the same time because it will generate us false positives also.

[williamcroberts]

@ereshetova do you need an substantive code changes or can I drop this and move on :-p

[ereshetova]

@williamcroberts I think this is good! The rest i will handle via configs, etc. Thank you very much for helping!