Re: [PATCH] kvm: use PIT channel index in hpet_legacy_start mode

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/01/2016 13:37, Yang Zhang wrote:
> On 2016/1/7 20:32, P J P wrote:
>> From: P J P <pjp@fedoraproject.org>
>>
>> While setting the KVM PIT counters in 'kvm_pit_load_count', if
>> 'hpet_legacy_start' is set, the function disables the timer on
>> channel[0], instead of the respective index 'channel'. Update it
>> to use 'channel' index parameter.
>>
>> Signed-off-by: P J P <pjp@fedoraproject.org>
>> ---
>>   arch/x86/kvm/i8254.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>> index 08116ff..154e936 100644
>> --- a/arch/x86/kvm/i8254.c
>> +++ b/arch/x86/kvm/i8254.c
>> @@ -420,10 +420,11 @@ void kvm_pit_load_count(struct kvm *kvm, int
>> channel, u32 val, int hpet_legacy_s
>>       u8 saved_mode;
>>       if (hpet_legacy_start) {
>>           /* save existing mode for later reenablement */
>> -        saved_mode = kvm->arch.vpit->pit_state.channels[0].mode;
>> -        kvm->arch.vpit->pit_state.channels[0].mode = 0xff; /* disable
>> timer */
>> +        saved_mode = kvm->arch.vpit->pit_state.channels[channel].mode;
>> +        /* disable timer */
>> +        kvm->arch.vpit->pit_state.channels[channel].mode = 0xff;
>>           pit_load_count(kvm, channel, val);
>> -        kvm->arch.vpit->pit_state.channels[0].mode = saved_mode;
>> +        kvm->arch.vpit->pit_state.channels[channel].mode = saved_mode;
>>       } else {
>>           pit_load_count(kvm, channel, val);
>>       }
>>
> 
> Will this trigger the same issue like CVE-2015-7513 ?
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0185604c2d82c560dab2f2933a18f797e74ab5a8

I am not sure (--verbose please :)) but the right fix is to change the caller
like this:

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 08116ff227cc..b0ea42b78ccd 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -420,6 +420,7 @@ void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_s
 	u8 saved_mode;
 	if (hpet_legacy_start) {
 		/* save existing mode for later reenablement */
+		WARN_ON(channel != 0);
 		saved_mode = kvm->arch.vpit->pit_state.channels[0].mode;
 		kvm->arch.vpit->pit_state.channels[0].mode = 0xff; /* disable timer */
 		pit_load_count(kvm, channel, val);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7ffc224bbe41..97592e190413 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3606,7 +3606,8 @@ static int kvm_vm_ioctl_set_pit2(struct kvm *kvm, struct kvm_pit_state2 *ps)
 	       sizeof(kvm->arch.vpit->pit_state.channels));
 	kvm->arch.vpit->pit_state.flags = ps->flags;
 	for (i = 0; i < 3; i++)
-		kvm_pit_load_count(kvm, i, kvm->arch.vpit->pit_state.channels[i].count, start);
+		kvm_pit_load_count(kvm, i, kvm->arch.vpit->pit_state.channels[i].count,
+				   start && i == 0);
 	mutex_unlock(&kvm->arch.vpit->pit_state.lock);
 	return 0;
 }

Thanks,

Paolo