From 57bf26dc17d903787c444f8cfa5cbda0943c56a2 Mon Sep 17 00:00:00 2001 From: Yonghua Huang Date: Fri, 14 Dec 2018 00:03:52 +0800 Subject: [PATCH] hv: fix possible buffer overflow issues - cpu_secondary_init() @cpu.c - ptirq_intx_pin_remap() @ assign.c etc. Tracked-On: #1252 Signed-off-by: Yonghua Huang Acked-by: Eddie Dong --- hypervisor/arch/x86/assign.c | 121 ++++++++++++------------ hypervisor/arch/x86/cpu.c | 3 + hypervisor/common/hypercall.c | 66 +++++++------ hypervisor/common/ptdev.c | 3 +- hypervisor/include/public/acrn_common.h | 2 +- 5 files changed, 99 insertions(+), 96 deletions(-) diff --git a/hypervisor/arch/x86/assign.c b/hypervisor/arch/x86/assign.c index 1dad43d15..acb919efe 100644 --- a/hypervisor/arch/x86/assign.c +++ b/hypervisor/arch/x86/assign.c @@ -628,7 +628,8 @@ static void activate_physical_ioapic(struct acrn_vm *vm, int32_t ptirq_intx_pin_remap(struct acrn_vm *vm, uint8_t virt_pin, enum ptirq_vpin_source vpin_src) { - struct ptirq_remapping_info *entry; + int32_t status = 0; + struct ptirq_remapping_info *entry = NULL; bool need_switch_vpin_src = false; DEFINE_IOAPIC_SID(virt_sid, virt_pin, vpin_src); bool pic_pin = (vpin_src == PTDEV_VPIN_PIC); @@ -647,80 +648,80 @@ int32_t ptirq_intx_pin_remap(struct acrn_vm *vm, uint8_t virt_pin, /* no remap for hypervisor owned intx */ #ifdef CONFIG_COM_IRQ if (ptdev_hv_owned_intx(vm, &virt_sid)) { - goto END; + status = -ENODEV; } #endif /* CONFIG_COM_IRQ */ - /* query if we have virt to phys mapping */ - spinlock_obtain(&ptdev_lock); - entry = ptirq_lookup_entry_by_vpin(vm, virt_pin, pic_pin); - if (entry == NULL) { - if (is_vm0(vm)) { + if (status || (pic_pin && (virt_pin >= NR_VPIC_PINS_TOTAL))) { + status = -EINVAL; + } else { + /* query if we have virt to phys mapping */ + spinlock_obtain(&ptdev_lock); + entry = ptirq_lookup_entry_by_vpin(vm, virt_pin, pic_pin); + if (entry == NULL) { + if (is_vm0(vm)) { - /* for vm0, there is chance of vpin source switch - * between vPIC & vIOAPIC for one legacy phys_pin. - * - * here checks if there is already mapping entry from - * the other vpin source for legacy pin. If yes, then - * switch vpin source is needed - */ - if (virt_pin < NR_LEGACY_PIN) { - uint8_t vpin = pic_ioapic_pin_map[virt_pin]; + /* for vm0, there is chance of vpin source switch + * between vPIC & vIOAPIC for one legacy phys_pin. + * + * here checks if there is already mapping entry from + * the other vpin source for legacy pin. If yes, then + * switch vpin source is needed + */ + if (virt_pin < NR_LEGACY_PIN) { + uint8_t vpin = pic_ioapic_pin_map[virt_pin]; - entry = ptirq_lookup_entry_by_vpin(vm, vpin, !pic_pin); - if (entry != NULL) { - need_switch_vpin_src = true; + entry = ptirq_lookup_entry_by_vpin(vm, vpin, !pic_pin); + if (entry != NULL) { + need_switch_vpin_src = true; + } } - } - /* entry could be updated by above switch check */ - if (entry == NULL) { - uint8_t phys_pin = virt_pin; - - /* fix vPIC pin to correct native IOAPIC pin */ - if (pic_pin) { - phys_pin = pic_ioapic_pin_map[virt_pin]; - } - entry = add_intx_remapping(vm, - virt_pin, phys_pin, pic_pin); + /* entry could be updated by above switch check */ if (entry == NULL) { - pr_err("%s, add intx remapping failed", - __func__); - spinlock_release(&ptdev_lock); - return -ENODEV; + uint8_t phys_pin = virt_pin; + + /* fix vPIC pin to correct native IOAPIC pin */ + if (pic_pin) { + phys_pin = pic_ioapic_pin_map[virt_pin]; + } + entry = add_intx_remapping(vm, + virt_pin, phys_pin, pic_pin); + if (entry == NULL) { + pr_err("%s, add intx remapping failed", + __func__); + status = -ENODEV; + } } + } else { + /* ptirq_intx_pin_remap is triggered by vPIC/vIOAPIC + * everytime a pin get unmask, here filter out pins + * not get mapped. + */ + status = -ENODEV; } - } else { - /* ptirq_intx_pin_remap is triggered by vPIC/vIOAPIC - * everytime a pin get unmask, here filter out pins - * not get mapped. - */ - spinlock_release(&ptdev_lock); - goto END; } + spinlock_release(&ptdev_lock); } - /* if vpin source need switch */ - if (need_switch_vpin_src) { - dev_dbg(ACRN_DBG_IRQ, - "IOAPIC pin=%hhu pirq=%u vpin=%d switch from %s to %s " - "vpin=%d for vm%d", entry->phys_sid.intx_id.pin, - entry->allocated_pirq, entry->virt_sid.intx_id.pin, - (vpin_src == 0)? "vPIC" : "vIOAPIC", - (vpin_src == 0)? "vIOPIC" : "vPIC", - virt_pin, entry->vm->vm_id); - entry->virt_sid.value = virt_sid.value; + if (!status) { + spinlock_obtain(&ptdev_lock); + /* if vpin source need switch */ + if ((need_switch_vpin_src) && (entry != NULL)) { + dev_dbg(ACRN_DBG_IRQ, + "IOAPIC pin=%hhu pirq=%u vpin=%d switch from %s to %s vpin=%d for vm%d", + entry->phys_sid.intx_id.pin, + entry->allocated_pirq, entry->virt_sid.intx_id.pin, + (vpin_src == 0) ? "vPIC" : "vIOAPIC", + (vpin_src == 0) ? "vIOPIC" : "vPIC", + virt_pin, entry->vm->vm_id); + entry->virt_sid.value = virt_sid.value; + } + spinlock_release(&ptdev_lock); + activate_physical_ioapic(vm, entry); } - spinlock_release(&ptdev_lock); - activate_physical_ioapic(vm, entry); - dev_dbg(ACRN_DBG_IRQ, - "IOAPIC pin=%hhu pirq=%u assigned to vm%d %s vpin=%d", - entry->phys_sid.intx_id.pin, entry->allocated_pirq, - entry->vm->vm_id, vpin_src == PTDEV_VPIN_PIC ? - "vPIC" : "vIOAPIC", virt_pin); -END: - return 0; + return status; } /* @pre vm != NULL diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c index ec119b9ce..e0c48b379 100644 --- a/hypervisor/arch/x86/cpu.c +++ b/hypervisor/arch/x86/cpu.c @@ -407,6 +407,9 @@ void init_cpu_pre(uint16_t pcpu_id) early_init_lapic(); pcpu_id = get_cpu_id_from_lapic_id(get_cur_lapic_id()); + if (pcpu_id >= CONFIG_MAX_PCPU_NUM) { + panic("Invalid pCPU ID!"); + } } bitmap_set_nolock(pcpu_id, &pcpu_active_bitmap); diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c index 0bcb92d0d..c06f3fe2d 100644 --- a/hypervisor/common/hypercall.c +++ b/hypervisor/common/hypercall.c @@ -1049,46 +1049,44 @@ int32_t hcall_get_cpu_pm_state(struct acrn_vm *vm, uint64_t cmd, uint64_t param) */ int32_t hcall_vm_intr_monitor(struct acrn_vm *vm, uint16_t vmid, uint64_t param) { + int32_t status = -EINVAL; struct acrn_intr_monitor *intr_hdr; uint64_t hpa; struct acrn_vm *target_vm = get_vm_from_vmid(vmid); - if (target_vm == NULL) { - return -1; + if (target_vm != NULL) { + + /* the param for this hypercall is page aligned */ + hpa = gpa2hpa(vm, param); + if (hpa != INVALID_HPA) { + intr_hdr = (struct acrn_intr_monitor *)hpa2hva(hpa); + stac(); + if (intr_hdr->buf_cnt <= (MAX_PTDEV_NUM * 2U)) { + switch (intr_hdr->cmd) { + case INTR_CMD_GET_DATA: + intr_hdr->buf_cnt = ptirq_get_intr_data(target_vm, + intr_hdr->buffer, intr_hdr->buf_cnt); + break; + + case INTR_CMD_DELAY_INT: + /* buffer[0] is the delay time (in MS), if 0 to cancel delay */ + target_vm->intr_inject_delay_delta = + intr_hdr->buffer[0] * CYCLES_PER_MS; + break; + + default: + /* if cmd wrong it goes here should not happen */ + break; + } + + status = 0; + pr_dbg("intr monitor:%d, cnt=%d", intr_hdr->cmd, intr_hdr->buf_cnt); + } + clac(); + } } - /* the param for this hypercall is page aligned */ - hpa = gpa2hpa(vm, param); - if (hpa == INVALID_HPA) { - pr_err("%s,vm[%hu] gpa 0x%llx,GPA is unmapping.", - __func__, vm->vm_id, param); - return -EINVAL; - } - - intr_hdr = (struct acrn_intr_monitor *)hpa2hva(hpa); - - stac(); - switch (intr_hdr->cmd) { - case INTR_CMD_GET_DATA: - intr_hdr->buf_cnt = ptirq_get_intr_data(target_vm, - intr_hdr->buffer, intr_hdr->buf_cnt); - break; - - case INTR_CMD_DELAY_INT: - /* buffer[0] is the delay time (in MS), if 0 to cancel delay */ - target_vm->intr_inject_delay_delta = - intr_hdr->buffer[0] * CYCLES_PER_MS; - break; - - default: - /* if cmd wrong it goes here should not happen */ - break; - } - - pr_dbg("intr monitor:%d, cnt=%d", intr_hdr->cmd, intr_hdr->buf_cnt); - clac(); - - return 0; + return status; } /** diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c index 057004b0e..52cdebd23 100644 --- a/hypervisor/common/ptdev.c +++ b/hypervisor/common/ptdev.c @@ -123,7 +123,8 @@ void ptirq_release_entry(struct ptirq_remapping_info *entry) list_del_init(&entry->softirq_node); spinlock_irqrestore_release(&entry->vm->softirq_dev_lock, rflags); atomic_clear32(&entry->active, ACTIVE_FLAG); - bitmap_clear_nolock((entry->ptdev_entry_id) & 0x3FU, &ptirq_entry_bitmaps[(entry->ptdev_entry_id) >> 6U]); + bitmap_clear_nolock((entry->ptdev_entry_id) & 0x3FU, + &ptirq_entry_bitmaps[((entry->ptdev_entry_id) & 0x3FU) >> 6U]); } /* interrupt context */ diff --git a/hypervisor/include/public/acrn_common.h b/hypervisor/include/public/acrn_common.h index 02e951786..e179a39ea 100644 --- a/hypervisor/include/public/acrn_common.h +++ b/hypervisor/include/public/acrn_common.h @@ -626,7 +626,7 @@ enum pm_cmd_type { * * the parameter for HC_VM_INTR_MONITOR hypercall */ -#define MAX_PTDEV_NUM 24 +#define MAX_PTDEV_NUM 24U struct acrn_intr_monitor { /** sub command for intr monitor */ uint32_t cmd;