* KVM: x86: relax MSR_KVM_SYSTEM_TIME alignment check
@ 2013-03-22 19:14 Marcelo Tosatti
2013-03-22 19:21 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2013-03-22 19:14 UTC (permalink / raw)
To: Andy Honig, Gleb Natapov; +Cc: kvm
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 <mtosatti@redhat.com>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f19ac0a..ad36d386 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1952,8 +1952,9 @@ 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))
+ /* Check that address+len does not cross page boundary */
+ if ((gpa_offset + sizeof(struct pvclock_vcpu_time_info) - 1)
+ & PAGE_MASK)
break;
if (kvm_gfn_to_hva_cache_init(vcpu->kvm,
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: KVM: x86: relax MSR_KVM_SYSTEM_TIME alignment check
2013-03-22 19:14 KVM: x86: relax MSR_KVM_SYSTEM_TIME alignment check Marcelo Tosatti
@ 2013-03-22 19:21 ` Gleb Natapov
2013-03-22 19:47 ` Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-03-22 19:21 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Andy Honig, kvm
On Fri, Mar 22, 2013 at 04:14:07PM -0300, Marcelo Tosatti wrote:
>
> 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.
>
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?
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f19ac0a..ad36d386 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1952,8 +1952,9 @@ 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))
> + /* Check that address+len does not cross page boundary */
> + if ((gpa_offset + sizeof(struct pvclock_vcpu_time_info) - 1)
> + & PAGE_MASK)
> break;
>
> if (kvm_gfn_to_hva_cache_init(vcpu->kvm,
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: KVM: x86: relax MSR_KVM_SYSTEM_TIME alignment check
2013-03-22 19:21 ` Gleb Natapov
@ 2013-03-22 19:47 ` Marcelo Tosatti
2013-03-22 21:19 ` KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2013-03-22 19:47 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Andy Honig, kvm
On Fri, Mar 22, 2013 at 09:21:00PM +0200, Gleb Natapov wrote:
> On Fri, Mar 22, 2013 at 04:14:07PM -0300, Marcelo Tosatti wrote:
> >
> > 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.
> >
> 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?
Pure build time luck - there is no guarantee that percpu data will not
cross page boundary AFAIK.
^ permalink raw reply [flat|nested] 8+ messages in thread
* KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address
2013-03-22 19:47 ` Marcelo Tosatti
@ 2013-03-22 21:19 ` Marcelo Tosatti
2013-03-22 21:57 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2013-03-22 21:19 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Andy Honig, kvm
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 <mtosatti@redhat.com>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f19ac0a..ec830fa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address
2013-03-22 21:19 ` KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address Marcelo Tosatti
@ 2013-03-22 21:57 ` Gleb Natapov
2013-03-23 0:17 ` Andrew Honig
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-03-22 21:57 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Andy Honig, kvm
On Fri, Mar 22, 2013 at 06:19:47PM -0300, Marcelo Tosatti wrote:
>
> 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.
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f19ac0a..ec830fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address
2013-03-22 21:57 ` Gleb Natapov
@ 2013-03-23 0:17 ` Andrew Honig
2013-03-23 14:12 ` Gleb Natapov
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Honig @ 2013-03-23 0:17 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Marcelo Tosatti, kvm, Eric Northup
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
On Fri, Mar 22, 2013 at 2:57 PM, Gleb Natapov <gleb@redhat.com> wrote:
> On Fri, Mar 22, 2013 at 06:19:47PM -0300, Marcelo Tosatti wrote:
>>
>> 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.
>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index f19ac0a..ec830fa 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address
2013-03-23 0:17 ` Andrew Honig
@ 2013-03-23 14:12 ` Gleb Natapov
2013-04-11 16:05 ` Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Gleb Natapov @ 2013-03-23 14:12 UTC (permalink / raw)
To: Andrew Honig; +Cc: Marcelo Tosatti, kvm, Eric Northup
On Fri, Mar 22, 2013 at 05:17:38PM -0700, Andrew Honig wrote:
> 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?
> 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.
>
> Andy
>
> On Fri, Mar 22, 2013 at 2:57 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Fri, Mar 22, 2013 at 06:19:47PM -0300, Marcelo Tosatti wrote:
> >>
> >> 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.
> >
> >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index f19ac0a..ec830fa 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -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.
--
Gleb.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address
2013-03-23 14:12 ` Gleb Natapov
@ 2013-04-11 16:05 ` Marcelo Tosatti
0 siblings, 0 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2013-04-11 16:05 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Andrew Honig, kvm, Eric Northup
On Sat, Mar 23, 2013 at 04:12:11PM +0200, Gleb Natapov wrote:
> On Fri, Mar 22, 2013 at 05:17:38PM -0700, Andrew Honig wrote:
> > 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?
Yes, performance is not an issue at this level.
> > 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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-04-11 16:05 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-22 19:14 KVM: x86: relax MSR_KVM_SYSTEM_TIME alignment check Marcelo Tosatti
2013-03-22 19:21 ` Gleb Natapov
2013-03-22 19:47 ` Marcelo Tosatti
2013-03-22 21:19 ` KVM: x86: drop alignment checks from KVM_MSR_SYSTEM_TIME address Marcelo Tosatti
2013-03-22 21:57 ` Gleb Natapov
2013-03-23 0:17 ` Andrew Honig
2013-03-23 14:12 ` Gleb Natapov
2013-04-11 16:05 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox