Messages in this thread |  | | Subject | Re: [PATCH v6 40/42] virt: Add SEV-SNP guest driver | From | Brijesh Singh <> | Date | Wed, 27 Oct 2021 16:12:52 -0500 |
| |
On 10/27/21 4:05 PM, Peter Gonda wrote: ....
>>>>> >>>>> 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? > >>
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.
>> thanks
|  |