kernel/vtpm: fix potential UB when accessing guest buffer

[stefano-garzarella]

Creating a mutable reference to the guest buffer, could have been an UB because the guest could potentially modify it.

So, let's copy the guest buffer to an internal one and then copy it back to give the result to the guest. To avoid overflowing the stack, allocate the buffer with Vec.

[stefano-garzarella]

This is a followup of #574 with a reference to the discussion we had here: #574 (comment) (thanks @00xc and @cclaudio)

I'm reporting the last comment from Carlos, to restart the discussion:

00xc Jan 16, 2025 •

I'd say so, yes (although someone can correct me on the need for a TLB flush on both operations). We would also need to revert the RMP change on the error path, and that in turn might require holding PVALIDATE_LOCK (although I'm foggy on the attack vector that lock prevented, @joergroedel remembers perhaps). Anyhow, this can be handled in the review for the separate PR.

I handled the error path, moving the error handling after the section, not sure what to do with PVALIDATE_LOCK.
@joergroedel any suggestion here?

[msft-jlange]

I don't think this is the sort of pattern we want to develop for interaction with guest memory. This change will revoke all guest access to the page that is being used by the TPM, and depending on how the guest is built (where it may be reading data, polling for memory changes, etc.), the guest may encounter an unexpected and fatal memory exception as a result of the permission change.

This pattern of modifying guest memory from the SVSM will be increasingly common as the SVSM begins to implement paravisor functionality. A much better pattern will be for the SVSM to be written defensively, using atomics or other Sync-safe accesses that assume the possibility for concurrent access - this is preferable to just assuming mutable access to the data and hoping for the best. If that's difficult, then consider copying the guest memory into a local SVSM buffer that can be accessed mutably for the operation, where the contents can be copied back to guest memory after the operation completes.

[stefano-garzarella]

I don't think this is the sort of pattern we want to develop for interaction with guest memory. This change will revoke all guest access to the page that is being used by the TPM, and depending on how the guest is built (where it may be reading data, polling for memory changes, etc.), the guest may encounter an unexpected and fatal memory exception as a result of the permission change.

@msft-jlange yeah, I see your point.

This pattern of modifying guest memory from the SVSM will be increasingly common as the SVSM begins to implement paravisor functionality. A much better pattern will be for the SVSM to be written defensively, using atomics or other Sync-safe accesses that assume the possibility for concurrent access - this is preferable to just assuming mutable access to the data and hoping for the best. If that's difficult, then consider copying the guest memory into a local SVSM buffer that can be accessed mutably for the operation, where the contents can be copied back to guest memory after the operation completes.

Maybe we should use something like https://github.com/rust-vmm/vm-memory/blob/main/src/volatile_memory.rs and when we need to share a buffer with C code, we will need to allocate a tmp one.

[stefano-garzarella]

If that's difficult, then consider copying the guest memory into a local SVSM buffer that can be accessed mutably for the operation, where the contents can be copied back to guest memory after the operation completes.

@msft-jlange I tried that in this version, but now I have a problem with GuestPtr::read().
I just pushed the code, so you can take a look, but I have a double fault the first time OVMF sends a vTPM command:

[SVSM] vTPM buffer - start 2139054080 offset 2752 end 2139062272
[SVSM] ERROR: Panic on CPU[0]! COCONUT-SVSM Version: 7b5bcddf
[SVSM] ERROR: Info: panicked at kernel/src/cpu/idt/svsm.rs:149:9:
Double-Fault at RIP 0xffffff8000208658 RSP: 0xfffffe0280003eb0 CR2: 0xfffffe0280003ea8
[SVSM] ---BACKTRACE---:
[SVSM] ERROR: Panic on CPU[0]! COCONUT-SVSM Version: 7b5bcddf
[SVSM] ERROR: Info: panicked at kernel/src/address.rs:443:21:
attempt to subtract with overflow
[SVSM] ---BACKTRACE---:
[SVSM]   [0xffffff8000234ba0] #
[SVSM]   [0xffffff800015e13e] #
[SVSM]   [0xffffff80001c54e6] #
[SVSM]   [0xffffff80001c5b2d] #
[SVSM]   [0xffffff80001c5e69] #
[SVSM]   [0xffffff80001c2494] #
[SVSM]   [0xffffff80001c36ab] #
[SVSM]   [0xffffff80001c6111] #
[SVSM]   [0xffffff8000129bd6] #
[SVSM]   [0xffffff80002317ab] #

Note: in the first line, I printed the paddr and the offset, so the buffer is crossing 2 pages. I thought it was the problem, but removing the vTPM driver in OVMF, and booting Linux, where the driver allocates the buffer page aligned, I have the same behavior, of course when Linux tries to access the vTPM :

