* [PATCH] KVM: arm/arm64: Assign pmc->idx before kvm_pmu_stop_counter() @ 2019-07-17 12:20 Zenghui Yu 2019-07-17 13:44 ` Julien Thierry 0 siblings, 1 reply; 4+ messages in thread From: Zenghui Yu @ 2019-07-17 12:20 UTC (permalink / raw) To: maz, kvmarm, linux-arm-kernel; +Cc: marc.zyngier, linux-kernel We use "pmc->idx" and the "chained" bitmap to determine if the pmc is chained, in kvm_pmu_pmc_is_chained(). But idx might be uninitialized (and random) when we doing this decision, through a KVM_ARM_VCPU_INIT ioctl -> kvm_pmu_vcpu_reset(). And the test_bit() against this random idx will potentially hit a KASAN BUG [1]. Fix it by moving the assignment of idx before kvm_pmu_stop_counter(). [1] https://www.spinics.net/lists/kvm-arm/msg36700.html Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters") Suggested-by: Andrew Murray <andrew.murray@arm.com> Cc: Marc Zyngier <maz@kernel.org> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> --- virt/kvm/arm/pmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c index 3dd8238..521bfdd 100644 --- a/virt/kvm/arm/pmu.c +++ b/virt/kvm/arm/pmu.c @@ -225,8 +225,8 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu) struct kvm_pmu *pmu = &vcpu->arch.pmu; for (i = 0; i < ARMV8_PMU_MAX_COUNTERS; i++) { - kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); pmu->pmc[i].idx = i; + kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); } bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS); -- 1.8.3.1 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: arm/arm64: Assign pmc->idx before kvm_pmu_stop_counter() 2019-07-17 12:20 [PATCH] KVM: arm/arm64: Assign pmc->idx before kvm_pmu_stop_counter() Zenghui Yu @ 2019-07-17 13:44 ` Julien Thierry 2019-07-17 15:00 ` Marc Zyngier 0 siblings, 1 reply; 4+ messages in thread From: Julien Thierry @ 2019-07-17 13:44 UTC (permalink / raw) To: Zenghui Yu, maz, kvmarm, linux-arm-kernel; +Cc: marc.zyngier, linux-kernel Hi Zenghui, On 17/07/2019 13:20, Zenghui Yu wrote: > We use "pmc->idx" and the "chained" bitmap to determine if the pmc is > chained, in kvm_pmu_pmc_is_chained(). But idx might be uninitialized > (and random) when we doing this decision, through a KVM_ARM_VCPU_INIT > ioctl -> kvm_pmu_vcpu_reset(). And the test_bit() against this random > idx will potentially hit a KASAN BUG [1]. > > Fix it by moving the assignment of idx before kvm_pmu_stop_counter(). > > [1] https://www.spinics.net/lists/kvm-arm/msg36700.html > > Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters") > Suggested-by: Andrew Murray <andrew.murray@arm.com> > Cc: Marc Zyngier <maz@kernel.org> > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>> --- > virt/kvm/arm/pmu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > index 3dd8238..521bfdd 100644 > --- a/virt/kvm/arm/pmu.c > +++ b/virt/kvm/arm/pmu.c > @@ -225,8 +225,8 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu) > struct kvm_pmu *pmu = &vcpu->arch.pmu; > > for (i = 0; i < ARMV8_PMU_MAX_COUNTERS; i++) { > - kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); > pmu->pmc[i].idx = i; Yes, this is kind of a static property that should really be part of a "kvm_pmu_vcpu_init()" or "kvm_pmu_vcpu_create()" and is not expected to be modified across resets... There is no such function at the time and I'm unsure whether this warrants creating that separate function (I would still suggest creating it to make things clearer). > + kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); Whatever other opinions are on splitting pmu_vcpu_init/reset, that change makes sense and fixes the issue: Acked-by: Julien Thierry <julien.thierry@arm.com> > } > > bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS); > Cheers, -- Julien Thierry _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: arm/arm64: Assign pmc->idx before kvm_pmu_stop_counter() 2019-07-17 13:44 ` Julien Thierry @ 2019-07-17 15:00 ` Marc Zyngier 2019-07-18 1:59 ` Zenghui Yu 0 siblings, 1 reply; 4+ messages in thread From: Marc Zyngier @ 2019-07-17 15:00 UTC (permalink / raw) To: Julien Thierry, Zenghui Yu, kvmarm, linux-arm-kernel Cc: marc.zyngier, linux-kernel On 17/07/2019 14:44, Julien Thierry wrote: > Hi Zenghui, > > On 17/07/2019 13:20, Zenghui Yu wrote: >> We use "pmc->idx" and the "chained" bitmap to determine if the pmc is >> chained, in kvm_pmu_pmc_is_chained(). But idx might be uninitialized >> (and random) when we doing this decision, through a KVM_ARM_VCPU_INIT >> ioctl -> kvm_pmu_vcpu_reset(). And the test_bit() against this random >> idx will potentially hit a KASAN BUG [1]. >> >> Fix it by moving the assignment of idx before kvm_pmu_stop_counter(). >> >> [1] https://www.spinics.net/lists/kvm-arm/msg36700.html >> >> Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters") >> Suggested-by: Andrew Murray <andrew.murray@arm.com> >> Cc: Marc Zyngier <maz@kernel.org> >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>> --- >> virt/kvm/arm/pmu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c >> index 3dd8238..521bfdd 100644 >> --- a/virt/kvm/arm/pmu.c >> +++ b/virt/kvm/arm/pmu.c >> @@ -225,8 +225,8 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu) >> struct kvm_pmu *pmu = &vcpu->arch.pmu; >> >> for (i = 0; i < ARMV8_PMU_MAX_COUNTERS; i++) { >> - kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); >> pmu->pmc[i].idx = i; > > Yes, this is kind of a static property that should really be part of a > "kvm_pmu_vcpu_init()" or "kvm_pmu_vcpu_create()" and is not expected to > be modified across resets... > > There is no such function at the time and I'm unsure whether this > warrants creating that separate function (I would still suggest creating > it to make things clearer). Yup, that's pretty bad, now that you mention it. I'd be all for the introduction of kvm_pmu_vcpu_init(), given that we already have kvm_pmu_vcpu_destroy(). > >> + kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); > > Whatever other opinions are on splitting pmu_vcpu_init/reset, that > change makes sense and fixes the issue: > > Acked-by: Julien Thierry <julien.thierry@arm.com> > >> } >> >> bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS); >> > > Cheers, > Zenghui, could you please update your patch to take the above into account? 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: arm/arm64: Assign pmc->idx before kvm_pmu_stop_counter() 2019-07-17 15:00 ` Marc Zyngier @ 2019-07-18 1:59 ` Zenghui Yu 0 siblings, 0 replies; 4+ messages in thread From: Zenghui Yu @ 2019-07-18 1:59 UTC (permalink / raw) To: Marc Zyngier, Julien Thierry, kvmarm, linux-arm-kernel Cc: marc.zyngier, linux-kernel Hi Julien, Marc, On 2019/7/17 23:00, Marc Zyngier wrote: > On 17/07/2019 14:44, Julien Thierry wrote: >> Hi Zenghui, >> >> On 17/07/2019 13:20, Zenghui Yu wrote: >>> We use "pmc->idx" and the "chained" bitmap to determine if the pmc is >>> chained, in kvm_pmu_pmc_is_chained(). But idx might be uninitialized >>> (and random) when we doing this decision, through a KVM_ARM_VCPU_INIT >>> ioctl -> kvm_pmu_vcpu_reset(). And the test_bit() against this random >>> idx will potentially hit a KASAN BUG [1]. >>> >>> Fix it by moving the assignment of idx before kvm_pmu_stop_counter(). >>> >>> [1] https://www.spinics.net/lists/kvm-arm/msg36700.html >>> >>> Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters") >>> Suggested-by: Andrew Murray <andrew.murray@arm.com> >>> Cc: Marc Zyngier <maz@kernel.org> >>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>> --- >>> virt/kvm/arm/pmu.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c >>> index 3dd8238..521bfdd 100644 >>> --- a/virt/kvm/arm/pmu.c >>> +++ b/virt/kvm/arm/pmu.c >>> @@ -225,8 +225,8 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu) >>> struct kvm_pmu *pmu = &vcpu->arch.pmu; >>> >>> for (i = 0; i < ARMV8_PMU_MAX_COUNTERS; i++) { >>> - kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); >>> pmu->pmc[i].idx = i; >> >> Yes, this is kind of a static property that should really be part of a >> "kvm_pmu_vcpu_init()" or "kvm_pmu_vcpu_create()" and is not expected to >> be modified across resets... >> >> There is no such function at the time and I'm unsure whether this >> warrants creating that separate function (I would still suggest creating >> it to make things clearer). > > Yup, that's pretty bad, now that you mention it. I'd be all for the > introduction of kvm_pmu_vcpu_init(), given that we already have > kvm_pmu_vcpu_destroy(). > >> >>> + kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]); >> >> Whatever other opinions are on splitting pmu_vcpu_init/reset, that >> change makes sense and fixes the issue: >> >> Acked-by: Julien Thierry <julien.thierry@arm.com> >> >>> } >>> >>> bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS); >>> >> >> Cheers, >> > > Zenghui, could you please update your patch to take the above into account? Sure. I will send a v2 with the new subject (may be "KVM: arm/arm64: Introduce kvm_pmu_vcpu_init() to ..."). Thanks for your suggestions! zenghui _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-18 2:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-17 12:20 [PATCH] KVM: arm/arm64: Assign pmc->idx before kvm_pmu_stop_counter() Zenghui Yu 2019-07-17 13:44 ` Julien Thierry 2019-07-17 15:00 ` Marc Zyngier 2019-07-18 1:59 ` Zenghui Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).