fix(mmio): don't return MMIO virtual addresses to the free list (use-after-free)#2255
fix(mmio): don't return MMIO virtual addresses to the free list (use-after-free)#2255edef1c wants to merge 1 commit intohermit-os:mainfrom
Conversation
5aa7ecd to
253b948
Compare
There was a problem hiding this comment.
Benchmark Results
Details
| Benchmark | Current: cb881f1 | Previous: 4912505 | Performance Ratio |
|---|---|---|---|
| startup_benchmark Build Time | 99.27 s |
99.11 s |
1.00 ❗ |
| startup_benchmark File Size | 0.86 MB |
0.86 MB |
1.00 ❗ |
| Startup Time - 1 core | 0.97 s (±0.02 s) |
0.96 s (±0.03 s) |
1.01 |
| Startup Time - 2 cores | 0.97 s (±0.03 s) |
0.96 s (±0.03 s) |
1.00 |
| Startup Time - 4 cores | 0.97 s (±0.03 s) |
0.96 s (±0.02 s) |
1.01 |
| multithreaded_benchmark Build Time | 101.80 s |
102.01 s |
1.00 ❗ |
| multithreaded_benchmark File Size | 0.95 MB |
0.95 MB |
1.00 ❗ |
| Multithreaded Pi Efficiency - 2 Threads | 91.12 % (±6.64 %) |
85.65 % (±8.90 %) |
1.06 |
| Multithreaded Pi Efficiency - 4 Threads | 45.02 % (±2.75 %) |
43.12 % (±3.13 %) |
1.04 |
| Multithreaded Pi Efficiency - 8 Threads | 26.44 % (±1.80 %) |
25.03 % (±1.95 %) |
1.06 |
| micro_benchmarks Build Time | 112.93 s |
114.00 s |
0.99 ❗ |
| micro_benchmarks File Size | 0.96 MB |
0.96 MB |
1.00 ❗ |
| Scheduling time - 1 thread | 69.86 ticks (±3.84 ticks) |
70.19 ticks (±3.58 ticks) |
1.00 |
| Scheduling time - 2 threads | 38.47 ticks (±4.79 ticks) |
38.99 ticks (±4.43 ticks) |
0.99 |
| Micro - Time for syscall (getpid) | 2.94 ticks (±0.26 ticks) |
2.96 ticks (±0.24 ticks) |
0.99 |
| Memcpy speed - (built_in) block size 4096 | 63522.17 MByte/s (±45339.86 MByte/s) |
64026.03 MByte/s (±45553.92 MByte/s) |
0.99 |
| Memcpy speed - (built_in) block size 1048576 | 29963.60 MByte/s (±24956.52 MByte/s) |
29743.12 MByte/s (±24826.10 MByte/s) |
1.01 |
| Memcpy speed - (built_in) block size 16777216 | 25666.85 MByte/s (±21512.30 MByte/s) |
24007.89 MByte/s (±20230.77 MByte/s) |
1.07 |
| Memset speed - (built_in) block size 4096 | 63985.25 MByte/s (±45622.98 MByte/s) |
64637.10 MByte/s (±45951.78 MByte/s) |
0.99 |
| Memset speed - (built_in) block size 1048576 | 30754.88 MByte/s (±25394.92 MByte/s) |
30516.60 MByte/s (±25260.76 MByte/s) |
1.01 |
| Memset speed - (built_in) block size 16777216 | 26362.96 MByte/s (±21919.69 MByte/s) |
24779.16 MByte/s (±20739.68 MByte/s) |
1.06 |
| Memcpy speed - (rust) block size 4096 | 58330.20 MByte/s (±43048.88 MByte/s) |
60077.78 MByte/s (±44331.51 MByte/s) |
0.97 |
| Memcpy speed - (rust) block size 1048576 | 29827.70 MByte/s (±24862.50 MByte/s) |
29870.69 MByte/s (±24878.06 MByte/s) |
1.00 |
| Memcpy speed - (rust) block size 16777216 | 24954.47 MByte/s (±21049.51 MByte/s) |
24212.89 MByte/s (±20297.89 MByte/s) |
1.03 |
| Memset speed - (rust) block size 4096 | 59114.01 MByte/s (±43499.71 MByte/s) |
60925.02 MByte/s (±44834.41 MByte/s) |
0.97 |
| Memset speed - (rust) block size 1048576 | 30628.95 MByte/s (±25305.17 MByte/s) |
30651.78 MByte/s (±25309.43 MByte/s) |
1.00 |
| Memset speed - (rust) block size 16777216 | 25729.58 MByte/s (±21546.95 MByte/s) |
24986.00 MByte/s (±20809.17 MByte/s) |
1.03 |
| alloc_benchmarks Build Time | 104.48 s |
105.28 s |
0.99 ❗ |
| alloc_benchmarks File Size | 0.93 MB |
0.93 MB |
1.00 ❗ |
| Allocations - Allocation success | 100.00 % |
100.00 % |
1 |
| Allocations - Deallocation success | 100.00 % |
100.00 % |
1 |
| Allocations - Pre-fail Allocations | 100.00 % |
100.00 % |
1 |
| Allocations - Average Allocation time | 12659.28 Ticks (±165.88 Ticks) |
9727.01 Ticks (±267.20 Ticks) |
1.30 ❗ |
| Allocations - Average Allocation time (no fail) | 12659.28 Ticks (±165.88 Ticks) |
9727.01 Ticks (±267.20 Ticks) |
1.30 ❗ |
| Allocations - Average Deallocation time | 1065.63 Ticks (±771.58 Ticks) |
2219.93 Ticks (±959.95 Ticks) |
0.48 |
| mutex_benchmark Build Time | 103.58 s |
104.33 s |
0.99 ❗ |
| mutex_benchmark File Size | 0.96 MB |
0.96 MB |
1.00 ❗ |
| Mutex Stress Test Average Time per Iteration - 1 Threads | 12.92 ns (±0.66 ns) |
12.84 ns (±0.76 ns) |
1.01 |
| Mutex Stress Test Average Time per Iteration - 2 Threads | 15.66 ns (±0.76 ns) |
15.44 ns (±0.78 ns) |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
|
FWIW, it is not totally clear to me why PageBox merely reserves the VA, and leaks the mapping / doesn't unmap on Drop; the VA use-after-free would have been dead obvious in all cases with a more RAII-style design. |
check_linux_args MMIO device registers into a PageBox-allocated VA, then return a VolatileRef<'static> pointing at it. When the PageBox is dropped, the VA range goes back to the free list. A subsequent allocation could reuse and remap the same VA, corrupting the driver's VolatileRef. Use into_raw() to prevent the VA from being reclaimed when a device is found.
253b948 to
cb881f1
Compare
mkroening
left a comment
There was a problem hiding this comment.
Thanks, this looks good! :)
Can you share how to reproduce this? Are you using Firecracker or something else?
A similar bug exists in guess_device
Fixing that one would be good too. I can reproduce that as follows:
cargo xtask ci rs --arch x86_64 --package httpd --no-default-features --features ci,hermit/dhcpv4,hermit/tcp,hermit/virtio-net,hermit/virtio-console qemu --microvm --devices virtio-console-mmio --devices virtio-net-mmioSwapping the devices makes the difference.
FWIW, it is not totally clear to me why PageBox merely reserves the VA, and leaks the mapping / doesn't unmap on Drop; the VA use-after-free would have been dead obvious in all cases with a more RAII-style design.
That's a good point. I was planning to do that eventually, but I did not have time for that yet. I have opened #2338 for tracking.
Eventually, I will unify and clean up MMIO and PCI device detection across architectures, which should help with these legacy code issues in the future.
This turned out to be caused by another issue and was fixed in 0d156de. |
In check_linux_args, we return VAs to the free list while a VolatileRef to them is active. This breaks multi-device configurations. A similar bug exists in guess_device, but the current version of this patch doesn't fix it, since my reference target supplies mmio args.