Make read/write methods from GuestPtr unsafe, and copy_{from, to} methods from SecretsPage as well

[p4zuu]

Leaving as draft because I still have doubts.

Providing partial fix for #359 (tasks 7, 8, 9).

My two interrogations are:

1. There are several uses of the following pattern:

    let guard = PerCPUPageMappingGuard::create_4k(paddr)?; // paddr possibly guest or HV controlled
    let start = guard.virt_addr();
    let guest_page = GuestPtr::<Tt>::new(start + offset);
    let mut request = unsafe { guest_page.read()? };

As already discussed with @00xc, if start + offset is valid but start + offset + size_of::<T> ends on an unmapped page or on an adjacent page belonging to another guest or the SVSM kernel (I don't really know if it's possible), read() could raise a #PF and possibly return an error and panicking, or reading content from an unauthorized page. If such scenarii are possible, we should either:

• check that start + offset + size_of::<T> doesn't cross the page end
• allow GuestPtr::read()/write() for partial read/write
• more general, we should always check that a guest or the HV can't force GuestPtr::read()/write() to return an error that will be treated as a fatal error by the SVSM kernel (that would be valid DoS).

2. In some core protocol handlers, a guest provides a physical address (example the pvalidate handler), if I understand the code correctly, the guest-provided physical address pointing to the request entries is only verified if it's mapped or not. My understanding is that a guest could make the kernel read at any mapped address to fetch the PValidateRequest (including the kernel memory). Moreover, the guest-provided entries suffer from the same issue, the paddr to pvalidate are only verified to be mapped. A guest could make the SVSM kernel pvalidate any page. For the core protocol handlers that work with guest-provided paddrs, my understanding is that we should validate that paddrs only belong to the guest's memory region. I left TODOs is some places (not all of them) to points where there is this issue.

[00xc]

Leaving as draft because I still have doubts.

Providing partial fix for #359 (tasks 7, 8, 9).

My two interrogations are:

1. There are several uses of the following pattern:

    let guard = PerCPUPageMappingGuard::create_4k(paddr)?; // paddr possibly guest or HV controlled
    let start = guard.virt_addr();
    let guest_page = GuestPtr::<Tt>::new(start + offset);
    let mut request = unsafe { guest_page.read()? };

As already discussed with @00xc, if start + offset is valid but start + offset + size_of::<T> ends on an unmapped page or on an adjacent page belonging to another guest or the SVSM kernel (I don't really know if it's possible), read() could raise a #PF and possibly return an error and panicking, or reading content from an unauthorized page. If such scenarii are possible, we should either:

• check that start + offset + size_of::<T> doesn't cross the page end
• allow GuestPtr::read()/write() for partial read/write
• more general, we should always check that a guest or the HV can't force GuestPtr::read()/write() to return an error that will be treated as a fatal error by the SVSM kernel (that would be valid DoS).

One case we cannot protect from (as we already discussed) is #VC handling of kernel code, as we have no way of resetting to a valid instruction pointer.

2. In some core protocol handlers, a guest provides a physical address (example the pvalidate handler), if I understand the code correctly, the guest-provided physical address pointing to the request entries is only verified if it's mapped or not. My understanding is that a guest could make the kernel read at any mapped address to fetch the PValidateRequest (including the kernel memory). Moreover, the guest-provided entries suffer from the same issue, the paddr to pvalidate are only verified to be mapped. A guest could make the SVSM kernel pvalidate any page. For the core protocol handlers that work with guest-provided paddrs, my understanding is that we should validate that paddrs only belong to the guest's memory region. I left TODOs is some places (not all of them) to points where there is this issue.

As far as I can tell we already guard against this via valid_phys_address(), so I think we're safe for now. Although perhaps we should enforce this at the type level.

[p4zuu]

As far as I can tell we already guard against this via valid_phys_address(), so I think we're safe for now. Although perhaps we should enforce this at the type level.

You're right, I misread init_memory_map(), I thought MEMORY_MAP contained SVSM kernel memory regions, but it actually contains guest regions, so we're good.

[p4zuu]

Several uses of GuestPtr::read()/write() are to update VMSA's calling area. If I understand correctly, VMSA can't be modified by a guest or by the hypervisor, so we can trust its location and its content. With this, I think we can implement 2 safe methods to read and write CAA (and possibly extrapolate to other VMSA fields), that would remove several unsafe that I currently added for this PR. Something like:

