[PATCH] KVM: arm64: Use the correct timer for accessing CNT

[PATCH] KVM: arm64: Use the correct timer for accessing CNT

[KarimAllah Ahmed]

Use the physical timer object when reading the physical timer counter
instead of using the virtual timer object. This is only visible when
reading it from user-space as kvm_arm_timer_get_reg() is only executed on
the get register patch from user-space.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.cs.columbia.edu
Cc: linux-kernel@vger.kernel.org
Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
---
 virt/kvm/arm/arch_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 0d9438e..93bd59b 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -788,7 +788,7 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
 					  vcpu_ptimer(vcpu), TIMER_REG_CTL);
 	case KVM_REG_ARM_PTIMER_CNT:
 		return kvm_arm_timer_read(vcpu,
-					  vcpu_vtimer(vcpu), TIMER_REG_CNT);
+					  vcpu_ptimer(vcpu), TIMER_REG_CNT);
 	case KVM_REG_ARM_PTIMER_CVAL:
 		return kvm_arm_timer_read(vcpu,
 					  vcpu_ptimer(vcpu), TIMER_REG_CVAL);
-- 
2.7.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Use the correct timer for accessing CNT

[Zenghui Yu]

Hi,

On 2020/3/16 17:39, KarimAllah Ahmed wrote:
> Use the physical timer object when reading the physical timer counter
> instead of using the virtual timer object. This is only visible when
> reading it from user-space as kvm_arm_timer_get_reg() is only executed on
> the get register patch from user-space.

s/patch/path/

I think the physical counter hasn't yet been accessed by the current
userspace, wrong?

> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>

Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>

And this might also deserve:

Fixes: 84135d3d18da ("KVM: arm/arm64: consolidate arch timer trap handlers")


Thanks.

> ---
>   virt/kvm/arm/arch_timer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 0d9438e..93bd59b 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -788,7 +788,7 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
>   					  vcpu_ptimer(vcpu), TIMER_REG_CTL);
>   	case KVM_REG_ARM_PTIMER_CNT:
>   		return kvm_arm_timer_read(vcpu,
> -					  vcpu_vtimer(vcpu), TIMER_REG_CNT);
> +					  vcpu_ptimer(vcpu), TIMER_REG_CNT);
>   	case KVM_REG_ARM_PTIMER_CVAL:
>   		return kvm_arm_timer_read(vcpu,
>   					  vcpu_ptimer(vcpu), TIMER_REG_CVAL);
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Use the correct timer for accessing CNT

[Marc Zyngier]

Hi Zenghui,

On 2020-03-16 10:49, Zenghui Yu wrote:
> Hi,
> 
> On 2020/3/16 17:39, KarimAllah Ahmed wrote:
>> Use the physical timer object when reading the physical timer counter
>> instead of using the virtual timer object. This is only visible when
>> reading it from user-space as kvm_arm_timer_get_reg() is only executed 
>> on
>> the get register patch from user-space.
> 
> s/patch/path/
> 
> I think the physical counter hasn't yet been accessed by the current
> userspace, wrong?

I don't think userspace can access it, as the ONE_REG API only exposes 
the virtual
timer so far, and userspace is much better off just reading the counter 
directly
(it has access to the virtual counter, and the guarantee that cntvoff is 
0 in this
context).

But as we move towards a situation where we can save/restore the 
physical timer
just like the virtual one, we're going to use this path and hit this 
bug.

> 
>> 
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: kvmarm@lists.cs.columbia.edu
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: KarimAllah Ahmed <karahmed@amazon.de>
> 
> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com>
> 
> And this might also deserve:
> 
> Fixes: 84135d3d18da ("KVM: arm/arm64: consolidate arch timer trap 
> handlers")

Indeed. Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Use the correct timer for accessing CNT

[Zenghui Yu]

Hi Marc,

On 2020/3/16 19:09, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On 2020-03-16 10:49, Zenghui Yu wrote:
>> Hi,
>>
>> On 2020/3/16 17:39, KarimAllah Ahmed wrote:
>>> Use the physical timer object when reading the physical timer counter
>>> instead of using the virtual timer object. This is only visible when
>>> reading it from user-space as kvm_arm_timer_get_reg() is only 
>>> executed on
>>> the get register patch from user-space.
>>
>> s/patch/path/
>>
>> I think the physical counter hasn't yet been accessed by the current
>> userspace, wrong?
> 
> I don't think userspace can access it, as the ONE_REG API only exposes 
> the virtual
> timer so far, and userspace is much better off just reading the counter 
> directly
> (it has access to the virtual counter, and the guarantee that cntvoff is 
> 0 in this
> context).

Yeah, I see. The physical timer registers are all ignored in
walk_one_sys_reg() and won't be exposed.

> 
> But as we move towards a situation where we can save/restore the 
> physical timer
> just like the virtual one, we're going to use this path and hit this bug.

Thanks for the explanation.


Zenghui

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Use the correct timer for accessing CNT

[James Morse]

Hi KarimAllah,

On 3/16/20 9:39 AM, KarimAllah Ahmed wrote:
> Use the physical timer object when reading the physical timer counter
> instead of using the virtual timer object. This is only visible when
> reading it from user-space as kvm_arm_timer_get_reg() is only executed on
> the get register patch from user-space.

Have you seen this go wrong?

I agree this looks like this was a typo introduced by:
84135d3d1 ("KVM: arm/arm64: consolidate arch timer trap handlers")
-----------------%<-----------------
        case KVM_REG_ARM_PTIMER_CNT:
-               return kvm_phys_timer_read();
+               return kvm_arm_timer_read(vcpu,
+                                         vcpu_vtimer(vcpu), TIMER_REG_CNT);
-----------------%<-----------------

This would be a problem when the guest reads the physical counter
directly, (which doesn't get trapped), and the VMM makes this API call
and gets a number in a totally different ball-park.


Can the VMM actually read these registers with this path?

kvm_arm_get_reg() gets to filter out the coproc registers that aren't in
the sys_reg[], it also uses is_timer_reg() to spot the timer/counter
registers, but is_timer_reg() only matches three of them:
|	case KVM_REG_ARM_TIMER_CTL:
|	case KVM_REG_ARM_TIMER_CNT:
|	case KVM_REG_ARM_TIMER_CVAL:

KVM_REG_ARM_PTIMER_CNT is not one of them.

It looks like when the VMM tries to read this, it fails is_timer_reg(),
and matches in the sys_regs[] and is handled by access_arch_timer(),
which uses kvm_arm_timer_read_sysreg() -> kvm_arm_timer_read(),
bypassing this bug.

... this looks like a bug in dead code ...


Thanks!

James

> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 0d9438e..93bd59b 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -788,7 +788,7 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
>  					  vcpu_ptimer(vcpu), TIMER_REG_CTL);
>  	case KVM_REG_ARM_PTIMER_CNT:
>  		return kvm_arm_timer_read(vcpu,
> -					  vcpu_vtimer(vcpu), TIMER_REG_CNT);
> +					  vcpu_ptimer(vcpu), TIMER_REG_CNT);
>  	case KVM_REG_ARM_PTIMER_CVAL:
>  		return kvm_arm_timer_read(vcpu,
>  					  vcpu_ptimer(vcpu), TIMER_REG_CVAL);
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm