From: Peter Dinda Date: Tue, 1 Sep 2015 19:54:38 +0000 (-0500) Subject: Cleanup and sanity-checking of integer overflow, null comparisons, dead code (Coverit... X-Git-Url: http://v3vee.org/palacios/gitweb/gitweb.cgi?p=palacios.git;a=commitdiff_plain;h=a5d2c00cc461b4a60a1360a2a0bba55cef467bab Cleanup and sanity-checking of integer overflow, null comparisons, dead code (Coverity static analysis) --- diff --git a/linux_module/iface-host-dev.c b/linux_module/iface-host-dev.c index c8f8f60..23adb35 100644 --- a/linux_module/iface-host-dev.c +++ b/linux_module/iface-host-dev.c @@ -369,6 +369,7 @@ static long host_dev_ioctl(struct file * fp, unsigned int val, unsigned long arg switch (op.type) { case PALACIOS_HOST_DEV_USER_REQUEST_READ_GUEST: { + // possible overflow here, but only if user is asking for too much... void *temp = palacios_alloc(op.len); DEEP_DEBUG_PRINT("palacios: hostdev: read guest\n"); diff --git a/linux_module/iface-keyed-stream.c b/linux_module/iface-keyed-stream.c index a6fb960..99524e1 100644 --- a/linux_module/iface-keyed-stream.c +++ b/linux_module/iface-keyed-stream.c @@ -1853,6 +1853,7 @@ static long keyed_stream_ioctl_user(struct file * filp, unsigned int ioctl, unsi return -EFAULT; } + // overflow possible here for very large request if (resize_op(&(s->op),size-sizeof(struct palacios_user_keyed_stream_op))) { ERROR("unable to resize op in user key push response\n"); palacios_spinlock_unlock_irqrestore(&(s->lock), flags); @@ -1934,6 +1935,7 @@ int keyed_stream_connect_user(struct v3_guest *guest, unsigned int cmd, unsigned return -1; } + // overflow possible here, but only if this is a huge guest request (>4GB) url = palacios_alloc(len); if (!url) { diff --git a/linux_module/main.c b/linux_module/main.c index 310c5a1..f69b269 100644 --- a/linux_module/main.c +++ b/linux_module/main.c @@ -122,6 +122,7 @@ static long v3_dev_ioctl(struct file * filp, guest->img_size = user_image.size; DEBUG("Palacios: Allocating kernel memory for guest image (%llu bytes)\n", user_image.size); + // overflow possible here, but only if guest image is probably to large for kernel anyway... guest->img = palacios_valloc(guest->img_size); if (!guest->img) { @@ -409,7 +410,7 @@ static int read_guests_details(struct seq_file *s, void *v) out: - if (mem) { palacios_vfree(mem); } + if (mem) { palacios_vfree(mem); } // dead code but kept for clarity if (core) { palacios_vfree(core); } if (base) { palacios_vfree(base); } diff --git a/linux_module/memtrack.c b/linux_module/memtrack.c index b64ab29..66c0c03 100644 --- a/linux_module/memtrack.c +++ b/linux_module/memtrack.c @@ -99,6 +99,7 @@ static int memtrack_snap(struct v3_guest *guest, //INFO("num_cores=%u",num_cores); + // overflow possible here, but only for an insane number of cores if (!(user_snap=palacios_alloc(sizeof(v3_mem_track_snapshot) + num_cores * sizeof(struct v3_core_mem_track)))) { ERROR("palacios: cannot allocate memory for copying user snapshot\n"); goto fail; diff --git a/linux_module/mm.c b/linux_module/mm.c index e4c374a..8eb8fc9 100644 --- a/linux_module/mm.c +++ b/linux_module/mm.c @@ -305,7 +305,6 @@ int palacios_init_mm( void ) { INFO("Could not allocate initial memory block for node %d below 4GB\n", node_id); if (!pgs) { ERROR("Could not allocate initial memory block for node %d without restrictions\n", node_id); - BUG_ON(!pgs); palacios_deinit_mm(); return -1; } diff --git a/palacios/src/devices/qcowdisk.c b/palacios/src/devices/qcowdisk.c index 910e9bd..6759682 100644 --- a/palacios/src/devices/qcowdisk.c +++ b/palacios/src/devices/qcowdisk.c @@ -275,7 +275,7 @@ static v3_qcow2_t *v3_qcow2_open(struct v3_vm_info* vm, char *path, int flags) res->fd = v3_file_open(vm, path, flags); - if (res->fd < 0) { + if (!res->fd) { ERROR("failed to open underlying file\n"); goto clean_mem; } diff --git a/palacios/src/devices/telnet_cons.c b/palacios/src/devices/telnet_cons.c index 5f82993..b9080f7 100644 --- a/palacios/src/devices/telnet_cons.c +++ b/palacios/src/devices/telnet_cons.c @@ -413,16 +413,8 @@ static struct v3_device_ops dev_ops = { static int key_handler( struct cons_state * state, uint8_t ascii) { PrintDebug(VM_NONE, VCORE_NONE, "Character recieved: 0x%x\n", ascii); - // printable - if (ascii < 0x80) { - const struct key_code * key = &(ascii_to_key_code[ascii]); - if (deliver_scan_code(state, (struct key_code *)key) == -1) { - PrintError(VM_NONE, VCORE_NONE, "Could not deliver scan code to vm\n"); - return -1; - } - - } else if (ascii == ESC_CHAR) { // Escape Key + if (ascii == ESC_CHAR) { // Escape Key // This means that another 2 characters are pending // receive it and deliver accordingly char esc_seq[2] = {0, 0}; @@ -457,7 +449,16 @@ static int key_handler( struct cons_state * state, uint8_t ascii) { } else if (esc_seq[1] == 'D') { // LEFT ARROW struct key_code left = { 0x4B, 0 }; deliver_scan_code(state, &left); + } + + } else if (ascii < 0x80) { // printable + const struct key_code * key = &(ascii_to_key_code[ascii]); + + if (deliver_scan_code(state, (struct key_code *)key) == -1) { + PrintError(VM_NONE, VCORE_NONE, "Could not deliver scan code to vm\n"); + return -1; } + } else { PrintError(VM_NONE, VCORE_NONE, "Invalid character received from network (%c) (code=%d)\n", ascii, ascii); diff --git a/palacios/src/palacios/svm_npt.h b/palacios/src/palacios/svm_npt.h index 3094d3d..f0d5262 100644 --- a/palacios/src/palacios/svm_npt.h +++ b/palacios/src/palacios/svm_npt.h @@ -63,6 +63,8 @@ static int handle_svm_invalidate_nested_addr(struct guest_info * info, addr_t in #endif switch(mode) { + // Note that the dead code here (for other than LONG and PROTECTED + // is kept here for clarity and parallelism with other impls case REAL: case PROTECTED: return invalidate_addr_32(info, inv_addr, actual_start, actual_end); @@ -95,6 +97,8 @@ static int handle_svm_invalidate_nested_addr_range(struct guest_info * info, #endif switch(mode) { + // dead code except for LONG and PROTECTED cases + // this is kept for clarity and parallelism with other implementations case REAL: case PROTECTED: return invalidate_addr_32_range(info, inv_addr_start, inv_addr_end, actual_start, actual_end); diff --git a/palacios/src/palacios/vmm_cpu_mapper.c b/palacios/src/palacios/vmm_cpu_mapper.c index 626f50f..2c7fe89 100644 --- a/palacios/src/palacios/vmm_cpu_mapper.c +++ b/palacios/src/palacios/vmm_cpu_mapper.c @@ -224,7 +224,7 @@ int default_mapper_admit(struct v3_vm_info *vm, unsigned int cpu_mask){ vcore_id--; } - if (vcore_id >= 0) { + if (vcore_id >= 0) { // dead code... v3_stop_vm(vm); return -1; } diff --git a/palacios/src/palacios/vmm_events.c b/palacios/src/palacios/vmm_events.c index c4580ee..7402959 100644 --- a/palacios/src/palacios/vmm_events.c +++ b/palacios/src/palacios/vmm_events.c @@ -35,6 +35,7 @@ int v3_init_events(struct v3_vm_info * vm) { return -1; } + // dead code if there are no events, but this is correct for (i = 0; i < V3_EVENT_INVALID; i++) { INIT_LIST_HEAD(&(map->events[i])); } @@ -46,6 +47,8 @@ int v3_deinit_events(struct v3_vm_info * vm) { struct v3_event_map * map = &(vm->event_map); int i = 0; + + // dead code if there are no events, but this is correct for (i = 0; i < V3_EVENT_INVALID; i++) { if (!list_empty(&(map->events[i]))) { struct v3_notifier * tmp_notifier = NULL; diff --git a/palacios/src/palacios/vmm_paging.c b/palacios/src/palacios/vmm_paging.c index 85c0874..03a158a 100644 --- a/palacios/src/palacios/vmm_paging.c +++ b/palacios/src/palacios/vmm_paging.c @@ -792,7 +792,7 @@ int v3_drill_host_pt_32(struct guest_info * info, v3_reg_t host_cr3, addr_t vadd case PT_ENTRY_NOT_PRESENT: return -1; case PT_ENTRY_LARGE_PAGE: - if ((ret == callback(info, PAGE_4MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_4MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_4MB; } return 0; @@ -842,7 +842,7 @@ int v3_drill_host_pt_32pae(struct guest_info * info, v3_reg_t host_cr3, addr_t v case PT_ENTRY_NOT_PRESENT: return -1; case PT_ENTRY_LARGE_PAGE: - if ((ret == callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_2MB; } return 0; @@ -897,7 +897,7 @@ int v3_drill_host_pt_64(struct guest_info * info, v3_reg_t host_cr3, addr_t vadd case PT_ENTRY_NOT_PRESENT: return -1; case PT_ENTRY_LARGE_PAGE: - if ((ret == callback(info, PAGE_1GB, vaddr, (addr_t)V3_VAddr((void *)host_pde_pa), host_pde_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_1GB, vaddr, (addr_t)V3_VAddr((void *)host_pde_pa), host_pde_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_1GB; } PrintError(info->vm_info, info, "1 Gigabyte Pages not supported\n"); @@ -912,7 +912,7 @@ int v3_drill_host_pt_64(struct guest_info * info, v3_reg_t host_cr3, addr_t vadd case PT_ENTRY_NOT_PRESENT: return -1; case PT_ENTRY_LARGE_PAGE: - if ((ret == callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_2MB, vaddr, (addr_t)V3_VAddr((void *)host_pte_pa), host_pte_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_2MB; } return 0; @@ -977,7 +977,7 @@ int v3_drill_guest_pt_32(struct guest_info * info, v3_reg_t guest_cr3, addr_t va } - if ((ret == callback(info, PAGE_4MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_4MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_4MB; } return 0; @@ -1071,7 +1071,7 @@ int v3_drill_guest_pt_32pae(struct guest_info * info, v3_reg_t guest_cr3, addr_t large_page_va = 0; } - if ((ret == callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_2MB; } return 0; @@ -1166,7 +1166,7 @@ int v3_drill_guest_pt_64(struct guest_info * info, v3_reg_t guest_cr3, addr_t va large_page_va = 0; } - if ((ret == callback(info, PAGE_1GB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_1GB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_1GB; } PrintError(info->vm_info, info, "1 Gigabyte Pages not supported\n"); @@ -1199,7 +1199,7 @@ int v3_drill_guest_pt_64(struct guest_info * info, v3_reg_t guest_cr3, addr_t va large_page_va = 0; } - if ((ret == callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { + if ((ret = callback(info, PAGE_2MB, vaddr, large_page_va, large_page_pa, private_data)) != 0) { return (ret == -1) ? -1 : PAGE_2MB; } return 0; diff --git a/palacios/src/palacios/vmm_swapping.c b/palacios/src/palacios/vmm_swapping.c index e3095d0..a5e806b 100644 --- a/palacios/src/palacios/vmm_swapping.c +++ b/palacios/src/palacios/vmm_swapping.c @@ -163,7 +163,7 @@ int v3_init_swapping_vm(struct v3_vm_info *vm, struct v3_xml *config) // Can we allocate the file? - if ((vm->swap_state.swapfd = v3_file_open(vm,file, FILE_OPEN_MODE_READ | FILE_OPEN_MODE_WRITE | FILE_OPEN_MODE_CREATE))<0) { + if (!(vm->swap_state.swapfd = v3_file_open(vm,file, FILE_OPEN_MODE_READ | FILE_OPEN_MODE_WRITE | FILE_OPEN_MODE_CREATE))) { PrintError(vm,VCORE_NONE,"swapper: cannot open or create swap file\n"); return -1; } else { @@ -196,7 +196,7 @@ int v3_init_swapping_vm(struct v3_vm_info *vm, struct v3_xml *config) !strcasecmp(strategy,"next_fit") ? V3_SWAP_NEXT_FIT : !strcasecmp(strategy,"random") ? V3_SWAP_RANDOM : !strcasecmp(strategy,"lru") ? V3_SWAP_LRU : - !strcasecmp(strategy,"default") ? V3_SWAP_RANDOM : + !strcasecmp(strategy,"default") ? V3_SWAP_RANDOM : // identical branches for clarity V3_SWAP_RANDOM; vm->swap_state.host_mem_size=alloc;