Messages in this thread | | | Subject | Re: [PATCH RFC 03/39] KVM: x86/xen: register shared_info page | From | Ankur Arora <> | Date | Tue, 1 Dec 2020 16:40:07 -0800 |
| |
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
> >
| |