Re: [PATCH v2 3/3] x86/sev: Add SEV-SNP CipherTextHiding support

[Sean Christopherson <seanjc@google.com>]

On Wed, Oct 02, 2024, Ashish Kalra wrote:
> Hello Peter,
> 
> On 10/2/2024 9:58 AM, Peter Gonda wrote:
> > On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
> >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> >> index 564daf748293..77900abb1b46 100644
> >> --- a/drivers/crypto/ccp/sev-dev.c
> >> +++ b/drivers/crypto/ccp/sev-dev.c
> >> @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true;
> >>  module_param(psp_init_on_probe, bool, 0444);
> >>  MODULE_PARM_DESC(psp_init_on_probe, "  if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
> >>
> >> +static bool cipher_text_hiding = true;
> >> +module_param(cipher_text_hiding, bool, 0444);
> >> +MODULE_PARM_DESC(cipher_text_hiding, "  if true, the PSP will enable Cipher Text Hiding");
> >> +
> >> +static int max_snp_asid;
> >> +module_param(max_snp_asid, int, 0444);
> >> +MODULE_PARM_DESC(max_snp_asid, "  override MAX_SNP_ASID for Cipher Text Hiding");
> > My read of the spec is if Ciphertext hiding is not enabled there is no
> > additional split in the ASID space. Am I understanding that correctly?
> Yes that is correct.
> > If so, I don't think we want to enable ciphertext hiding by default
> > because it might break whatever management of ASIDs systems already
> > have. For instance right now we have to split SEV-ES and SEV ASIDS,
> > and SNP guests need SEV-ES ASIDS. This change would half the # of SNP
> > enable ASIDs on a system.
> 
> My thought here is that we probably want to enable Ciphertext hiding by
> default as that should fix any security issues and concerns around SNP
> encryption as .Ciphertext hiding prevents host accesses from reading the
> ciphertext of SNP guest private memory.
> 
> This patch does add a new CCP module parameter, max_snp_asid, which can be
> used to dedicate all SEV-ES ASIDs to SNP guests.
> 
> >
> > Also should we move the ASID splitting code to be all in one place?
> > Right now KVM handles it in sev_hardware_setup().
> 
> Yes, but there is going to be a separate set of patches to move all ASID
> handling code to CCP module.
> 
> This refactoring won't be part of the SNP ciphertext hiding support patches.

It should, because that's not a "refactoring", that's a change of roles and
responsibilities.  And this series does the same; even worse, this series leaves
things in a half-baked state, where the CCP and KVM have a weird shared ownership
of ASID management.

I'm ok with essentially treating CipherText Hiding enablement as an extension of
firmware, e.g. it's better than having to go into UEFI settings to toggle the
feature on/off.  But we need to have a clear, well-defined vision for how we want
this all to look in the end.