Re: [PATCH v3 3/7] virt: geniezone: Introduce GenieZone hypervisor support

[Marc Zyngier]

On Fri, 12 May 2023 09:04:01 +0100,
Yi-De Wu <yi-de.wu@xxxxxxxxxxxx> wrote:
> 
> From: "Yingshiuan Pan" <yingshiuan.pan@xxxxxxxxxxxx>
> 
> GenieZone is MediaTek hypervisor solution, and it is running in EL2
> stand alone as a type-I hypervisor. This patch exports a set of ioctl
> interfaces for userspace VMM (e.g., crosvm) to operate guest VMs
> lifecycle (creation and destroy) on GenieZone.
> 
> Signed-off-by: Yingshiuan Pan <yingshiuan.pan@xxxxxxxxxxxx>
> Signed-off-by: Yi-De Wu <yi-de.wu@xxxxxxxxxxxx>

[...]

> +/**
> + * gzvm_gfn_to_pfn_memslot() - Translate gfn (guest ipa) to pfn (host pa),
> + *			       result is in @pfn
> + *
> + * Leverage KVM's gfn_to_pfn_memslot(). Because gfn_to_pfn_memslot() needs
> + * kvm_memory_slot as parameter, this function populates necessary fileds
> + * for calling gfn_to_pfn_memslot().
> + *
> + * Return:
> + * * 0			- Succeed
> + * * -EFAULT		- Failed to convert
> + */
> +static int gzvm_gfn_to_pfn_memslot(struct gzvm_memslot *memslot, u64 gfn, u64 *pfn)
> +{
> +	hfn_t __pfn;
> +	struct kvm_memory_slot kvm_slot = {0};
> +
> +	kvm_slot.base_gfn = memslot->base_gfn;
> +	kvm_slot.npages = memslot->npages;
> +	kvm_slot.dirty_bitmap = NULL;
> +	kvm_slot.userspace_addr = memslot->userspace_addr;
> +	kvm_slot.flags = memslot->flags;
> +	kvm_slot.id = memslot->slot_id;
> +	kvm_slot.as_id = 0;
> +
> +	__pfn = gfn_to_pfn_memslot(&kvm_slot, gfn);
> +	if (is_error_noslot_pfn(__pfn)) {
> +		*pfn = 0;
> +		return -EFAULT;
> +	}

I have commented on this before: there is absolutely *no way* that you
can use KVM as the unwilling helper for your stuff. You are passing
uninitialised data to the core KVM, completely ignoring the semantics
of all the other fields.

More importantly, you are now holding us responsible for any breakage
that would be caused to your code if we change the internals of this
*PRIVATE FUNCTION*.

Do you see Xen or Hyper-V using KVM's internals as some sort of
backend to make their life easier? No, because they understand that
this is off-limits, and creates an unhealthy dependency for both
hypervisors.

So this is a strong NAK. And you can trust me to keep voicing my
opposition to this sort of horror, wherever I will see these patches.

	M.

-- 
Without deviation from the norm, progress is not possible.