On Wed, Oct 27, 2021 at 3:13 PM Brijesh Singh <brijesh.singh@xxxxxxx> wrote: > > > > 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. Duh! Sorry I thought you said we needed a VMPL0 SVSM to do that. That sounds great. > > > >> thanks
