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

[Sean Christopherson <seanjc@google.com>]

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;

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

	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.

If userspace needs to *stall* cert requests, e.g. while the certs are being updated,
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.