From ede1459e1976f07837e73b008e53b3ffd09b0e8e Mon Sep 17 00:00:00 2001 From: "Yan, Like" Date: Fri, 29 Mar 2019 14:27:13 +0800 Subject: [PATCH] hv: fix the vm pointer check before use After using get_vm_from_vmid(), vm pointer is always not NULL. But there are still many NULL pointer checks. This commit replaced the NULL vm pointer check with a validation check which checks the vm status. In addition, NULL check for pointer returned by get_sos_vm() and get_vm_config() is removed. Tracked-On: #2520 Signed-off-by: Yan, Like Acked-by: Eddie Dong --- hypervisor/arch/x86/configs/vm_config.c | 1 + hypervisor/arch/x86/guest/vm.c | 16 +++++++---- hypervisor/common/hypercall.c | 37 ++++++++++++------------- hypervisor/common/io_req.c | 6 ++-- hypervisor/debug/profiling.c | 2 +- hypervisor/debug/shell.c | 10 +++---- hypervisor/debug/vuart.c | 2 +- hypervisor/dm/vpci/vpci.c | 8 ++---- hypervisor/include/arch/x86/guest/vm.h | 1 + 9 files changed, 43 insertions(+), 40 deletions(-) diff --git a/hypervisor/arch/x86/configs/vm_config.c b/hypervisor/arch/x86/configs/vm_config.c index 4a8368e3f..a055b669b 100644 --- a/hypervisor/arch/x86/configs/vm_config.c +++ b/hypervisor/arch/x86/configs/vm_config.c @@ -53,6 +53,7 @@ static struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM] __aligned(PAGE_SIZE) /* * @pre vm_id < CONFIG_MAX_VM_NUM + * @post return != NULL */ struct acrn_vm_config *get_vm_config(uint16_t vm_id) { diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c index 4c1897f75..a78c87f6b 100644 --- a/hypervisor/arch/x86/guest/vm.c +++ b/hypervisor/arch/x86/guest/vm.c @@ -48,18 +48,24 @@ uint16_t find_free_vm_id(void) return (vm_config->type == UNDEFINED_VM) ? id : INVALID_VM_ID; } +/** + * @pre vm != NULL && vm->vmid < CONFIG_MAX_VM_NUM + */ static inline void free_vm_id(const struct acrn_vm *vm) { struct acrn_vm_config *vm_config = get_vm_config(vm->vm_id); - if (vm_config != NULL) { - vm_config->type = UNDEFINED_VM; - } + vm_config->type = UNDEFINED_VM; +} + +bool is_valid_vm(const struct acrn_vm *vm) +{ + return (vm != NULL) && (vm->state != VM_STATE_INVALID); } bool is_sos_vm(const struct acrn_vm *vm) { - return (vm != NULL) && (vm == sos_vm_ptr); + return (vm != NULL) && (get_vm_config(vm->vm_id)->type == SOS_VM); } /** @@ -447,7 +453,7 @@ int32_t create_vm(uint16_t vm_id, struct acrn_vm_config *vm_config, struct acrn_ } } - if (need_cleanup && (vm != NULL)) { + if (need_cleanup && is_valid_vm(vm)) { if (vm->arch_vm.nworld_eptp != NULL) { (void)memset(vm->arch_vm.nworld_eptp, 0U, PAGE_SIZE); } diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c index cfff4b662..cf24d5dff 100644 --- a/hypervisor/common/hypercall.c +++ b/hypervisor/common/hypercall.c @@ -181,7 +181,7 @@ int32_t hcall_destroy_vm(uint16_t vmid) int32_t ret; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if (target_vm == NULL) { + if (!is_valid_vm(target_vm)) { ret = -1; } else { ret = shutdown_vm(target_vm); @@ -206,7 +206,7 @@ int32_t hcall_start_vm(uint16_t vmid) int32_t ret = 0; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if (target_vm == NULL) { + if (!is_valid_vm(target_vm)) { ret = -1; } else if (target_vm->sw.io_shared_page == NULL) { ret = -1; @@ -233,7 +233,7 @@ int32_t hcall_pause_vm(uint16_t vmid) struct acrn_vm *target_vm = get_vm_from_vmid(vmid); int32_t ret; - if (target_vm == NULL) { + if (!is_valid_vm(target_vm)) { ret = -1; } else { pause_vm(target_vm); @@ -265,7 +265,7 @@ int32_t hcall_create_vcpu(struct acrn_vm *vm, uint16_t vmid, uint64_t param) struct acrn_create_vcpu cv; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if ((target_vm == NULL) || (param == 0U)) { + if (!is_valid_vm(target_vm) || (param == 0U)) { ret = -1; } else if (copy_from_gpa(vm, &cv, param, sizeof(cv)) != 0) { pr_err("%s: Unable copy param to vm\n", __func__); @@ -300,7 +300,7 @@ int32_t hcall_reset_vm(uint16_t vmid) struct acrn_vm *target_vm = get_vm_from_vmid(vmid); int32_t ret; - if ((target_vm == NULL) || is_sos_vm(target_vm)) { + if (!is_valid_vm(target_vm) || is_sos_vm(target_vm)) { ret = -1; } else { ret = reset_vm(target_vm); @@ -330,7 +330,7 @@ int32_t hcall_set_vcpu_regs(struct acrn_vm *vm, uint16_t vmid, uint64_t param) int32_t ret = -1; /* Only allow setup init ctx while target_vm is inactive */ - if ((target_vm != NULL) && (param != 0U) && (!is_sos_vm(target_vm)) && (target_vm->state != VM_STARTED)) { + if (is_valid_vm(target_vm) && (param != 0U) && (!is_sos_vm(target_vm)) && (target_vm->state != VM_STARTED)) { if (copy_from_gpa(vm, &vcpu_regs, param, sizeof(vcpu_regs)) != 0) { pr_err("%s: Unable copy param to vm\n", __func__); } else if (vcpu_regs.vcpu_id >= CONFIG_MAX_VCPUS_PER_VM) { @@ -368,7 +368,7 @@ int32_t hcall_set_irqline(const struct acrn_vm *vm, uint16_t vmid, struct acrn_vm *target_vm = get_vm_from_vmid(vmid); int32_t ret; - if (target_vm == NULL) { + if (!is_valid_vm(target_vm)) { ret = -EINVAL; } else if (ops->gsi >= vioapic_pincount(vm)) { ret = -EINVAL; @@ -457,7 +457,7 @@ int32_t hcall_inject_msi(struct acrn_vm *vm, uint16_t vmid, uint64_t param) struct acrn_msi_entry msi; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if (target_vm != NULL) { + if (is_valid_vm(target_vm)) { (void)memset((void *)&msi, 0U, sizeof(msi)); if (copy_from_gpa(vm, &msi, param, sizeof(msi)) != 0) { pr_err("%s: Unable copy param to vm\n", __func__); @@ -499,7 +499,7 @@ int32_t hcall_set_ioreq_buffer(struct acrn_vm *vm, uint16_t vmid, uint64_t param int32_t ret; (void)memset((void *)&iobuf, 0U, sizeof(iobuf)); - if ((target_vm == NULL) || (copy_from_gpa(vm, &iobuf, param, sizeof(iobuf)) != 0)) { + if (!is_valid_vm(target_vm) || (copy_from_gpa(vm, &iobuf, param, sizeof(iobuf)) != 0)) { pr_err("%p %s: target_vm is not valid or Unable copy param to vm\n", target_vm, __func__); ret = -1; } else { @@ -543,7 +543,7 @@ int32_t hcall_notify_ioreq_finish(uint16_t vmid, uint16_t vcpu_id) int32_t ret = -EINVAL; /* make sure we have set req_buf */ - if ((target_vm != NULL) && (target_vm->sw.io_shared_page != NULL)) { + if (is_valid_vm(target_vm) && (target_vm->sw.io_shared_page != NULL)) { dev_dbg(ACRN_DBG_HYCALL, "[%d] NOTIFY_FINISH for vcpu %d", vmid, vcpu_id); @@ -761,7 +761,7 @@ int32_t hcall_write_protect_page(struct acrn_vm *vm, uint16_t vmid, uint64_t wp_ struct acrn_vm *target_vm = get_vm_from_vmid(vmid); int32_t ret; - if ((target_vm == NULL) || is_sos_vm(target_vm)) { + if (!is_valid_vm(target_vm) || is_sos_vm(target_vm)) { pr_err("%p %s: target_vm is invalid or Targeting to service vm", target_vm, __func__); ret = -EINVAL; } else { @@ -798,7 +798,7 @@ int32_t hcall_gpa_to_hpa(struct acrn_vm *vm, uint16_t vmid, uint64_t param) struct acrn_vm *target_vm = get_vm_from_vmid(vmid); (void)memset((void *)&v_gpa2hpa, 0U, sizeof(v_gpa2hpa)); - if ((target_vm == NULL) || (copy_from_gpa(vm, &v_gpa2hpa, param, sizeof(v_gpa2hpa)) != 0)) { + if (!is_valid_vm(target_vm) || (copy_from_gpa(vm, &v_gpa2hpa, param, sizeof(v_gpa2hpa)) != 0)) { pr_err("target_vm is invalid or HCALL gpa2hpa: Unable copy param from vm\n"); ret = -1; } else { @@ -838,7 +838,7 @@ int32_t hcall_assign_ptdev(struct acrn_vm *vm, uint16_t vmid, uint64_t param) bool bdf_valid = true; bool iommu_valid = true; - if (target_vm != NULL) { + if (is_valid_vm(target_vm)) { if (param < 0x10000UL) { bdf = (uint16_t) param; } else { @@ -898,7 +898,7 @@ int32_t hcall_deassign_ptdev(struct acrn_vm *vm, uint16_t vmid, uint64_t param) bool bdf_valid = true; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if (target_vm == NULL) { + if (!is_valid_vm(target_vm)) { ret = -1; } else { if (param < 0x10000UL) { @@ -938,7 +938,7 @@ int32_t hcall_set_ptdev_intr_info(struct acrn_vm *vm, uint16_t vmid, uint64_t pa struct acrn_vm *target_vm = get_vm_from_vmid(vmid); (void)memset((void *)&irq, 0U, sizeof(irq)); - if (target_vm == NULL) { + if (!is_valid_vm(target_vm)) { ret = -1; } else if (copy_from_gpa(vm, &irq, param, sizeof(irq)) != 0) { pr_err("%s: Unable copy param to vm\n", __func__); @@ -983,7 +983,7 @@ hcall_reset_ptdev_intr_info(struct acrn_vm *vm, uint16_t vmid, uint64_t param) struct hc_ptdev_irq irq; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if (target_vm != NULL) { + if (is_valid_vm(target_vm)) { (void)memset((void *)&irq, 0U, sizeof(irq)); if (copy_from_gpa(vm, &irq, param, sizeof(irq)) != 0) { @@ -1036,7 +1036,7 @@ int32_t hcall_get_cpu_pm_state(struct acrn_vm *vm, uint64_t cmd, uint64_t param) target_vm_id = (uint16_t)((cmd & PMCMD_VMID_MASK) >> PMCMD_VMID_SHIFT); target_vm = get_vm_from_vmid(target_vm_id); - if (target_vm == NULL) { + if (!is_valid_vm(target_vm)) { ret = -1; } else { @@ -1153,8 +1153,7 @@ int32_t hcall_vm_intr_monitor(struct acrn_vm *vm, uint16_t vmid, uint64_t param) uint64_t hpa; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if (target_vm != NULL) { - + if (is_valid_vm(target_vm)) { /* the param for this hypercall is page aligned */ hpa = gpa2hpa(vm, param); if (hpa != INVALID_HPA) { diff --git a/hypervisor/common/io_req.c b/hypervisor/common/io_req.c index 4bc880573..839e2aad8 100644 --- a/hypervisor/common/io_req.c +++ b/hypervisor/common/io_req.c @@ -23,11 +23,9 @@ static void fire_vhm_interrupt(void) struct acrn_vcpu *vcpu; sos_vm = get_sos_vm(); - if (sos_vm != NULL) { - vcpu = vcpu_from_vid(sos_vm, BOOT_CPU_ID); + vcpu = vcpu_from_vid(sos_vm, BOOT_CPU_ID); - vlapic_set_intr(vcpu, acrn_vhm_vector, LAPIC_TRIG_EDGE); - } + vlapic_set_intr(vcpu, acrn_vhm_vector, LAPIC_TRIG_EDGE); } #if defined(HV_DEBUG) diff --git a/hypervisor/debug/profiling.c b/hypervisor/debug/profiling.c index 10d454992..7c5c5c627 100644 --- a/hypervisor/debug/profiling.c +++ b/hypervisor/debug/profiling.c @@ -881,7 +881,7 @@ int32_t profiling_vm_list_info(struct acrn_vm *vm, uint64_t addr) for (j = 0U; j < CONFIG_MAX_VM_NUM; j++) { tmp_vm = get_vm_from_vmid(j); - if (tmp_vm == NULL) { + if (!is_valid_vm(tmp_vm)) { break; } vm_info_list.num_vms++; diff --git a/hypervisor/debug/shell.c b/hypervisor/debug/shell.c index 7d0a85410..ed99921f0 100644 --- a/hypervisor/debug/shell.c +++ b/hypervisor/debug/shell.c @@ -604,7 +604,7 @@ static int32_t shell_list_vcpu(__unused int32_t argc, __unused char **argv) for (idx = 0U; idx < CONFIG_MAX_VM_NUM; idx++) { vm = get_vm_from_vmid(idx); - if (vm == NULL) { + if (!is_valid_vm(vm)) { continue; } foreach_vcpu(i, vm, vcpu) { @@ -754,7 +754,7 @@ static int32_t shell_vcpu_dumpreg(int32_t argc, char **argv) vcpu_id = (uint16_t)strtol_deci(argv[2]); vm = get_vm_from_vmid(vm_id); - if (vm == NULL) { + if (!is_valid_vm(vm)) { shell_puts("No vm found in the input \r\n"); status = -EINVAL; goto out; @@ -852,13 +852,13 @@ static int32_t shell_to_sos_console(__unused int32_t argc, __unused char **argv) #endif /* Get the virtual device node */ vm = get_vm_from_vmid(vm_id); - if (vm == NULL) { + if (!is_valid_vm(vm)) { return -EINVAL; } #ifdef CONFIG_PARTITION_MODE vm_config = get_vm_config(vm_id); - if (vm_config != NULL && vm_config->vm_vuart == false) { + if (!vm_config->vm_vuart) { snprintf(temp_str, TEMP_STR_SIZE, "No vUART configured for vm%d\n", vm_id); shell_puts(temp_str); return 0; @@ -1072,7 +1072,7 @@ static void get_vioapic_info(char *str_arg, size_t str_max, uint16_t vmid) struct acrn_vm *vm = get_vm_from_vmid(vmid); uint32_t pin, pincount; - if (vm == NULL) { + if (!is_valid_vm(vm)) { len = snprintf(str, size, "\r\nvm is not exist for vmid %hu", vmid); if (len >= size) { goto overflow; diff --git a/hypervisor/debug/vuart.c b/hypervisor/debug/vuart.c index 64214a220..948f23773 100644 --- a/hypervisor/debug/vuart.c +++ b/hypervisor/debug/vuart.c @@ -390,7 +390,7 @@ struct acrn_vuart *vuart_console_active(void) vm = get_sos_vm(); #endif - if (vm != NULL) { + if (is_valid_vm(vm)) { struct acrn_vuart *vu = vm_vuart(vm); if (vu->active) { diff --git a/hypervisor/dm/vpci/vpci.c b/hypervisor/dm/vpci/vpci.c index c5d8e2c15..b459399ac 100644 --- a/hypervisor/dm/vpci/vpci.c +++ b/hypervisor/dm/vpci/vpci.c @@ -554,11 +554,9 @@ void vpci_reset_ptdev_intr_info(const struct acrn_vm *target_vm, uint16_t vbdf, if (vdev->vpci->vm == target_vm) { vm = get_sos_vm(); - if (vm != NULL) { - vdev->vpci = &vm->vpci; - /* vbdf equals to pbdf in sos */ - vdev->vbdf.value = vdev->pdev->bdf.value; - } + vdev->vpci = &vm->vpci; + /* vbdf equals to pbdf in sos */ + vdev->vbdf.value = vdev->pdev->bdf.value; } } } diff --git a/hypervisor/include/arch/x86/guest/vm.h b/hypervisor/include/arch/x86/guest/vm.h index c5ce6d780..1b69e91c4 100644 --- a/hypervisor/include/arch/x86/guest/vm.h +++ b/hypervisor/include/arch/x86/guest/vm.h @@ -209,6 +209,7 @@ int32_t reset_vm(struct acrn_vm *vm); int32_t create_vm(uint16_t vm_id, struct acrn_vm_config *vm_config, struct acrn_vm **rtn_vm); void prepare_vm(uint16_t vm_id, struct acrn_vm_config *vm_config); void launch_vms(uint16_t pcpu_id); +bool is_valid_vm(const struct acrn_vm *vm); bool is_sos_vm(const struct acrn_vm *vm); uint16_t find_free_vm_id(void); struct acrn_vm *get_vm_from_vmid(uint16_t vm_id);