KVM: x86: relax MSR_KVM_SYSTEM_TIME alignment check

RHEL5 i386 guests register non 32-byte aligned addresses:

kvm-clock: cpu 1, msr 0:3018aa5, secondary cpu clock
kvm-clock: cpu 2, msr 0:301f8e9, secondary cpu clock
kvm-clock: cpu 3, msr 0:302672d, secondary cpu clock

Check for an address+len that would cross page boundary instead.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +0 -0

index f19ac0a..ad36d386 100644
@@ -1952,8 +1952,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 

Quoted 7 lines · Marcelo Tosatti

RHEL5 i386 guests register non 32-byte aligned addresses:

kvm-clock: cpu 1, msr 0:3018aa5, secondary cpu clock
kvm-clock: cpu 2, msr 0:301f8e9, secondary cpu clock
kvm-clock: cpu 3, msr 0:302672d, secondary cpu clock

Quoted 3 lines

Check for an address+len that would cross page boundary instead.

Ugh. Is there guaranty that it will not register a memory region that crosses page boundary or it is pure luck that this does not happen?

The alignment check is not necessary given that "KVM: x86: Convert MSR_KVM_SYSTEM_TIME to use gfn_to_hva_cache" (0b79459b482e85cb742) uses kvm_write_guest which is able to handle data across page properly.

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +0 -0

index f19ac0a..ec830fa 100644

Quoted 6 lines · Marcelo Tosatti

The alignment check is not necessary given that "KVM: x86: Convert MSR_KVM_SYSTEM_TIME to use gfn_to_hva_cache" (0b79459b482e85cb742) uses kvm_write_guest which is able to handle data across page properly.

It uses kvm_write_guest_cached which calls to copy_to_user() directly. While this will not allow to overwrite page that does not belong to qemu process, it is possible to write outside of memory slot. May be we should use kvm_write_guest to do system time updates.

Quoted 7 lines

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +0 -0

index f19ac0a..ec830fa 100644

arch/x86/kvm/x86.c int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 1957

Quoted 10 lines

arch/x86/kvm/x86.c +0 -4

@@ -1952,10 +1952,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
              gpa_offset = data & ~(PAGE_MASK | 1);
 
-             /* Check that the address is 32-byte aligned. */
-             if (gpa_offset & (sizeof(struct pvclock_vcpu_time_info) - 1))
-                     break;
-
              if (kvm_gfn_to_hva_cache_init(vcpu->kvm,
                   &vcpu->arch.pv_time, data & ~1ULL))
                      vcpu->arch.pv_time_enabled = false;

--
Gleb.

kvm_write_guest would work, but it will hurt performance a bit because it'll be doing the address translation each time the time is updated, which happens on most guest enters.

Another possibility would be to change kvm_gfn_to_hva_cache_init to accept a size parameter. If the requested range is all on one page then it operates the same as it currently does. If the address range is on more than one page then it falls back to kvm_write_guest. This preserves the good performance for all cases that currently work, while still supporting the unlikely case of page straddling requests. It also makes it harder to write a security bugs for other callers of kvm_gfn_to_hva_cache_init by explicitly requiring a size parameter.

I can write a patch if you like the idea.

Andy

Quoted 32 lines · Gleb Natapov

Quoted 6 lines · Marcelo Tosatti

The alignment check is not necessary given that "KVM: x86: Convert MSR_KVM_SYSTEM_TIME to use gfn_to_hva_cache" (0b79459b482e85cb742) uses kvm_write_guest which is able to handle data across page properly.

It uses kvm_write_guest_cached which calls to copy_to_user() directly. While this will not allow to overwrite page that does not belong to qemu process, it is possible to write outside of memory slot. May be we should use kvm_write_guest to do system time updates.

Quoted 7 lines

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/x86.c +0 -0

index f19ac0a..ec830fa 100644

arch/x86/kvm/x86.c int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 1957

Quoted 10 lines

arch/x86/kvm/x86.c +0 -4

@@ -1952,10 +1952,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
              gpa_offset = data & ~(PAGE_MASK | 1);
 
-             /* Check that the address is 32-byte aligned. */
-             if (gpa_offset & (sizeof(struct pvclock_vcpu_time_info) - 1))
-                     break;
-
              if (kvm_gfn_to_hva_cache_init(vcpu->kvm,
                   &vcpu->arch.pv_time, data & ~1ULL))
                      vcpu->arch.pv_time_enabled = false;

--
Gleb.

Quoted 4 lines · Andrew Honig

kvm_write_guest would work, but it will hurt performance a bit because it'll be doing the address translation each time the time is updated, which happens on most guest enters.

Time updates are rare, so this should no be an issue. Marcelo?

Quoted 10 lines

Another possibility would be to change kvm_gfn_to_hva_cache_init to accept a size parameter. If the requested range is all on one page then it operates the same as it currently does. If the address range is on more than one page then it falls back to kvm_write_guest. This preserves the good performance for all cases that currently work, while still supporting the unlikely case of page straddling requests. It also makes it harder to write a security bugs for other callers of kvm_gfn_to_hva_cache_init by explicitly requiring a size parameter.

I can write a patch if you like the idea.

Nice idea. Send a patch please.