bhyve: Validate VirtIO queue guest addresses
ClosedPublic
Actions

Authored by hayzam_gmail.com on Tue, Sep 22, 2:53 PM.

Details

Reviewers

markj

Group Reviewers

bhyve

Commits

rG4485bfaed214: bhyve: Validate VirtIO queue guest addresses

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 before updating the queue state, and
ignore an unmappable PFN write after printing a diagnostic. This leaves
an unallocated queue unallocated and preserves an existing queue's PFN,
ring pointers, flags, and indices.

Also check the indirect descriptor table mapping before dereferencing
it, returning an error if the table is unmappable, and ignore guest
queue notifications for unallocated queues.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 77186
Build 74069: arc lint + arc unit

Event Timeline

hayzam_gmail.com created this revision.Tue, Sep 22, 2:53 PM

Herald added a reviewer: bhyve. · View Herald TranscriptTue, Sep 22, 2:53 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

hayzam_gmail.com requested review of this revision.Tue, Sep 22, 2:53 PM

Harbormaster completed remote builds in B77186: Diff 187426.Tue, Sep 22, 2:53 PM

markj added inline comments.Wed, Sep 23, 4:34 PM

usr.sbin/bhyve/pci_virtio_net.c
250	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()`.

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

Harbormaster completed remote builds in B77214: Diff 187514.Wed, Sep 23, 5:34 PM

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 added inline comments.Wed, Sep 23, 8:27 PM

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.

Address review comments

Harbormaster completed remote builds in B77523: Diff 188195.Wed, Sep 30, 12:58 PM

hayzam_gmail.com marked an inline comment as done.Wed, Sep 30, 12:59 PM

hayzam_gmail.com added inline comments.

usr.sbin/bhyve/virtio.c
196	Agreed. Invalid PFN writes now log an error and leave the queue unchanged. I’ve also removed zero-PFN queue-disable handling; that belongs in a separate patch.