Page MenuHomeFreeBSD

vmm: fix use after free in ppt_detach()
ClosedPublic

Authored by rew on Jan 15 2023, 10:40 PM.
Tags
None
Referenced Files
F102816633: D38072.diff
Sun, Nov 17, 1:49 PM
Unknown Object (File)
Tue, Oct 22, 8:11 AM
Unknown Object (File)
Sep 18 2024, 10:56 PM
Unknown Object (File)
Sep 18 2024, 7:31 PM
Unknown Object (File)
Sep 18 2024, 6:48 AM
Unknown Object (File)
Sep 17 2024, 1:32 AM
Unknown Object (File)
Sep 15 2024, 8:35 PM
Unknown Object (File)
Sep 7 2024, 2:27 PM

Details

Summary

The following panic after kldunload`ing vmm:

Fatal trap 12: page fault while in kernel mode
cpuid = 6; apic id = 06
fault virtual address   = 0xffff80403037b7a8
fault code              = supervisor read data, page not present
instruction pointer     = 0x20:0xffffffff810b67d4
stack pointer           = 0x28:0xfffffe0111748b80
frame pointer           = 0x28:0xfffffe0111748b90
code segment            = base 0x0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags        = interrupt enabled, resume, IOPL = 0
current process         = 1904 (kldunload)
rdi: deadc0dedeadc0de rsi:             3c00 rdx:         3037b7a8
rcx: ffff804000000000  r8: ffffffff813f45a0  r9:                2
rax:    ffffffffff000 rbx: deadc0dedeadc0de rbp: fffffe0111748b90
r10:                1 r11:            10000 r12: fffff80001c83900
r13:               3c r14: fffff80000000000 r15: fffff80001c83800
trap number             = 12
panic: page fault
cpuid = 6
time = 1673375171
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe0111748940
vpanic() at vpanic+0x151/frame 0xfffffe0111748990
panic() at panic+0x43/frame 0xfffffe01117489f0
trap_fatal() at trap_fatal+0x409/frame 0xfffffe0111748a50
trap_pfault() at trap_pfault+0xab/frame 0xfffffe0111748ab0
calltrap() at calltrap+0x8/frame 0xfffffe0111748ab0
--- trap 0xc, rip = 0xffffffff810b67d4, rsp = 0xfffffe0111748b80, rbp = 0xfffffe0111748b90 ---
pmap_kextract() at pmap_kextract+0x164/frame 0xfffffe0111748b90
vtd_add_device() at vtd_add_device+0x25/frame 0xfffffe0111748c00
ppt_detach() at ppt_detach+0x10e/frame 0xfffffe0111748c40
device_detach() at device_detach+0x195/frame 0xfffffe0111748c80
devclass_driver_deleted() at devclass_driver_deleted+0x67/frame 0xfffffe0111748cc0
devclass_delete_driver() at devclass_delete_driver+0x8f/frame 0xfffffe0111748d00
driver_module_handler() at driver_module_handler+0xdc/frame 0xfffffe0111748d50
module_unload() at module_unload+0x44/frame 0xfffffe0111748d70
linker_file_unload() at linker_file_unload+0x1eb/frame 0xfffffe0111748db0
kern_kldunload() at kern_kldunload+0x18d/frame 0xfffffe0111748e00
amd64_syscall() at amd64_syscall+0x12e/frame 0xfffffe0111748f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe0111748f30
--- syscall (444, FreeBSD ELF64, kldunloadf), rip = 0x3b90e51a7b0a, rsp = 0x3b90e334cf08, rbp = 0x3b90e334d760 ---
KDB: enter: panic

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 49129
Build 46018: arc lint + arc unit

Event Timeline

rew requested review of this revision.Jan 15 2023, 10:40 PM

previous diff was incorrect

Does the bug occur because the ppt module is unloaded after the vmm module when vmm.ko is unloaded?

sys/amd64/vmm/intel/vtd.c
763

Why is this needed?

Does the bug occur because the ppt module is unloaded after the vmm module when vmm.ko is unloaded?

Yes, and the vmm module is what cleans up the iommu.

sys/amd64/vmm/intel/vtd.c
763

It's not needed I suppose

I originally had it in for testing..then left it in because thought it'd be reasonable to zero out the memory left behind. But, since it's a mapping to a device, there's probably nothing left behind that needs to be cleared out?

With the bzero() removed or my comment otherwise addressed somehow, this seems ok.

sys/amd64/vmm/intel/vtd.c
763

Yeah, I don't think there's anything sensitive in there that's worth zeroing out, so I'd omit this. Without a comment explaining why the bzero() call is there and not in amdvi_destroy_domain(), it looks a bit strange.

This revision is now accepted and ready to land.Jan 18 2023, 4:08 PM
jhb added a subscriber: jhb.
jhb added inline comments.
sys/amd64/vmm/intel/vtd.c
763

Plus, if you really want to zero before free, it's best practice now to use zfree instead of free. I agree with Mark that you don't need the bzero at all though.

This revision was automatically updated to reflect the committed changes.