diff --git a/shim/include/handle_vm_kvm_set_user_memory_region.h b/shim/include/handle_vm_kvm_set_user_memory_region.h index aae722c3b..4d501e20f 100644 --- a/shim/include/handle_vm_kvm_set_user_memory_region.h +++ b/shim/include/handle_vm_kvm_set_user_memory_region.h @@ -35,11 +35,6 @@ extern "C" { #endif -/** @brief TBD */ -#define KVM_MEM_LOG_DIRTY_PAGES (((uint64_t)1) << ((uint64_t)0)) -/** @brief allows a slot to be read-only */ -#define KVM_MEM_READONLY (((uint64_t)1) << ((uint64_t)1)) - /** * * @brief Handles the execution of kvm_set_user_memory_region. diff --git a/shim/include/kvm_userspace_memory_region.h b/shim/include/kvm_userspace_memory_region.h index 2a29a446b..f4da48e6c 100644 --- a/shim/include/kvm_userspace_memory_region.h +++ b/shim/include/kvm_userspace_memory_region.h @@ -36,6 +36,9 @@ extern "C" #pragma pack(push, 1) +#define KVM_MEM_LOG_DIRTY_PAGES (1UL << 0) +#define KVM_MEM_READONLY (1UL << 1) + /** * @struct kvm_userspace_memory_region * diff --git a/shim/integration/kvm_get_msr_index_list.cpp b/shim/integration/kvm_get_msr_index_list.cpp index 723167779..491ab2f39 100644 --- a/shim/integration/kvm_get_msr_index_list.cpp +++ b/shim/integration/kvm_get_msr_index_list.cpp @@ -63,6 +63,18 @@ main() noexcept -> bsl::exit_code mut_system_ctl.write(shim::KVM_GET_MSR_INDEX_LIST, &mut_msr_list).is_neg()); } + // nmsrs is too small + { + mut_msr_list.nmsrs = bsl::safe_u32::magic_0().get(); + mut_msr_list.indices = mut_msr_indices.front_if(); + + integration::verify( + mut_system_ctl.write(shim::KVM_GET_MSR_INDEX_LIST, &mut_msr_list).is_neg()); + + auto mut_nmsrs{bsl::to_u32(mut_msr_list.nmsrs)}; + integration::verify(mut_nmsrs > bsl::safe_u32::magic_0()); + } + { mut_msr_list.nmsrs = init_nmsrs.get(); mut_msr_list.indices = mut_msr_indices.front_if(); @@ -100,6 +112,9 @@ main() noexcept -> bsl::exit_code // Try a bunch of times { + mut_msr_list.nmsrs = init_nmsrs.get(); + mut_msr_list.indices = mut_msr_indices.front_if(); + constexpr auto num_loops{0x1000_umx}; for (bsl::safe_idx mut_i{}; mut_i < num_loops; ++mut_i) { integration::verify( diff --git a/shim/integration/kvm_set_user_memory_region.cpp b/shim/integration/kvm_set_user_memory_region.cpp index b6954c9d9..d8f61311d 100644 --- a/shim/integration/kvm_set_user_memory_region.cpp +++ b/shim/integration/kvm_set_user_memory_region.cpp @@ -201,6 +201,84 @@ main() noexcept -> bsl::exit_code mut_vm.close(); } + // non-canonical address below the canonical boundary + { + auto const vmfd{mut_system_ctl.send(shim::KVM_CREATE_VM)}; + integration::ioctl_t mut_vm{bsl::to_i32(vmfd)}; + constexpr auto non_canonical_address{0xFFFF7FFFFFFFFFFF_u64}; + shim::kvm_userspace_memory_region mut_region{}; + mut_region.slot = {}; + mut_region.flags = {}; + mut_region.guest_phys_addr = {}; + mut_region.memory_size = vm_image.size().get(); + mut_region.userspace_addr = reinterpret_cast(non_canonical_address.get()); + + auto const ret{mut_vm.write(shim::KVM_SET_USER_MEMORY_REGION, &mut_region)}; + integration::verify(ret.is_neg()); + + mut_vm.close(); + } + + // non-canonical address above the canonical boundary + { + auto const vmfd{mut_system_ctl.send(shim::KVM_CREATE_VM)}; + integration::ioctl_t mut_vm{bsl::to_i32(vmfd)}; + constexpr auto non_canonical_address{0x0008000000000000_u64}; + shim::kvm_userspace_memory_region mut_region{}; + mut_region.slot = {}; + mut_region.flags = {}; + mut_region.guest_phys_addr = {}; + mut_region.memory_size = vm_image.size().get(); + mut_region.userspace_addr = reinterpret_cast(non_canonical_address.get()); + + auto const ret{mut_vm.write(shim::KVM_SET_USER_MEMORY_REGION, &mut_region)}; + integration::verify(ret.is_neg()); + + mut_vm.close(); + } + + // canonical address on the higher canonical boundary + { + auto const vmfd{mut_system_ctl.send(shim::KVM_CREATE_VM)}; + integration::ioctl_t mut_vm{bsl::to_i32(vmfd)}; + constexpr auto non_canonical_address{0xFFFF800000000000_u64}; + shim::kvm_userspace_memory_region mut_region{}; + mut_region.slot = {}; + mut_region.flags = {}; + mut_region.guest_phys_addr = {}; + mut_region.memory_size = vm_image.size().get(); + mut_region.userspace_addr = reinterpret_cast(non_canonical_address.get()); + + auto const ret{mut_vm.write(shim::KVM_SET_USER_MEMORY_REGION, &mut_region)}; + // This fails on platform_virt_to_phys, but none of the error paths distinguish + // between error type. This test should verify ret.is_zero(). Might have to add + // flags for the shim to short circuit. + integration::verify(ret.is_neg()); + + mut_vm.close(); + } + + // canonical address on the lower canonical boundary + { + auto const vmfd{mut_system_ctl.send(shim::KVM_CREATE_VM)}; + integration::ioctl_t mut_vm{bsl::to_i32(vmfd)}; + constexpr auto non_canonical_address{0x0007FFFFFFFFFFFF_u64}; + shim::kvm_userspace_memory_region mut_region{}; + mut_region.slot = {}; + mut_region.flags = {}; + mut_region.guest_phys_addr = {}; + mut_region.memory_size = vm_image.size().get(); + mut_region.userspace_addr = reinterpret_cast(non_canonical_address.get()); + + auto const ret{mut_vm.write(shim::KVM_SET_USER_MEMORY_REGION, &mut_region)}; + // This fails on platform_virt_to_phys, but none of the error paths distinguish + // between error type. This test should verify ret.is_zero(). Might have to add + // flags for the shim to short circuit. + integration::verify(ret.is_neg()); + + mut_vm.close(); + } + // The shim has a limited number of slots { auto const vmfd{mut_system_ctl.send(shim::KVM_CREATE_VM)}; diff --git a/shim/linux/include/platform_interface/shim_platform_interface.h b/shim/linux/include/platform_interface/shim_platform_interface.h index 03e73db7b..163dc2c68 100644 --- a/shim/linux/include/platform_interface/shim_platform_interface.h +++ b/shim/linux/include/platform_interface/shim_platform_interface.h @@ -78,12 +78,23 @@ /** @brief defines the /dev name of the shim */ #define SHIM_DEVICE_NAME "/dev/microv_shim" +/** + * @brief Hack for defining ioctl commands that require structs + * with zero-length arrays. This is usually for ioctls that return + * a list. + * + * It is just like _IOWR, except it subtracts the size of a pointer + * from the size of the struct passed. + */ +#define _IOWR_LIST(type, nr, size) \ + _IOC(_IOC_READ | _IOC_WRITE, (type), (nr), sizeof(size) - sizeof(void *)) + /** @brief defines KVM's KVM_GET_API_VERSION IOCTL */ #define KVM_GET_API_VERSION _IO(SHIMIO, 0x00) /** @brief defines KVM's KVM_CREATE_VM IOCTL */ #define KVM_CREATE_VM _IO(SHIMIO, 0x01) /** @brief defines KVM's KVM_GET_MSR_INDEX_LIST IOCTL */ -#define KVM_GET_MSR_INDEX_LIST _IOWR(SHIMIO, 0x02, struct kvm_msr_list) +#define KVM_GET_MSR_INDEX_LIST _IOWR_LIST(SHIMIO, 0x02, struct kvm_msr_list) /** @brief defines KVM's KVM_GET_MSR_FEATURE_INDEX_LIST IOCTL */ #define KVM_GET_MSR_FEATURE_INDEX_LIST _IOWR(SHIMIO, 0x0a, struct kvm_msr_list) /** @brief defines KVM's KVM_CHECK_EXTENSION IOCTL */ diff --git a/shim/linux/include/platform_interface/shim_platform_interface.hpp b/shim/linux/include/platform_interface/shim_platform_interface.hpp index 74321c259..219b0a01e 100644 --- a/shim/linux/include/platform_interface/shim_platform_interface.hpp +++ b/shim/linux/include/platform_interface/shim_platform_interface.hpp @@ -79,6 +79,17 @@ // #include // #include +/** + * @brief Hack for defining ioctl commands that require structs + * with zero-length arrays. This is usually for ioctls that return + * a list. + * + * It is just like _IOWR, except it subtracts the size of a pointer + * from the size of the struct passed. + */ +#define _IOWR_LIST(type, nr, size) \ + _IOC(_IOC_READ | _IOC_WRITE, (type), (nr), sizeof(size) - sizeof(void *)) + namespace shim { /// @brief magic number for KVM IOCTLs @@ -95,7 +106,7 @@ namespace shim constexpr bsl::safe_umx KVM_CREATE_VM{static_cast(_IO(SHIMIO.get(), 0x01))}; /// @brief defines KVM's KVM_GET_MSR_INDEX_LIST IOCTL constexpr bsl::safe_umx KVM_GET_MSR_INDEX_LIST{ - static_cast(_IOWR(SHIMIO.get(), 0x02, struct kvm_msr_list))}; + static_cast(_IOWR_LIST(SHIMIO.get(), 0x02, struct kvm_msr_list))}; // /// @brief defines KVM's KVM_GET_MSR_FEATURE_INDEX_LIST IOCTL // constexpr bsl::safe_umx KVM_GET_MSR_FEATURE_INDEX_LIST{static_cast(_IOWR(SHIMIO.get(), 0x0a, struct kvm_msr_list))}; /// @brief defines KVM's KVM_CHECK_EXTENSION IOCTL diff --git a/shim/linux/include/types.h b/shim/linux/include/types.h index ea5b19990..92fc2dd4f 100644 --- a/shim/linux/include/types.h +++ b/shim/linux/include/types.h @@ -50,4 +50,9 @@ */ #define SHIM_INTERRUPTED ((int64_t)-EINTR) +/** + * @brief Returned by a shim function when a provided data structure + * is not large enough to hold the return data. + */ +#define SHIM_2BIG ((int64_t)-E2BIG) #endif diff --git a/shim/linux/src/entry.c b/shim/linux/src/entry.c index a848e23b2..260e89cb8 100644 --- a/shim/linux/src/entry.c +++ b/shim/linux/src/entry.c @@ -233,7 +233,6 @@ dispatch_system_kvm_get_msr_index_list( struct kvm_msr_list __user *const user_args) { struct kvm_msr_list mut_args; - uint32_t __user *pmut_mut_user_indices; int64_t mut_ret; uint64_t mut_alloc_size; @@ -242,35 +241,44 @@ dispatch_system_kvm_get_msr_index_list( return -EINVAL; } - mut_alloc_size = mut_args.nmsrs * sizeof(*mut_args.indices); + if (mut_args.nmsrs > 0) { + mut_alloc_size = mut_args.nmsrs * sizeof(*mut_args.indices); + } + else { + mut_alloc_size = sizeof(*mut_args.indices); + } + if (mut_alloc_size > HYPERVISOR_PAGE_SIZE) { bferror("requested nmsrs too big"); return -ENOMEM; } - pmut_mut_user_indices = mut_args.indices; mut_args.indices = vzalloc(mut_alloc_size); - if (NULL == mut_args.indices) { bferror("vzalloc failed"); return -ENOMEM; } - mut_ret = -EINVAL; - if (handle_system_kvm_get_msr_index_list(&mut_args)) { - bferror("handle_system_kvm_get_msr_index_list failed"); + mut_ret = handle_system_kvm_get_msr_index_list(&mut_args); + if (SHIM_2BIG == mut_ret) { + if (platform_copy_to_user( + user_args, &mut_args, sizeof(mut_args.nmsrs))) { + bferror("platform_copy_to_user nmsrs failed"); + mut_ret = -EINVAL; + } + goto out; } - - if (platform_copy_to_user(user_args, &mut_args, sizeof(mut_args.nmsrs))) { - bferror("platform_copy_to_user nmsrs failed"); + else if (mut_ret) { + bferror("handle_system_kvm_get_msr_index_list failed"); goto out; } if (platform_copy_to_user( - pmut_mut_user_indices, - mut_args.indices, - mut_args.nmsrs * sizeof(*mut_args.indices))) { + user_args, + &mut_args, + sizeof(mut_args.nmsrs) + + mut_args.nmsrs * sizeof(*mut_args.indices))) { bferror("platform_copy_to_user indices failed"); goto out; } diff --git a/shim/src/handle_system_kvm_get_msr_index_list.c b/shim/src/handle_system_kvm_get_msr_index_list.c index 00df95a34..acf662478 100644 --- a/shim/src/handle_system_kvm_get_msr_index_list.c +++ b/shim/src/handle_system_kvm_get_msr_index_list.c @@ -41,11 +41,14 @@ * * * @param pmut_ioctl_args the arguments provided by userspace - * @return SHIM_SUCCESS on success, SHIM_FAILURE on failure. + * @return SHIM_SUCCESS on success, SHIM_FAILURE on failure, and SHIM_2BIG when + * the number of MSRs is greater than what was set in nmsrs. When SHIM_2BIG is + * returned, the correct number of MSRs is set in the nmsrs field. */ NODISCARD int64_t handle_system_kvm_get_msr_index_list(struct kvm_msr_list *const pmut_ioctl_args) NOEXCEPT { + int64_t mut_ret; int64_t mut_i; uint32_t mut_nmsrs = ((uint32_t)0); @@ -78,18 +81,29 @@ handle_system_kvm_get_msr_index_list(struct kvm_msr_list *const pmut_ioctl_args) return SHIM_FAILURE; } + /** + * If the provided buffer is not large enough to fit all the MSRs, we still + * need to calculate the total number, and set the nmsrs field correctly. + */ if (pmut_rdl->num_entries + ((uint64_t)mut_nmsrs) > ((uint64_t)pmut_ioctl_args->nmsrs)) { - bferror("number of MSRs is larger than kvm_msr_list indices"); - return SHIM_FAILURE; + mut_nmsrs += pmut_rdl->num_entries; } - - for (mut_i = ((int64_t)0); mut_i < ((int64_t)pmut_rdl->num_entries); ++mut_i) { - pmut_ioctl_args->indices[mut_nmsrs] = ((uint32_t)pmut_rdl->entries[mut_i].reg); - ++mut_nmsrs; + else { + for (mut_i = ((int64_t)0); mut_i < ((int64_t)pmut_rdl->num_entries); ++mut_i) { + pmut_ioctl_args->indices[mut_nmsrs] = ((uint32_t)pmut_rdl->entries[mut_i].reg); + ++mut_nmsrs; + } } } while (((uint64_t)0) != pmut_rdl->reg1); + if (mut_nmsrs > pmut_ioctl_args->nmsrs) { + mut_ret = SHIM_2BIG; + } + else { + mut_ret = SHIM_SUCCESS; + } + pmut_ioctl_args->nmsrs = mut_nmsrs; - return SHIM_SUCCESS; + return mut_ret; } diff --git a/shim/src/handle_vm_kvm_set_user_memory_region.c b/shim/src/handle_vm_kvm_set_user_memory_region.c index b96fcbf11..bdf525212 100644 --- a/shim/src/handle_vm_kvm_set_user_memory_region.c +++ b/shim/src/handle_vm_kvm_set_user_memory_region.c @@ -70,6 +70,30 @@ get_slot_as(uint32_t const slot) NOEXCEPT return slot & as_mask; } +/** + * + * @brief Translates KVM_MEM_* flags to MV_MAP_FLAG* flags + * + * + * @param flags KVM memory flag in the set_user_memory_region + * @return Returns the microv equivalent flag for the kvm flag + */ +NODISCARD static inline uint64_t +kvm_to_mv_page_flags(uint32_t const flags) NOEXCEPT +{ + uint64_t mut_flags = ((uint64_t)0); + if (flags & ((uint32_t)KVM_MEM_READONLY)) { + // Anticipate that we'll have to provide RE access here + // in reality + mut_flags |= MV_MAP_FLAG_READ_ACCESS; + } + else { + mv_touch(); + } + + return mut_flags; +} + /** * * @brief Handles the execution of kvm_set_user_memory_region. @@ -83,6 +107,8 @@ NODISCARD int64_t handle_vm_kvm_set_user_memory_region( struct kvm_userspace_memory_region const *const args, struct shim_vm_t *const pmut_vm) NOEXCEPT { + uint64_t const high_canonical_boundary = ((uint64_t)0xFFFF800000000000ULL); + uint64_t const low_canonical_boundary = ((uint64_t)0x00007FFFFFFFFFFFULL); struct mv_mdl_t *pmut_mut_mdl; int64_t mut_i; @@ -92,6 +118,7 @@ handle_vm_kvm_set_user_memory_region( uint32_t mut_slot_as; uint64_t mut_dst; uint64_t mut_src; + uint64_t const flags = kvm_to_mv_page_flags(args->flags); platform_expects(NULL != args); platform_expects(NULL != pmut_vm); @@ -124,7 +151,7 @@ handle_vm_kvm_set_user_memory_region( } if (((uint64_t)0) == args->memory_size) { - bferror("deleting an existing slot is currently not implemeneted"); + bferror("deleting an existing slot is currently not implemented"); return SHIM_FAILURE; } @@ -148,18 +175,26 @@ handle_vm_kvm_set_user_memory_region( return SHIM_FAILURE; } - /// TODO: - /// - Check to make sure that the userspace address that was provided - /// is canonical. Otherwise MicroV will get mad. - /// + if (high_canonical_boundary < args->userspace_addr) { + if (low_canonical_boundary <= args->userspace_addr) { + mv_touch(); + } + else { + bferror("args->userspace_addr is not a canonical address"); + return SHIM_FAILURE; + } + } + else { + mv_touch(); + } - /// TODO: - /// - Check to make sure that the provided flags are supported by MicroV - /// and then construct the MicroV flags as required. - /// + // Don't support KVM_MEM_LOG_DIRTY_PAGES right now + if (args->flags & ~((uint32_t)KVM_MEM_READONLY)) { + return SHIM_FAILURE; + } /// TODO: - /// - Check to make sure that non of the slots overlap. This is not + /// - Check to make sure that none of the slots overlap. This is not /// allowed by the KVM API, and even if it were, MicroV would get /// mad as it doesn't allow this either. /// @@ -231,15 +266,9 @@ handle_vm_kvm_set_user_memory_region( pmut_mut_mdl->entries[pmut_mut_mdl->num_entries].dst = dst; pmut_mut_mdl->entries[pmut_mut_mdl->num_entries].src = src; pmut_mut_mdl->entries[pmut_mut_mdl->num_entries].bytes = HYPERVISOR_PAGE_SIZE; + pmut_mut_mdl->entries[pmut_mut_mdl->num_entries].flags = flags; ++pmut_mut_mdl->num_entries; - /// TODO: - /// - Need to add support for memory flags. Right now, MicroV ignores - /// the flags field and always sets the memory to RWE. This needs - /// to be fixed, and then we will need to translate the KVM flags - /// to MicroV flags here and send them up properly. - /// - /// TODO: /// - Right now MicroV assumes that every entry is 4k in size. /// Instead, it should be modified to handle any page aligned diff --git a/shim/tests/include/types.h b/shim/tests/include/types.h index 138613e9c..128331921 100644 --- a/shim/tests/include/types.h +++ b/shim/tests/include/types.h @@ -49,4 +49,10 @@ */ #define SHIM_INTERRUPTED ((int64_t)-2) +/** + * @brief Returned by a shim function when a provided data structure + * is not large enough to hold the return data. + */ +#define SHIM_2BIG ((int64_t)-3) + #endif diff --git a/shim/tests/src/test_handle_system_kvm_get_msr_index_list.cpp b/shim/tests/src/test_handle_system_kvm_get_msr_index_list.cpp index cffb43e2c..92e6583bf 100644 --- a/shim/tests/src/test_handle_system_kvm_get_msr_index_list.cpp +++ b/shim/tests/src/test_handle_system_kvm_get_msr_index_list.cpp @@ -169,7 +169,7 @@ namespace shim mut_args.indices = g_mut_msr_indices.front_if(); g_mut_val = VAL64.get(); bsl::ut_then{} = [&]() noexcept { - bsl::ut_check(SHIM_FAILURE == handle(&mut_args)); + bsl::ut_check(SHIM_2BIG == handle(&mut_args)); }; bsl::ut_cleanup{} = [&]() noexcept { g_mut_val = {}; diff --git a/vmm/src/x64/emulated_mmio_t.hpp b/vmm/src/x64/emulated_mmio_t.hpp index 4025aa44c..5126a0670 100644 --- a/vmm/src/x64/emulated_mmio_t.hpp +++ b/vmm/src/x64/emulated_mmio_t.hpp @@ -247,7 +247,7 @@ namespace microv auto const gpa{bsl::to_u64(entry->dst)}; auto const spa{this->gpa_to_spa(mut_sys, bsl::to_u64(entry->src))}; - + auto const flags{bsl::to_u64(entry->flags)}; /// TODO: /// - Add support for entries that have a size greater than /// 4k. For now we only support 4k pages. @@ -258,8 +258,7 @@ namespace microv /// because guest software will not attempt to undo a /// failed map operation. /// - auto const ret{ - m_slpt.map(tls, mut_page_pool, gpa, spa, MAP_PAGE_RWE, false, mut_sys)}; + auto const ret{m_slpt.map(tls, mut_page_pool, gpa, spa, flags, false, mut_sys)}; if (bsl::unlikely(ret == bsl::errc_already_exists)) { bsl::error() << "mdl entry " // -- diff --git a/vmm/src/x64/intel/dispatch_vmexit_io.hpp b/vmm/src/x64/intel/dispatch_vmexit_io.hpp index fdaa2dd4e..d04bcd11b 100644 --- a/vmm/src/x64/intel/dispatch_vmexit_io.hpp +++ b/vmm/src/x64/intel/dispatch_vmexit_io.hpp @@ -73,11 +73,6 @@ namespace microv vs_pool_t &mut_vs_pool, bsl::safe_u16 const &vsid) noexcept -> bsl::errc_type { - /// TODO: - /// - Need to properly handle string instructions (INS/OUTS) - /// - Need to properly handle IN instructions - /// - bsl::expects(!mut_sys.is_the_active_vm_the_root_vm()); bsl::discard(gs); @@ -131,8 +126,7 @@ namespace microv mut_exit_io->type = hypercall::MV_EXIT_IO_OUT.get(); } else { - bsl::error() << "MV_EXIT_IO_IN not implemented\n" << bsl::here(); - return bsl::errc_failure; + mut_exit_io->type = hypercall::MV_EXIT_IO_IN.get(); } constexpr auto bytes1{0_u64};