Re: [PATCH v10 13/26] gunyah: vm_mgr: Add ioctls to support basic non-proxy VM boot

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2/15/2023 10:35 PM, Greg Kroah-Hartman wrote:
On Tue, Feb 14, 2023 at 01:24:26PM -0800, Elliot Berman wrote:
+	case GH_VM_SET_DTB_CONFIG: {
+		struct gh_vm_dtb_config dtb_config;
+
+		if (copy_from_user(&dtb_config, argp, sizeof(dtb_config)))
+			return -EFAULT;
+
+		dtb_config.size = PAGE_ALIGN(dtb_config.size);
+		ghvm->dtb_config = dtb_config;

Do you really mean to copy this tiny structure twice (once from
userspace and the second time off of the stack)?  If so, why?

Ah, yes this can be optimized to copy directly.

And where are the values of the structure checked for validity?  Can any
64bit value work for size and "gpa"?


The values get checked when starting the VM

static int gh_vm_start(struct gh_vm *ghvm)
	...
mapping = gh_vm_mem_find_mapping(ghvm, ghvm->dtb_config.gpa, ghvm->dtb_config.size);
	if (!mapping) {
		pr_warn("Failed to find the memory_handle for DTB\n");
		ret = -EINVAL;
		goto err;
	}

If user passes an address that they've not set up, then gh_vm_mem_find_mapping returns NULL and GH_VM_START ioctl fails.

I've not done the check from the GH_VM_SET_DTB_CONFIG ioctl itself because I didn't want to require userspace to share the memory first. We'd need to check again anyway since user could SET_USER_MEMORY, SET_DTB_CONFIG, SET_USER_MEMORY (remove), VM_START.

Thanks,
Elliot




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux