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
