diff options
author | Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> | 2013-01-30 05:40:41 -0500 |
---|---|---|
committer | Marcelo Tosatti <mtosatti@redhat.com> | 2013-02-04 19:56:47 -0500 |
commit | 75d61fbcf563373696578570e914f555e12c8d97 (patch) | |
tree | 8835d8a5cf79704569f568675792672c930746ae /virt/kvm/kvm_main.c | |
parent | f64c0398939483eb1d8951f24fbc21e94ed54457 (diff) |
KVM: set_memory_region: Disallow changing read-only attribute later
As Xiao pointed out, there are a few problems with it:
- kvm_arch_commit_memory_region() write protects the memory slot only
for GET_DIRTY_LOG when modifying the flags.
- FNAME(sync_page) uses the old spte value to set a new one without
checking KVM_MEM_READONLY flag.
Since we flush all shadow pages when creating a new slot, the simplest
fix is to disallow such problematic flag changes: this is safe because
no one is doing such things.
Reviewed-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Diffstat (limited to 'virt/kvm/kvm_main.c')
-rw-r--r-- | virt/kvm/kvm_main.c | 35 |
1 files changed, 12 insertions, 23 deletions
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 64c5dc37c6a1..2e93630b4add 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c | |||
@@ -754,7 +754,6 @@ int __kvm_set_memory_region(struct kvm *kvm, | |||
754 | struct kvm_memory_slot *slot; | 754 | struct kvm_memory_slot *slot; |
755 | struct kvm_memory_slot old, new; | 755 | struct kvm_memory_slot old, new; |
756 | struct kvm_memslots *slots = NULL, *old_memslots; | 756 | struct kvm_memslots *slots = NULL, *old_memslots; |
757 | bool old_iommu_mapped; | ||
758 | enum kvm_mr_change change; | 757 | enum kvm_mr_change change; |
759 | 758 | ||
760 | r = check_memory_region_flags(mem); | 759 | r = check_memory_region_flags(mem); |
@@ -797,15 +796,14 @@ int __kvm_set_memory_region(struct kvm *kvm, | |||
797 | new.npages = npages; | 796 | new.npages = npages; |
798 | new.flags = mem->flags; | 797 | new.flags = mem->flags; |
799 | 798 | ||
800 | old_iommu_mapped = old.npages; | ||
801 | |||
802 | r = -EINVAL; | 799 | r = -EINVAL; |
803 | if (npages) { | 800 | if (npages) { |
804 | if (!old.npages) | 801 | if (!old.npages) |
805 | change = KVM_MR_CREATE; | 802 | change = KVM_MR_CREATE; |
806 | else { /* Modify an existing slot. */ | 803 | else { /* Modify an existing slot. */ |
807 | if ((mem->userspace_addr != old.userspace_addr) || | 804 | if ((mem->userspace_addr != old.userspace_addr) || |
808 | (npages != old.npages)) | 805 | (npages != old.npages) || |
806 | ((new.flags ^ old.flags) & KVM_MEM_READONLY)) | ||
809 | goto out; | 807 | goto out; |
810 | 808 | ||
811 | if (base_gfn != old.base_gfn) | 809 | if (base_gfn != old.base_gfn) |
@@ -867,7 +865,6 @@ int __kvm_set_memory_region(struct kvm *kvm, | |||
867 | 865 | ||
868 | /* slot was deleted or moved, clear iommu mapping */ | 866 | /* slot was deleted or moved, clear iommu mapping */ |
869 | kvm_iommu_unmap_pages(kvm, &old); | 867 | kvm_iommu_unmap_pages(kvm, &old); |
870 | old_iommu_mapped = false; | ||
871 | /* From this point no new shadow pages pointing to a deleted, | 868 | /* From this point no new shadow pages pointing to a deleted, |
872 | * or moved, memslot will be created. | 869 | * or moved, memslot will be created. |
873 | * | 870 | * |
@@ -898,25 +895,17 @@ int __kvm_set_memory_region(struct kvm *kvm, | |||
898 | 895 | ||
899 | /* | 896 | /* |
900 | * IOMMU mapping: New slots need to be mapped. Old slots need to be | 897 | * IOMMU mapping: New slots need to be mapped. Old slots need to be |
901 | * un-mapped and re-mapped if their base changes or if flags that the | 898 | * un-mapped and re-mapped if their base changes. Since base change |
902 | * iommu cares about change (read-only). Base change unmapping is | 899 | * unmapping is handled above with slot deletion, mapping alone is |
903 | * handled above with slot deletion, so we only unmap incompatible | 900 | * needed here. Anything else the iommu might care about for existing |
904 | * flags here. Anything else the iommu might care about for existing | 901 | * slots (size changes, userspace addr changes and read-only flag |
905 | * slots (size changes, userspace addr changes) is disallowed above, | 902 | * changes) is disallowed above, so any other attribute changes getting |
906 | * so any other attribute changes getting here can be skipped. | 903 | * here can be skipped. |
907 | */ | 904 | */ |
908 | if (change != KVM_MR_DELETE) { | 905 | if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) { |
909 | if (old_iommu_mapped && | 906 | r = kvm_iommu_map_pages(kvm, &new); |
910 | ((new.flags ^ old.flags) & KVM_MEM_READONLY)) { | 907 | if (r) |
911 | kvm_iommu_unmap_pages(kvm, &old); | 908 | goto out_slots; |
912 | old_iommu_mapped = false; | ||
913 | } | ||
914 | |||
915 | if (!old_iommu_mapped) { | ||
916 | r = kvm_iommu_map_pages(kvm, &new); | ||
917 | if (r) | ||
918 | goto out_slots; | ||
919 | } | ||
920 | } | 909 | } |
921 | 910 | ||
922 | /* actual memory is freed via old in kvm_free_physmem_slot below */ | 911 | /* actual memory is freed via old in kvm_free_physmem_slot below */ |