fix(virtio-balloon): reject guest writes to device-owned config fields - #6148
fix(virtio-balloon): reject guest writes to device-owned config fields#6148leepokai wants to merge 1 commit into
Conversation
`write_config` copied any in-bounds guest bytes straight into the 12-byte balloon config space, so a guest could MMIO-write offset 0 or 8 and overwrite `num_pages` or `free_page_hint_cmd_id`. Both fields are device-owned: `num_pages` is set by the operator through `PATCH /balloon` and read back through `GET /balloon` (`amount_mib`) and `GET /balloon/statistics` (`target_pages`/`target_mib`), while `free_page_hint_cmd_id` is driven by the device. A guest could therefore forge the balloon target the orchestrator trusts. Per the virtio-balloon spec, `actual` is the only driver-writable field, so unlike virtio-block the config space cannot be made fully read-only. Restrict accepted writes to the byte range of `actual_pages`, and reject anything that touches the other fields or lands out of bounds: nothing is copied, the attempt is logged, and it is counted under the new balloon `cfg_fails` metric. This mirrors the handling already in place for virtio-block. Signed-off-by: libokai <kevin2005ha@gmail.com>
xhon-pelushi
left a comment
xhon-pelushi
left a comment
There was a problem hiding this comment.
Built this and checked the layout assumptions. The device change is correct and I think it should land. The test changes, though, introduce a reproducible flake under the default parallel test runner — 5 failures in 12 runs here.
The device change checks out
Layout. ConfigSpace is #[repr(C)] with three u32s in order (num_pages, actual_pages, free_page_hint_cmd_id), so offset_of!(ConfigSpace, actual_pages) is 4 and actual_end is 8 — matching the virtio-balloon config layout. Deriving the range from offset_of! plus SIZE_OF_U32 rather than writing 4/8 is the better choice; it stays correct if the fields are ever reordered.
Spec. actual is the only driver-writable field — the guest reports the current balloon size through it — while num_pages and free_page_hint_cmd_id are device-owned. So narrowing to exactly that window is right, and balloon really is the one device that has to accept some guest config writes rather than rejecting them wholesale.
The impact is worth stating explicitly, because the commit message undersells it: before this change, an in-bounds guest write to num_pages landed in the config space, and num_pages is what the operator reads back as size_mb through the management API. A guest could therefore misreport the balloon target to the host operator. That's the state confusion this closes, not just an out-of-bounds tidy-up.
Convention. The cfg_fails name and the warn-and-count shape match what block (device.rs:625), net (device.rs:1037), vsock (device.rs:346) and pmem (device.rs:558) already do, so nothing is invented here. Metrics wiring is complete — struct field, const fn new(), and the doc-comment JSON example. docs/metrics.md lists namespaces and links to BalloonDeviceMetrics rather than enumerating individual fields, so no docs change is needed.
Tests pass, single-threaded:
master -- --test-threads=1 : 28 passed, 0 failed
PR head -- --test-threads=1 : 29 passed, 0 failed (+1, the new test)
The problem: the new metric assertions race
test_virtio_write_config had no metric assertions on main; this PR adds four check_metric_after_block!(METRICS.cfg_fails, …) to it, and the new test_write_config_rejects_read_only_fields adds three more. METRICS is a process-global singleton, so two tests asserting exact deltas on the same counter interfere.
Running only those two tests, in parallel, twelve times:
run 1: FAILED run 2: FAILED run 4: FAILED run 5: FAILED run 7: FAILED
failures: 5 of 12
thread 'devices::virtio::balloon::device::tests::test_virtio_write_config' panicked at
src/vmm/src/devices/virtio/balloon/device.rs:1235:9:
assertion `left == right` failed: unexpected metric value
left: 2
right: 0
Line 1235 is this PR's own check_metric_after_block!(METRICS.cfg_fails, 0, …) — the "this write must not be counted as a failure" assertion. It sees 1 or 2 because the sibling test bumped the same global in between. cargo test parallelises by default, so this will flake in CI, not just locally.
The cause is structural rather than a mistake in the tests: balloon and vsock use the global METRICS, whereas block, net and pmem use a per-device self.metrics, which is why none of them hit this. serial_test isn't a dependency, so #[serial] isn't available as a quick fix.
Three ways out, in increasing order of scope:
- Merge the two tests so the global counter is only ever observed from one test. Cheapest, and keeps all the coverage.
- Drop the exact-delta assertions and assert the config space instead. The behaviour under test is "nothing was copied", which the existing
assert_eq!(actual_config_space, expected_config_space)already proves without touching a global. Theexpect 0case in particular buys little. - Move balloon to per-device metrics, matching block/net/pmem. Correct long-term, clearly a separate PR.
For what it's worth this isn't a new problem class — the pre-existing test_hinting_* tests already fail under the default runner for the same reason (main: 5 failures; this branch: 3; all pass with --test-threads=1). But since this PR adds new global-metric assertions, it's worth not adding to the pile.
One deliberate choice worth surfacing
Straddling writes are now rejected all-or-nothing: a write covering num_pages and actual together updates neither, where previously it updated both. A conformant driver never does this — Linux's virtio_balloon writes 4 bytes at offset 4 via virtio_cwrite on actual, and byte- or halfword-granular MMIO writes within [4,8) are still accepted — so I think reject-all is the right call. Just noting it's a choice rather than forced, in case anyone later reports a driver that writes the config space in one shot.
Very minor: write_config(8, &[]) satisfies start >= actual_start && end <= actual_end and is accepted as a no-op, while write_config(0, &[]) is counted in cfg_fails and logged. Zero-length config writes shouldn't reach here, so this is cosmetic.
What I did not test
cargo test -p vmm --lib devices::virtio::balloon only — no integration or VM-level tests, so no real guest driver exercised this path, and I haven't confirmed the behaviour against a live Linux virtio_balloon. x86-64 Linux only; nothing checked on aarch64.
|
The unit tests failing in parallel is a known issue with FC, not something introduced with this PR, so not an issue for me. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6148 +/- ##
=======================================
Coverage 82.98% 82.98%
=======================================
Files 277 277
Lines 30885 30887 +2
=======================================
+ Hits 25630 25632 +2
Misses 5255 5255
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Agreed on the pre-existing ones — the The race I was pointing at is a different one, and I think it is new here. Both tests involved are introduced or newly instrumented by this PR:
Running only those two, in parallel, twelve times gave 5 failures, all at this PR's own So it's the same underlying weakness — balloon using a process-global Entirely your call whether that matters. If you're happy carrying it alongside the existing ones, that's a reasonable position and the device change itself is good. I just wanted the distinction on record, since "known FC flakiness" and "two new assertions racing on one counter" have different fixes — the latter goes away by merging the two tests or dropping the |
3 participants
Changes
VirtioDevice::write_configfor the balloon device now accepts a guest writeonly when the whole byte range falls inside the
actual_pagesfield of theconfig space. Any write that touches
num_pagesorfree_page_hint_cmd_id, orthat lands out of bounds, is rejected: nothing is copied, a warning is logged,
and the attempt is counted under a new balloon
cfg_failsmetric.The accepted range is derived from
offset_of!(ConfigSpace, actual_pages)andthe field size rather than hardcoded, so it stays correct if the struct layout
changes. The existing checked-arithmetic bounds handling is preserved.
Reason
Before this change,
write_configcopied any in-bounds guest bytes straightinto the 12-byte balloon config space:
ConfigSpaceis:Per the virtio-balloon spec,
actualis the only driver-writable field — theguest reports the current balloon size through it.
num_pagesandfree_page_hint_cmd_idare device-owned:num_pagesis set by the operator viaPATCH /balloon(update_size()), andis read back by the operator through
GET /balloon(amount_mib, viasize_mb()) andGET /balloon/statistics(target_pages/target_mib, vialatest_stats()).free_page_hint_cmd_idis driven by the device (update_free_page_hint_cmd,start_hinting,stop_hinting).Because the old code accepted any in-bounds offset, a guest could MMIO-write
offset 0 (or 8) and overwrite those fields. The practical impact is a
control-plane integrity issue: the guest can forge the balloon target that the
orchestrator subsequently reads back from the management API, making
GET /balloonandGET /balloon/statisticsreport a size the operator neverset. There is no memory-safety impact — the bounds check was and remains in
place, so the guest cannot write outside the 12-byte config space.
This mirrors the hardening already applied to virtio-block in commit 80fb45d
("fix(virtio-block): make device config space read-only for the guest"), which
made
VirtioBlock::write_configreject all guest writes and count them under acfg_failsmetric. Balloon cannot be made fully read-only, becauseactualislegitimately driver-written, so the restriction here is field-scoped instead of
total.
Testing
Unit tests updated/added in
src/vmm/src/devices/virtio/balloon/device.rs:test_write_config_rejects_read_only_fields(new): asserts that guest writesto
num_pages(offset 0) andfree_page_hint_cmd_id(offset 8) leave thosefields — and the operator-facing
size_mb()— unchanged while incrementingcfg_fails, and that a write toactual(offset 4) is still accepted withoutincrementing
cfg_fails.test_virtio_write_config: reworked for the new contract — theactualwriteis accepted; writes at offsets 0, 6 and 8, the out-of-bounds write and the
u64::MAXoffset write are all rejected and counted.test_free_page_hinting_configandtest_num_pages: previously setnum_pagesthroughwrite_config; they now set it through the device API(
update_num_pages), andtest_num_pagesadditionally asserts that a guestwrite to
num_pagesis rejected.Commands run on
x86_64(Ubuntu 24.04, toolchain 1.97.0 as pinned byrust-toolchain.toml):cargo test -p vmm --lib devices::virtio::ballooncargo test -p vmm --lib metricscargo clippy -p vmm --all-targets -- -D warningscargo clippy --all --all-targets -- -D warningscargo fmt -- --checkcargo build -p vmmThe new regression test was also confirmed to fail against the unfixed
write_config(assertion left == right failed: unexpected metric value, left: 0, right: 1) and to pass with the fix applied, together withtest_virtio_write_configandtest_num_pages.Not run: the integration test suite (
tools/devtool test) and any KVM-dependenttest — the development environment has no
/dev/kvmand no Docker daemon. Thechange is pure device config-space logic and is fully covered by the unit tests
above. Only
x86_64was built and tested; the change is architectureindependent.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.
PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.