Re: [PATCH v10 48/50] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event

[Alexey Kardashevskiy <aik@amd.com>]

On 18/10/23 03:27, Sean Christopherson wrote:
> On Mon, Oct 16, 2023, Dionna Amalie Glaze wrote:
>>> +
>>> +       /*
>>> +        * If a VMM-specific certificate blob hasn't been provided, grab the
>>> +        * host-wide one.
>>> +        */
>>> +       snp_certs = sev_snp_certs_get(sev->snp_certs);
>>> +       if (!snp_certs)
>>> +               snp_certs = sev_snp_global_certs_get();
>>> +
>>
>> This is where the generation I suggested adding would get checked. If
>> the instance certs' generation is not the global generation, then I
>> think we need a way to return to the VMM to make that right before
>> continuing to provide outdated certificates.
>> This might be an unreasonable request, but the fact that the certs and
>> reported_tcb can be set while a VM is running makes this an issue.
> 
> Before we get that far, the changelogs need to explain why the kernel is storing
> userspace blobs in the first place.  The whole thing is a bit of a mess.
> 
> sev_snp_global_certs_get() has data races that could lead to variations of TOCTOU
> bugs: sev_ioctl_snp_set_config() can overwrite psp_master->sev_data->snp_certs
> while sev_snp_global_certs_get() is running.  If the compiler reloads snp_certs
> between bumping the refcount and grabbing the pointer, KVM will end up leaking a
> refcount and consuming a pointer without a refcount.
> 
> 	if (!kref_get_unless_zero(&certs->kref))
> 		return NULL;
> 
> 	return certs;

I'm missing something here. The @certs pointer is on the stack, if it is 
being released elsewhere - kref_get_unless_zero() is going to fail and 
return NULL. How can this @certs not have the refcount incremented?


> If allocating memory for the certs fails, the kernel will have set the config
> but not store the corresponding certs.


Ah true.

> 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
> 		if (ret)
> 			goto e_free;
> 
> 		memcpy(&sev->snp_config, &config, sizeof(config));
> 	}
> 
> 	/*
> 	 * If the new certs are passed then cache it else free the old certs.
> 	 */
> 	if (input.certs_len) {
> 		snp_certs = sev_snp_certs_new(certs, input.certs_len);
> 		if (!snp_certs) {
> 			ret = -ENOMEM;
> 			goto e_free;
> 		}
> 	}
> 
> Reasoning about ordering is also difficult, e.g. what is KVM's contract with
> userspace in terms of recognizing new global certs?
 >
> I don't understand why the kernel needs to manage the certs.  AFAICT the so called
> global certs aren't an input to SEV_CMD_SNP_CONFIG, i.e. SNP_SET_EXT_CONFIG is
> purely a software defined thing.
> > The easiest solution I can think of is to have KVM provide a chunk of 
memory in
> kvm_sev_info for SNP guests that userspace can mmap(), a la vcpu->run.
> 
> 	struct sev_snp_certs {
> 		u8 data[KVM_MAX_SEV_SNP_CERT_SIZE];
> 		u32 size;
> 		u8 pad[<size to make the struct page aligned>];
> 	};
> 
> When the guest requests the certs, KVM does something like:
> 
> 	certs_size = READ_ONCE(sev->snp_certs->size);
> 	if (certs_size > sizeof(sev->snp_certs->data) ||
> 	    !IS_ALIGNED(certs_size, PAGE_SIZE))
> 		certs_size = 0;
> 
> 	if (certs_size && (data_npages << PAGE_SHIFT) < certs_size) {
> 		vcpu->arch.regs[VCPU_REGS_RBX] = certs_size >> PAGE_SHIFT;
> 		exitcode = SNP_GUEST_VMM_ERR(SNP_GUEST_VMM_ERR_INVALID_LEN);
> 		goto cleanup;
> 	}
> 
> 	...
> 
> 	if (certs_size &&
> 	    kvm_write_guest(kvm, data_gpa, sev->snp_certs->data, certs_size))
> 		exitcode = SEV_RET_INVALID_ADDRESS;
> 
> If userspace wants to provide garbage to the guest, so be it, not KVM's problem.
> That way, whether the VM gets the global cert or a per-VM cert is purely a userspace
> concern.

The global cert lives in CCP (/dev/sev), the per VM cert lives in 
kvmvm_fd. "A la vcpu->run" is fine for the latter but for the former we 
need something else. And there is scenario when one global certs blob is 
what is needed and copying it over multiple VMs seems suboptimal.

> If userspace needs to *stall* cert requests, e.g. while the certs are being updated,

afaik it does not need to.

> then that's a different issue entirely.  If the GHCB allows telling the guest to
> retry the request, then it should be trivially easy to solve, e.g. add a flag in
> sev_snp_certs.  If KVM must "immediately" handle the request, then we'll need more
> elaborate uAPI.


-- 
Alexey