Is `Tcg::submit_command` be unsafe when the output slice is smaller than expected?

[ChocolateLoverRaj]

What happens if you submit a command that could have an output of up to 20 bytes, but you give an output slice of only 10 bytes?

I just tried calling TPM2_GetRandom and it was having weird behavior when I had an output slice that was too small, so I'm wondering what happens.

@nicholasbishop

[phip1611]

Hey there, thanks for the issue!

In our code, you can find:

    pub fn submit_command(
        &mut self,
        input_parameter_block: &[u8],
        output_parameter_block: &mut [u8],
    ) -> Result {
        let input_parameter_block_len = u32::try_from(input_parameter_block.len())
            .map_err(|_| Error::from(Status::BAD_BUFFER_SIZE))?;
        let output_parameter_block_len = u32::try_from(output_parameter_block.len())
            .map_err(|_| Error::from(Status::BAD_BUFFER_SIZE))?;

        unsafe {
            (self.0.submit_command)(
                &mut self.0,
                input_parameter_block_len,
                input_parameter_block.as_ptr(),
                output_parameter_block_len,
                output_parameter_block.as_mut_ptr(),
            )
            .to_result()
        }
    }

so the underlying UEFI implementation should see the buffer sizes and handle things properly. I'd be surprised if UEFI writes to the buffer anyway - but there also might be a bug in some UEFI implementations?! Could you provide more context please?

[ChocolateLoverRaj]

When I called TPM2_GetRandom and the output slice was not big enough, the random bytes were all zeroes for some reason. I thought this was weird. This is in QEMU.

[phip1611]

Do you have a reproducer?

This is in QEMU.

You probably mean you run the OVMF UEFI firmware implementation in QEMU and there, with the QEMU TPM, you get this error?

[ChocolateLoverRaj]

The problem was caused by a bug in my code, so I don't think there is actually a bug with OVMF, QEMU, or swtpm. After seeing

input_parameter_block_len,
                input_parameter_block.as_ptr(),
                output_parameter_block_len,
                output_parameter_block.as_mut_ptr(),

it makes sense that this function shouldn't be marked as safe because the UEFI function will know about the len, which will always match the slice len.

Another thing I was wondering is if output_parameter_block should be &mut MaybeUninit<u8>.

[nicholasbishop]

I just tried calling TPM2_GetRandom and it was having weird behavior when I had an output slice that was too small, so I'm wondering what happens.

I would expect the function to return a BUFFER_TOO_SMALL error. If that's not the behavior you see, it might be a bug; let us know.

Another thing I was wondering is if output_parameter_block should be &mut MaybeUninit.

The only reason (that I'm aware of) for using &mut [MaybeUninit<u8>] instead of &mut [u8] would be performance. It allows the caller to avoid the cost of initializing the output buffer. However, it comes at the cost of worse ergonomics for the caller. So resolving that tradeoff requires looking at whether the buffer is likely to be large. In this case, I think the answer is no; it's unlikely that the output buffer is going to be large enough that avoiding the cost of initialization matters. If you have a counterexample though, let us know.

[phip1611]

Since the problem was in your code, I'm closing this. Feel free to open new issues if you want to discuss more things!