On Wed, Oct 27, 2021 at 3:13 PM Brijesh Singh [off-list ref] wrote:

On 10/27/21 4:05 PM, Peter Gonda wrote:
....

quoted

quoted

quoted

quoted

quoted

Thanks for updating this sequence number logic. But I still have some
concerns. In verify_and_dec_payload() we check the encryption header
but all these fields are accessible to the hypervisor, meaning it can
change the header and cause this sequence number to not get
incremented. We then will reuse the sequence number for the next
command, which isn't great for AES GCM. It seems very hard to tell if
the FW actually got our request and created a response there by
incrementing the sequence number by 2, or if the hypervisor is acting
in bad faith. It seems like to be safe we need to completely stop
using this vmpck if we cannot confirm the PSP has gotten our request
and created a response. Thoughts?

Very good point, I think we can detect this condition by rearranging the
checks. The verify_and_dec_payload() is called only after the command is
succesful and does the following checks

1) Verifies the header
2) Decrypts the payload
3) Later we increment the sequence

If we arrange to the below order then we can avoid this condition.
1) Decrypt the payload
2) Increment the sequence number
3) Verify the header

The descryption will succeed only if PSP constructed the payload.

Does this make sense ?

Either ordering seems fine to me. I don't think it changes much though
since the header (bytes 30-50 according to the spec) are included in
the authenticated data of the encryption. So any hypervisor modictions
will lead to a decryption failure right?

Either case if we do fail the decryption, what are your thoughts on
not allowing further use of that VMPCK?

We have limited number of VMPCK (total 3). I am not sure switching to
different will change much. HV can quickly exaust it. Once we have SVSM
in-place then its possible that SVSM may use of the VMPCK. If the
decryption failed, then maybe its safe to erase the key from the secrets
page (in other words guest OS cannot use that key for any further
communication). A guest can reload the driver will different VMPCK id
and try again.

SNP cannot really cover DOS at all since the VMM could just never
schedule the VM. In this case we know that the hypervisor is trying to
mess with the guest, so my preference would be to stop sending guest
messages to prevent that duplicated IV usage. If one caller gets an
EBADMSG it knows its in this case but the rest of userspace has no
idea. Maybe log an error?

quoted

Yap, we cannot protect against the DOS. This is why I was saying that we
zero the key from secrets page so that guest cannot use that key for any
future communication (whether its from rest of userspace or kexec
kernel). I can update the driver to log the message and ensure that
future messages will *not* use that key. The VMPCK ID is a module
params, so a guest can reload the driver to use different VMPCK.

Duh! Sorry I thought you said we needed a VMPL0 SVSM to do that. That
sounds great.

quoted

quoted

thanks