pub fn read_caa(va: VirtAddr) -> Result<SvsmCaa, SvsmError> {
    let calling_area = GuestPtr::<SvsmCaa>::new(virt_addr);
    // SAFETY: HV and guests can't change VMSA's content, therefore CAA.
    unsafe { calling_area.read() }
}

[00xc]

Several uses of GuestPtr::read()/write() are to update VMSA's calling area. If I understand correctly, VMSA can't be modified by a guest or by the hypervisor, so we can trust its location and its content.

No, I don't think that's how it works. First, the guest's VMSA is different from the SVSM's VMSA - the guest is allowed to update its VMSA. Second, the CA is located in a different physical page than the VMSA. When the guest sets the CA's physical address (via core_create_vcpu()) or updates it (via core_remap_ca()) we have checks in place to verify it is valid. That is why mapping it and reading/writing to it is safe.

[00xc]

Left some suggestions. On top of that, safety comments regarding the CAA should be updated based on my previous comment above.

[p4zuu]

No, I don't think that's how it works. First, the guest's VMSA is different from the SVSM's VMSA - the guest is allowed to update its VMSA. Second, the CA is located in a different physical page than the VMSA. When the guest sets the CA's physical address (via core_create_vcpu()) or updates it (via core_remap_ca()) we have checks in place to verify it is valid. That is why mapping it and reading/writing to it is safe.

That makes sense to me, thanks. I think there is a 3rd way to update the CA: through prepare_fw_launch() raised during early setup with IGVM-provided value, so I guess we can safely trust it too, correct?

[joergroedel]

    let guard = PerCPUPageMappingGuard::create_4k(paddr)?; // paddr possibly guest or HV controlled
    let start = guard.virt_addr();
    let guest_page = GuestPtr::<Tt>::new(start + offset);
    let mut request = unsafe { guest_page.read()? };

Longer term we should introduce a safe interface around this pattern, which does all the mapping and bounds checking internally and avoids the unsafe keyword everywhere in the code. Just not so sure about TLB flushing, maybe we need a collector type as a base which handles that. Something like this:

{
    // Create TLB handle
    tlb = tlb_gather::new();
    // Create memory handle which can not outlive TLB handle
    mem_handle = tlb.new_mapping(phys_addr, size);
    let mut foo = mem_handle.read();
    let mut bar = mem_handle::offset(1).read();
    drop(mem_handle);
    // mem_handle goes out of scope, unmapping the memory, TLB handle makes sure the virt range is not
    // re-used until it is destroyed

    mem_handle2 = tlb.new_mapping(paddr2, size2)
    // ... use mem_handle2

    // memhandle2 goes out of scope, mapping is removed
    // tlb goes out of scope and flushes the TLB(s).
}
   

[joergroedel]

But for now it looks good to me, will merge it once @00xc approves.

[00xc]

But for now it looks good to me, will merge it once @00xc approves.

We still have some unresolved conversations. Once that's done I'll approve.

[p4zuu]

Longer term we should introduce a safe interface around this pattern, which does all the mapping and bounds checking internally and avoids the unsafe keyword everywhere in the code.

I agree, this pattern happens in quite a few places and there is room for improvements.

Just not so sure about TLB flushing, maybe we need a collector type as a base which handles that.

Couldn't we just simply create new read()/write() methods for PerCPUPageMappingGuard?

[p4zuu]

That makes sense to me, thanks. I think there is a 3rd way to update the CA: through prepare_fw_launch() raised during early setup with IGVM-provided value, so I guess we can safely trust it too, correct?

Checked with Carlos that the CA address updated in prepare_fw_launch() is validated in validate_fw_memory().

[00xc]

Just not so sure about TLB flushing, maybe we need a collector type as a base which handles that.

Couldn't we just simply create new read()/write() methods for PerCPUPageMappingGuard?

It wouldn't solve the fact that we would need to issue a flush for each guard. It also requires making PerCPUPageMappingGuard type-aware. Not a bad idea, but it is a more intrusive change.

[00xc]

There are some conflicts to resolve, please rebase.

[p4zuu]

There are some conflicts to resolve, please rebase.

Done