bhyve: Validate VirtIO queue guest addresses

[hayzam_gmail.com]

Summary

paddr_guest2host() returns NULL when a range does not fit within guest
RAM. An unmappable queue address previously left the queue marked
allocated with ring pointers computed from the failed mapping. Check
the legacy virtqueue ring mapping and indirect descriptor table mapping
before dereferencing them, and ignore notifications for unallocated
queues. Treat a zero PFN as a queue disable, leave an invalid queue
unallocated, and return an error for an unmappable indirect descriptor
table.

Also check queue readiness in the virtio-net RX path, which can be
reached from the backend rather than from a notification, including
after snapshot restore. Disable backend RX notifications until the
queue is ready to avoid repeatedly handling unread packets.

Sponsored by: The FreeBSD Foundation

[markj]

usr.sbin/bhyve/pci_virtio_net.c

250 ↗

(On Diff #187426)

This check is racy though: what happens if the queue is disabled right after this check? Is the race acceptable for some reason?

I wonder if we instead want to notify the driver whenever a queue is disabled. The device model can then do whatever's needed in order to synchronize its I/O paths. In particular, it'd be nice to drop that racy vq_ring_ready() check from vq_has_descs().

[hayzam_gmail.com]

Address review feedback, drop the virtio-net RX hunk.

[hayzam_gmail.com]

This check is racy though: what happens if the queue is disabled right after this check? Is the race acceptable for some reason? I wonder if we instead want to notify the driver whenever a queue is disabled. The device model can then do whatever's needed in order to synchronize its I/O paths. In particular, it'd be nice to drop that racy vq_ring_ready() check from vq_has_descs().

Agreed, I've dropped the virtio-net RX hunk. For virtio-net, the remaining notify guard is serialized with PFN writes under vs_mtx. I'll try to pursue the queue-disable notification and backend synchronization in a separate patch.

[markj]

usr.sbin/bhyve/virtio.c

196

Why do we disable the queue in this case? Why not ignore the write (and print an error message)?

This patch says it is validating guest-controlled addresses, but it's also introducing new functionality here, I believe. I'd rather handle that in a separate patch.