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
