unsound code patterns

[Freax13]

This codebase unfortunately uses code patterns that are unsound. A few examples (not exhaustive):

1. Improper synchronization:
   

   svsm/src/sev/ghcb.rs

   Lines 490 to 508 in 739fa7f

   	pub struct GHCBIOPort<'a> {
   	pub ghcb: RefCell<&'a mut GHCB>,
   	}
   	impl<'a> GHCBIOPort<'a> {
   	pub fn new(ghcb: RefCell<&'a mut GHCB>) -> Self {
   	GHCBIOPort { ghcb }
   	}
   	}
   	unsafe impl<'a> Sync for GHCBIOPort<'a> {}
   	impl<'a> IOPort for GHCBIOPort<'a> {
   	fn outb(&self, port: u16, value: u8) {
   	let mut g = self.ghcb.borrow_mut();
   	let ret = g.ioio_out(port, GHCBIOSize::Size8, value as u64);
   	if ret.is_err() {
   	request_termination_msr();
   	}
   	}

   
   GHCBIOPort should not implement Sync as it is not safe to share the contained RefCell across threads.
2. Unsafe use of raw pointers:
   

   svsm/src/console.rs

   Lines 13 to 38 in 739fa7f

   	#[derive(Debug)]
   	pub struct Console {
   	writer: *const dyn Terminal,
   	}
   	impl Console {
   	pub fn set(&mut self, w: *mut dyn Terminal) {
   	self.writer = w;
   	}
   	}
   	impl fmt::Write for Console {
   	fn write_str(&mut self, s: &str) -> fmt::Result {
   	if self.writer.is_null() {
   	return Ok(());
   	}
   	for ch in s.bytes() {
   	unsafe {
   	(*self.writer).put_byte(ch);
   	}
   	}
   	Ok(())
   	}
   	}

   
   Console::write_str creates a reference to the point set in Console::set without making sure that the pointer is dereferencable.
3. Missing lifetimes:
   

   svsm/src/mm/pagetable.rs

   Lines 710 to 747 in 739fa7f

   	#[derive(Debug)]
   	pub struct PageTableRef {
   	pgtable_ptr: *mut PageTable,
   	}
   	impl PageTableRef {
   	pub fn new(pgtable: &mut PageTable) -> PageTableRef {
   	PageTableRef {
   	pgtable_ptr: pgtable as *mut PageTable,
   	}
   	}
   	pub const fn unset() -> PageTableRef {
   	PageTableRef {
   	pgtable_ptr: ptr::null_mut(),
   	}
   	}
   	fn is_set(&self) -> bool {
   	!self.pgtable_ptr.is_null()
   	}
   	}
   	impl Deref for PageTableRef {
   	type Target = PageTable;
   	fn deref(&self) -> &Self::Target {
   	assert!(self.is_set());
   	unsafe { &*self.pgtable_ptr }
   	}
   	}
   	impl DerefMut for PageTableRef {
   	fn deref_mut(&mut self) -> &mut Self::Target {
   	assert!(self.is_set());
   	unsafe { &mut *self.pgtable_ptr }
   	}
   	}

   
   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.

[joergroedel]

You are right, there are several C-isms still in the code from the early days of the SVSM, when I was still starting my Rust learning journey 😊

So for the whole console code the solution will be to simplify it once the VC handler is merged. Then we can remove all the logic that makes it need a link to the GHCB.

The page-table code needs a general cleanup, that will happen along with the virtual memory manager changes and conversion work. For now it is safe because all page-tables are static, but with modules/processes this will change.

If you see any other place that could be improved I am open for PRs or discussussions in this issue. There are several areas for cleanup I have in mind, too. One is, e.g., changing stage2 to not need the PerCpu implementation, which allows further simplifications in PerCpu code. But there are certainly more places.

[Freax13]

If you see any other place that could be improved I am open for PRs or discussussions in this issue.

I might be able to take a shot at some of the issues over the weekend.

There are several areas for cleanup I have in mind, too. One is, e.g., changing stage2 to not need the PerCpu implementation, which allows further simplifications in PerCpu code. But there are certainly more places.

In that case it's probably a good idea to open issues to track these future tasks :)

[Freax13]

Continued list of issues:

• PageRef::new should be unsafe. The AsMut<[u8; PAGE_SIZE]> impl is dubious given that PageRef implements Clone
• 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.
• zero_mem_region should be unsafe.
• VmsaRef allows turning an arbitrary address into a mutable reference.
• Some of the methods of GuestPtr should be unsafe.
• write_u8 should be unsafe.
• VMSA::from_virt_addr should be unsafe.
• copy_secrets_page should be unsafe.
• 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.

Other than these safety violations, there is a lot of unsafe code in general and a lot of it is not very contained. Some of that's unavoidable given the nature of low level kernel code, but my guess is that this can be improved.
Ideally unsafe code should be used in self-contained abstractions that can then be used from safe code (i.e. Vec<T> hides all the gory unsafe details and exposes a mostly safe API). Right now there is a lot of code calling unsafe functions (or functions that should be unsafe) in other modules far away. This makes it more difficult to verify and test.

[00xc]

Continued list of issues:

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

Yeah this is definitely wrong. Opened #117 for this.

[00xc]

Continued list of issues:
...

• Some of the methods of GuestPtr should be unsafe.
• write_u8 should be unsafe.

These are fine in my opinion, as invalid accesses are properly recovered from via the exception table. I sort of mentioned this in the commit message for 0955d4e ("mm/guestmem: remove unsafe from {read,write}_u8()").

[Freax13]

Continued list of issues:
...

• Some of the methods of GuestPtr should be unsafe.
• write_u8 should be unsafe.

These are fine in my opinion, as invalid accesses are properly recovered from via the exception table. I sort of mentioned this in the commit message for 0955d4e ("mm/guestmem: remove unsafe from {read,write}_u8()").

Exceptions are not the only concern though, there are plenty of ways to cause havoc by writing to writable pages. For example these functions could be used to bypass the borrow checker or corrupt return pointers on the stack.

[joergroedel]

I suggest opening a new issue with the remaining points and keep track of the to-be-fixed places in a checklist in the first comment of the issue. This way we can track more easily what remains to be fixed.

[Freax13]

I suggest opening a new issue with the remaining points and keep track of the to-be-fixed places in a checklist in the first comment of the issue. This way we can track more easily what remains to be fixed.

Sounds good to me. Chances are that the issues listed here are out of date anyway.

Currently, I don't have the time to do this but feel free to do so. If everything goes well, I'll have more time to contribute in about two months.

[p4zuu]

I suggest opening a new issue with the remaining points and keep track of the to-be-fixed places in a checklist in the first comment of the issue. This way we can track more easily what remains to be fixed.

I have some time to spend atm, I'll take this and investigate the possible still-alive unsound issues. @Freax13 feel free to jump in when you have free time :)

[00xc]

Some updates:

• PerCpu::ghcb shouldn't give out mutable references with a static lifetime.

I'm working on having borrow checking for the GHCB at the moment.

• VmsaRef allows turning an arbitrary address into a mutable reference.

Will be fixed by #348.

• VMSA::from_virt_addr should be unsafe.

Outdated

[00xc]

I have some time to spend atm, I'll take this and investigate the possible still-alive unsound issues. @Freax13 feel free to jump in when you have free time :)

If you're up for it, could you assign yourself in the development document? You can add me as well, as I'm already working on a couple of the points above.

[p4zuu]

I have some time to spend atm, I'll take this and investigate the possible still-alive unsound issues. @Freax13 feel free to jump in when you have free time :)

If you're up for it, could you assign yourself in the development document? You can add me as well, as I'm already working on a couple of the points above.

I created #359 to track the remaining unsound patterns. The list still needs a bit of triage, we can discuss it in the issue. We could also maybe add a "Tracking" label to the new issue (or similar name) if we plan to append the new found patterns to the same list.
I also created #360 to set the owners to the three of us, and add the new tracking issue. I guess we can close this issue once the #360 is merged :)