Unsound code patterns

[p4zuu]

Here are some unsound code patterns possibly remaining after @Freax13's initial issue #104.

• PageTableRef doesn't have a lifetime associated with it, so there's no guarantee that the contained PageTable pointer is still live when PageTableRef::deref(_mut) dereferences the pointer. (Fixed by mm: always use this_cpu().get_pgtable() instead of get_init_pgtable_locked() #443)
• PageRef::new should be unsafe. The AsMut<[u8; PAGE_SIZE]> impl is dubious given that PageRef implements Clone (Fixed by mm: various improvements to PageRef #450)
• this_cpu_mut is unsound as it allows creating multiple mutable references pointing to the same location. (fixed by cpu/percpu: fix reentrant mutable references #372)
• PerCpu::ghcb shouldn't give out mutable references with a static lifetime. (fixed by cpu/percpu: fix reentrant mutable references #372)
• zero_mem_region should be unsafe.
• VmsaRef allows turning an arbitrary address into a mutable reference. (Fixed by cpu/percpu: store safe references to HV doorbell page and SVSM VMSA #387)
• Some of the methods of GuestPtr should be unsafe. (Fixed by Make read/write methods from GuestPtr unsafe, and copy_{from, to} methods from SecretsPage as well  #382)
• write_u8 should be unsafe. (Fixed by Make read/write methods from GuestPtr unsafe, and copy_{from, to} methods from SecretsPage as well  #382)
• copy_secrets_page should be unsafe. (Fixed by Make read/write methods from GuestPtr unsafe, and copy_{from, to} methods from SecretsPage as well  #382)
• ImmutAfterInitCell::reinit should be unsafe (or removed altogether).
• ImmutAfterInitCell::check_init should also check the init flag when debug assertions are disabled.
• load_idt either needs to take a 'static reference or be unsafe. The same applies to load_tss. (IDT and GDT fixed by cpu: fix IDT and GDT lifetimes on load methods #366, TSS suffers from the same issue and fix still missing)
• GuestVMExit does not list all VM exit types (and I don't think this would be feasible given that AMD can decide to add new exit types in the future). We use this type in VMSA. If a guest exits with an unsupported exit type, we have UB. (Fixed by cpuarch/vmsa: fix soundness issue with GuestVMExit #480)

[00xc]

• this_cpu_mut is unsound as it allows creating multiple mutable references pointing to the same location.
• PerCpu::ghcb shouldn't give out mutable references with a static lifetime. (@00xc currently working on it)

These will be fixed by #372.

• load_idt either needs to take a 'static reference or be unsafe. The same applies to load_tss.

The IDT part should be fixed by #366. TSS still not fixed yet.

[00xc]

• VmsaRef allows turning an arbitrary address into a mutable reference. (Will be fixed by Introduce safe page allocation abstractions #348)

This will actually be fixed by #387.

[Freax13]

• PageTableRef doesn't have a lifetime associated with it, so there's no guarantee that the contained PageTable pointer is still live when PageTableRef::deref(_mut) dereferences the pointer.

This will be fixed by #443 (though to be fair AFAICT the original issue doesn't exist anymore anyway).

[Freax13]

• PageRef::new should be unsafe. The AsMut<[u8; PAGE_SIZE]> impl is dubious given that PageRef implements Clone

This will be fixed by #450.

[Freax13]

GuestVMExit does not list all VM exit types (and I don't think this would be feasible given that AMD can decide to add new exit types in the future). We use this type in VMSA. If a guest exits with an unsupported exit type, we have UB.

[p4zuu]

GuestVMExit does not list all VM exit types (and I don't think this would be feasible given that AMD can decide to add new exit types in the future). We use this type in VMSA. If a guest exits with an unsupported exit type, we have UB.

Good catch. If I understand the code correctly, the UB can only be triggered in RequestParams::from_vmsa(), right? If we add a check here, it would still be possible to put an undefined value in VMSA::guest_exit_code somewhere else, but at least we could catch a guest-controlled UB

[Freax13]

GuestVMExit does not list all VM exit types (and I don't think this would be feasible given that AMD can decide to add new exit types in the future). We use this type in VMSA. If a guest exits with an unsupported exit type, we have UB.

Good catch. If I understand the code correctly, the UB can only be triggered in RequestParams::from_vmsa(), right?

It's UB even if we don't access the value.

If we add a check here, it would still be possible to put an undefined value in VMSA::guest_exit_code somewhere else, but at least we could catch a guest-controlled UB

Can you elaborate on how this check would be implemented?

IMHO we should change GuestVMExit to be defined as pub struct GuestVMExit(u64); and add associated constants for the variants e.g. impl GuestVMExit { pub const MC: Self = Self(0x52); }. This is a fairly common compromise between wanting to have properly named variants but also having the flexibility to allow unknown values.

[p4zuu]

IMHO we should change GuestVMExit to be defined as pub struct GuestVMExit(u64); and add associated constants for the variants e.g. impl GuestVMExit { pub const MC: Self = Self(0x52); }. This is a fairly common compromise between wanting to have properly named variants but also having the flexibility to allow unknown values.

Yes you're right. It looks easier to handle unknown values, and allow room for the new exit codes.

[p4zuu]

IMHO we should change GuestVMExit to be defined as pub struct GuestVMExit(u64); and add associated constants for the variants e.g. impl GuestVMExit { pub const MC: Self = Self(0x52); }. This is a fairly common compromise between wanting to have properly named variants but also having the flexibility to allow unknown values.

Do you plan working on implementing this? I can take it if you want

[Freax13]

IMHO we should change GuestVMExit to be defined as pub struct GuestVMExit(u64); and add associated constants for the variants e.g. impl GuestVMExit { pub const MC: Self = Self(0x52); }. This is a fairly common compromise between wanting to have properly named variants but also having the flexibility to allow unknown values.

Do you plan working on implementing this? I can take it if you want

Feel free, I'm already quite busy.