[SVSM] vTPM buffer - start 4325560320 offset 0 end 4325564416
[SVSM] ERROR: Panic on CPU[0]! COCONUT-SVSM Version: 7b5bcddf
[SVSM] ERROR: Info: panicked at kernel/src/cpu/idt/svsm.rs:149:9:
Double-Fault at RIP 0xffffff8000208659 RSP: 0xfffffe0280003340 CR2: 0xfffffe0280003338
[SVSM] ---BACKTRACE---:
[SVSM] ERROR: Panic on CPU[0]! COCONUT-SVSM Version: 7b5bcddf
[SVSM] ERROR: Info: panicked at kernel/src/address.rs:443:21:
attempt to subtract with overflow
[SVSM] ---BACKTRACE---:
[SVSM]   [0xffffff8000234bc0] #
[SVSM]   [0xffffff800015e13e] #
[SVSM]   [0xffffff80001c54e6] #
[SVSM]   [0xffffff80001c5b2d] #
[SVSM]   [0xffffff80001c5e69] #
[SVSM]   [0xffffff80001c2494] #
[SVSM]   [0xffffff80001c36ab] #
[SVSM]   [0xffffff80001c6111] #
[SVSM]   [0xffffff8000129bd6] #
[SVSM]   [0xffffff80002317cb] #

So I tried to reduce the size of the buffer, for example if I do something like this I can't see the double fault if the buffer is page aligned or not (from PAGE_SIZE - 900 it double faults) :

            let buf_ptr = GuestPtr::<[u8; PAGE_SIZE - 1000]>::new(vaddr);

Any suggestions on where I'm doing it wrong?

[stefano-garzarella]

Any suggestions on where I'm doing it wrong?

I can answer my-self.... I did a stack overflow! Increasing the stack size, it works well.

So, should I avoid that, right?
Any suggestions on alternatives?

[stefano-garzarella]

Any suggestions on where I'm doing it wrong?

I can answer my-self.... I did a stack overflow! Increasing the stack size, it works well.

So, should I avoid that, right? Any suggestions on alternatives?

Maybe we can add GuestPtr::read_ref() and I can do something like this:

let mut buffer: Vec<u8> = vec![0; PAGE_SIZE];
let buf_slice: &mut [u8; PAGE_SIZE] = buffer.as_mut_slice().try_into().expect("Failed to initialize vTPM buffer");

let buf_ptr = GuestPtr::<[u8; PAGE_SIZE]>::new(vaddr);
unsafe { buf_ptr.read_ref(buf_slice)? };

let response_size = tpm_send_command_request(buf_slice)?;

unsafe { buf_ptr.write_ref(buf_slice)? };

[stefano-garzarella]

v2:

• added GuestPtr::read_ref
• copied vTPM buffer from the guest to an internal one and copied it back to return results

@msft-jlange @cclaudio @00xc please take a look.

[msft-jlange]

It would be nice to avoid having to allocate an entire page for every request. Is there some header in the payload that indicates how big the request is? If so, you could read that header into a stack local (which should be small), allocate your buffer based on how big the header indicates the data to be, then copy the stack local into the buffer and use read_ref to read the remainder of the command block into the buffer.

[stefano-garzarella]

Looking in the SVSM specification, it actually does not say anywhere that the buffer must be PAGE_SIZE, so I think you are right. Although the Linux driver for now always passes a large PAGE_SIZE buffer since it's used also for the response, so in practice it should be the same, but I see your point, I'll try it!

[msft-jlange]

You should consider using let mut uninit_buffer: Box<MaybeUninit<[u8]>> = Box::new_uninit_slice(PAGE_SIZE) and then you can avoid having to zero-initialize the contents that you are about to write over (and you can also avoid the intermediate Vec representation). This would require GuestPtr::read_ref() to become GuestPtr::read_ptr() (because creating a reference to uninitialized data is UB but creating a pointer to it is not), and after the copy is done, you could use let mut buffer: &[u8] = unsafe { uninit_buffer.assume_init() } to make your buffer safely usable once the copy initialization is complete.

[stefano-garzarella]

Cool, thanks for this, I like it, I'll try!

That said, I just discovered that during the path from this code to the TCG TPM C code, we allocate/copy the buffer several times:

• in TpmSendCommandRequest::send()

  svsm/kernel/src/protocols/vtpm.rs

  Line 116 in 035bb52

  let mut buffer: Vec<u8> = Vec::with_capacity(SEND_COMMAND_RESP_OUTBUF_SIZE);

• in TcgTpm::send_tpm_command()

  svsm/kernel/src/vtpm/tcgtpm/mod.rs

  Line 97 in 035bb52

  let mut response_ffi = Vec::<u8>::with_capacity(TPM_BUFFER_MAX_SIZE);

I think the second case maybe we cannot avoid, since the C code seems to expect 2 different buffers for input and output, but maybe, since we have to copy them from/to the guest anyway, we could pass 2 buffers from the beginning and avoid these allocations/copies.

I'll try to see if we can refactor a bit.

[msft-jlange]

@msft-jlange @cclaudio @00xc please take a look.

Much better now! I left a couple of comments that could tighten up the use of the intermediate buffer, but from a safety perspective, what you have now is perfectly acceptable.