On 2020-12-01 5:07 a.m., David Woodhouse wrote:
On Wed, 2019-02-20 at 20:15 +0000, Joao Martins wrote:+static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn) +{ + struct shared_info *shared_info; + struct page *page; + + page = gfn_to_page(kvm, gfn); + if (is_error_page(page)) + return -EINVAL; + + kvm->arch.xen.shinfo_addr = gfn; + + shared_info = page_to_virt(page); + memset(shared_info, 0, sizeof(struct shared_info)); + kvm->arch.xen.shinfo = shared_info; + return 0; +} +Hm. How come we get to pin the page and directly dereference it every time, while kvm_setup_pvclock_page() has to use kvm_write_guest_cached() instead?
So looking at my WIP trees from the time, this is something that we went back and forth on as well with using just a pinned page or a persistent kvm_vcpu_map(). I remember distinguishing shared_info/vcpu_info from kvm_setup_pvclock_page() as shared_info is created early and is not expected to change during the lifetime of the guest which didn't seem true for MSR_KVM_SYSTEM_TIME (or MSR_KVM_STEAL_TIME) so that would either need to do a kvm_vcpu_map() kvm_vcpu_unmap() dance or do some kind of synchronization. That said, I don't think this code explicitly disallows any updates to shared_info.
If that was allowed, wouldn't it have been a much simpler fix for CVE-2019-3016? What am I missing?
Agreed. Perhaps, Paolo can chime in with why KVM never uses pinned page and always prefers to do cached mappings instead?
Should I rework these to use kvm_write_guest_cached()?
kvm_vcpu_map() would be better. The event channel logic does RMW operations on shared_info->vcpu_info. Ankur
