From 8c1a8a32438b95792bbd8719d1cd4fe36e9eba03 Mon Sep 17 00:00:00 2001 From: Christoffer Dall Date: Fri, 13 Oct 2017 11:40:11 +0200 Subject: KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table We currently allocate an entry dynamically, but we never check if the allocation actually succeeded. We actually don't need a dynamic allocation, because we know the maximum size of an ITS table entry, so we can simply use an allocation on the stack. Cc: Signed-off-by: Christoffer Dall --- virt/kvm/arm/vgic/vgic-its.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) (limited to 'virt') diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index f51c1e1b3f70..77652885a7c1 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1801,37 +1801,33 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry, static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz, int start_id, entry_fn_t fn, void *opaque) { - void *entry = kzalloc(esz, GFP_KERNEL); struct kvm *kvm = its->dev->kvm; unsigned long len = size; int id = start_id; gpa_t gpa = base; + char entry[esz]; int ret; + memset(entry, 0, esz); + while (len > 0) { int next_offset; size_t byte_offset; ret = kvm_read_guest(kvm, gpa, entry, esz); if (ret) - goto out; + return ret; next_offset = fn(its, id, entry, opaque); - if (next_offset <= 0) { - ret = next_offset; - goto out; - } + if (next_offset <= 0) + return next_offset; byte_offset = next_offset * esz; id += next_offset; gpa += byte_offset; len -= byte_offset; } - ret = 1; - -out: - kfree(entry); - return ret; + return 1; } /** -- cgit v1.2.3 From fe7d7b03c61f6dab70a973fb32c90f2254784e03 Mon Sep 17 00:00:00 2001 From: Julien Thierry Date: Fri, 20 Oct 2017 12:34:16 +0100 Subject: arm/arm64: kvm: Move initialization completion message KVM is being a bit too optimistic, Hyp mode is said to be initialized when Hyp segments have only been mapped. Notify KVM's successful initialization only once it is really fully initialized. Signed-off-by: Julien Thierry Acked-by: Marc Zyngier Cc: Christoffer Dall Signed-off-by: Christoffer Dall --- virt/kvm/arm/arm.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) (limited to 'virt') diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index b9f68e4add71..95cba0799828 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -1326,21 +1326,12 @@ static void teardown_hyp_mode(void) { int cpu; - if (is_kernel_in_hyp_mode()) - return; - free_hyp_pgds(); for_each_possible_cpu(cpu) free_page(per_cpu(kvm_arm_hyp_stack_page, cpu)); hyp_cpu_pm_exit(); } -static int init_vhe_mode(void) -{ - kvm_info("VHE mode initialized successfully\n"); - return 0; -} - /** * Inits Hyp-mode on all online CPUs */ @@ -1421,8 +1412,6 @@ static int init_hyp_mode(void) } } - kvm_info("Hyp mode initialized successfully\n"); - return 0; out_err: @@ -1456,6 +1445,7 @@ int kvm_arch_init(void *opaque) { int err; int ret, cpu; + bool in_hyp_mode; if (!is_hyp_mode_available()) { kvm_err("HYP mode not available\n"); @@ -1474,21 +1464,28 @@ int kvm_arch_init(void *opaque) if (err) return err; - if (is_kernel_in_hyp_mode()) - err = init_vhe_mode(); - else + in_hyp_mode = is_kernel_in_hyp_mode(); + + if (!in_hyp_mode) { err = init_hyp_mode(); - if (err) - goto out_err; + if (err) + goto out_err; + } err = init_subsystems(); if (err) goto out_hyp; + if (in_hyp_mode) + kvm_info("VHE mode initialized successfully\n"); + else + kvm_info("Hyp mode initialized successfully\n"); + return 0; out_hyp: - teardown_hyp_mode(); + if (!in_hyp_mode) + teardown_hyp_mode(); out_err: teardown_common_resources(); return err; -- cgit v1.2.3 From b92382620e33c9f1bcbcd7c169262b9bf0525871 Mon Sep 17 00:00:00 2001 From: wanghaibin Date: Thu, 26 Oct 2017 17:23:03 +0200 Subject: KVM: arm/arm64: vgic-its: Fix return value for device table restore If ITT only contains invalid entries, vgic_its_restore_itt returns 1 and this is considered as an an error in vgic_its_restore_dte. Also in case the device table only contains invalid entries, the table restore fails and this is not correct. This patch fixes those 2 issues: - vgic_its_restore_itt now returns <= 0 values. If all ITEs are invalid, this is considered as successful. - vgic_its_restore_device_tables also returns <= 0 values. We also simplify the returned value computation in handle_l1_dte. Signed-off-by: wanghaibin Signed-off-by: Eric Auger Reviewed-by: Christoffer Dall Signed-off-by: Christoffer Dall --- virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 'virt') diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 77652885a7c1..76685f4c6261 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1936,6 +1936,14 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device) return 0; } +/** + * vgic_its_restore_itt - restore the ITT of a device + * + * @its: its handle + * @dev: device handle + * + * Return 0 on success, < 0 on error + */ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev) { const struct vgic_its_abi *abi = vgic_its_get_abi(its); @@ -1947,6 +1955,10 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev) ret = scan_its_table(its, base, max_size, ite_esz, 0, vgic_its_restore_ite, dev); + /* scan_its_table returns +1 if all ITEs are invalid */ + if (ret > 0) + ret = 0; + return ret; } @@ -2103,10 +2115,7 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr, ret = scan_its_table(its, gpa, SZ_64K, dte_esz, l2_start_id, vgic_its_restore_dte, NULL); - if (ret <= 0) - return ret; - - return 1; + return ret; } /** @@ -2136,8 +2145,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its) vgic_its_restore_dte, NULL); } + /* scan_its_table returns +1 if all entries are invalid */ if (ret > 0) - ret = -EINVAL; + ret = 0; return ret; } -- cgit v1.2.3 From f31b98b57f72dcd458eb63f795f4efe272acc2e3 Mon Sep 17 00:00:00 2001 From: Eric Auger Date: Thu, 26 Oct 2017 17:23:04 +0200 Subject: KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value vgic_its_restore_cte returns +1 if the collection table entry is valid and properly decoded. As a consequence, if the collection table is fully filled with valid data that are decoded without error, vgic_its_restore_collection_table() returns +1. This is wrong. Let's return 0 in that case. Fixes: ea1ad53e1e31a3 (KVM: arm64: vgic-its: Collection table save/restore) Signed-off-by: Eric Auger Signed-off-by: Christoffer Dall --- virt/kvm/arm/vgic/vgic-its.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'virt') diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 76685f4c6261..6a715a6ec64e 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -2264,6 +2264,10 @@ static int vgic_its_restore_collection_table(struct vgic_its *its) gpa += cte_esz; read += cte_esz; } + + if (ret > 0) + return 0; + return ret; } -- cgit v1.2.3 From c9b51bb60d944067f36f67750e19c18c3cc2697c Mon Sep 17 00:00:00 2001 From: Eric Auger Date: Thu, 26 Oct 2017 17:23:05 +0200 Subject: KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS The spec says it is UNPREDICTABLE to enable the ITS if any of the following conditions are true: - GITS_CBASER.Valid == 0. - GITS_BASER.Valid == 0, for any GITS_BASER register where the Type field indicates Device. - GITS_BASER.Valid == 0, for any GITS_BASER register where the Type field indicates Interrupt Collection and GITS_TYPER.HCC == 0. In that case, let's keep the ITS disabled. Signed-off-by: Eric Auger Reported-by: Andre Przywara Reviewed-by: Christoffer Dall Signed-off-by: Christoffer Dall --- virt/kvm/arm/vgic/vgic-its.c | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'virt') diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 6a715a6ec64e..e69ef7d27fde 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1466,6 +1466,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its, { mutex_lock(&its->cmd_lock); + /* + * It is UNPREDICTABLE to enable the ITS if any of the CBASER or + * device/collection BASER are invalid + */ + if (!its->enabled && (val & GITS_CTLR_ENABLE) && + (!(its->baser_device_table & GITS_BASER_VALID) || + !(its->baser_coll_table & GITS_BASER_VALID) || + !(its->cbaser & GITS_CBASER_VALID))) + goto out; + its->enabled = !!(val & GITS_CTLR_ENABLE); /* @@ -1474,6 +1484,7 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its, */ vgic_its_process_commands(kvm, its); +out: mutex_unlock(&its->cmd_lock); } -- cgit v1.2.3 From c2385eaa6c5a87cdc4e04ed589ae103ca3297c84 Mon Sep 17 00:00:00 2001 From: Eric Auger Date: Thu, 26 Oct 2017 17:23:06 +0200 Subject: KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables At the moment we don't properly check the GITS_BASER.Valid bit before saving the collection and device tables. On vgic_its_save_collection_table() we use the GITS_BASER gpa field whereas the Valid bit should be used. On vgic_its_save_device_tables() there is no check. This can cause various bugs, among which a subsequent fault when accessing the table in guest memory. Let's systematically check the Valid bit before doing anything. We also uniformize the code between save and restore. Signed-off-by: Eric Auger Reviewed-by: Andre Przywara Reviewed-by: Christoffer Dall Reviewed-by: Marc Zyngier Signed-off-by: Christoffer Dall --- virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) (limited to 'virt') diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index e69ef7d27fde..547f12dc4d54 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -2067,11 +2067,12 @@ static int vgic_its_device_cmp(void *priv, struct list_head *a, static int vgic_its_save_device_tables(struct vgic_its *its) { const struct vgic_its_abi *abi = vgic_its_get_abi(its); + u64 baser = its->baser_device_table; struct its_device *dev; int dte_esz = abi->dte_esz; - u64 baser; - baser = its->baser_device_table; + if (!(baser & GITS_BASER_VALID)) + return 0; list_sort(NULL, &its->device_list, vgic_its_device_cmp); @@ -2215,17 +2216,17 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz) static int vgic_its_save_collection_table(struct vgic_its *its) { const struct vgic_its_abi *abi = vgic_its_get_abi(its); + u64 baser = its->baser_coll_table; + gpa_t gpa = BASER_ADDRESS(baser); struct its_collection *collection; u64 val; - gpa_t gpa; size_t max_size, filled = 0; int ret, cte_esz = abi->cte_esz; - gpa = BASER_ADDRESS(its->baser_coll_table); - if (!gpa) + if (!(baser & GITS_BASER_VALID)) return 0; - max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K; + max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K; list_for_each_entry(collection, &its->collection_list, coll_list) { ret = vgic_its_save_cte(its, collection, gpa, cte_esz); @@ -2256,17 +2257,18 @@ static int vgic_its_save_collection_table(struct vgic_its *its) static int vgic_its_restore_collection_table(struct vgic_its *its) { const struct vgic_its_abi *abi = vgic_its_get_abi(its); + u64 baser = its->baser_coll_table; int cte_esz = abi->cte_esz; size_t max_size, read = 0; gpa_t gpa; int ret; - if (!(its->baser_coll_table & GITS_BASER_VALID)) + if (!(baser & GITS_BASER_VALID)) return 0; - gpa = BASER_ADDRESS(its->baser_coll_table); + gpa = BASER_ADDRESS(baser); - max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K; + max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K; while (read < max_size) { ret = vgic_its_restore_cte(its, gpa, cte_esz); -- cgit v1.2.3