Hi Alexandru, On 9/10/21 10:28 AM, Alexandru Elisei wrote: > Hi Ricardo, > > On 9/9/21 5:47 PM, Ricardo Koller wrote: >> On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote: >>> Hi Ricardo, >>> >>> On 9/8/21 10:03 PM, Ricardo Koller wrote: >>>> Extend vgic_v3_check_base() to verify that the redistributor regions >>>> don't go above the VM-specified IPA size (phys_size). This can happen >>>> when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with: >>>> >>>> base + size > phys_size AND base < phys_size >>>> >>>> vgic_v3_check_base() is used to check the redist regions bases when >>>> setting them (with the vcpus added so far) and when attempting the first >>>> vcpu-run. >>>> >>>> Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx> >>>> --- >>>> arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c >>>> index 66004f61cd83..5afd9f6f68f6 100644 >>>> --- a/arch/arm64/kvm/vgic/vgic-v3.c >>>> +++ b/arch/arm64/kvm/vgic/vgic-v3.c >>>> @@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm) >>>> if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) < >>>> rdreg->base) >>>> return false; >>>> + >>>> + if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) > >>>> + kvm_phys_size(kvm)) >>>> + return false; >>> Looks to me like this same check (and the overflow one before it) is done when >>> adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base() >>> -> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell, >>> kvm_vgic_addr() handles both ways of setting the Redistributor address. >>> >>> Without this patch, did you manage to set a base address such that base + size > >>> kvm_phys_size()? >>> >> Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way >> to get to this situation is with the selftest in patch 2. I then tried >> an extra experiment: map the first redistributor, run the first vcpu, >> and access the redist from inside the guest. KVM didn't complain in any >> of these steps. > Yes, Eric pointed out that I was mistaken and there is no check being done for > base + size > kvm_phys_size(). > > What I was trying to say is that this check is better done when the user creates a > Redistributor region, not when a VCPU is first run. We have everything we need to > make the check when a region is created, why wait until the VCPU is run? > > For example, vgic_v3_insert_redist_region() is called each time the adds a new > Redistributor region (via either of the two APIs), and already has a check for the > upper limit overflowing (identical to the check in vgic_v3_check_base()). I would > add the check against the maximum IPA size there. you seem to refer to an old kernel as vgic_v3_insert_redist_region was renamed into vgic_v3_alloc_redist_region in e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region() I think in case you use the old rdist API you do not know yet the size of the redist region at this point (count=0), hence Ricardo's choice to do the check latter. > > Also, because vgic_v3_insert_redist_region() already checks for overflow, I > believe the overflow check in vgic_v3_check_base() is redundant. > > As far as I can tell, vgic_v3_check_base() is there to make sure that the > Distributor doesn't overlap with any of the Redistributors, and because the > Redistributors and the Distributor can be created in any order, we defer the check > until the first VCPU is run. I might be wrong about this, someone please correct > me if I'm wrong. > > Also, did you verify that KVM is also doing this check for GICv2? KVM does > something similar and calls vgic_v2_check_base() when mapping the GIC resources, > and I don't see a check for the maximum IPA size in that function either. I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it checks the base @) Thanks Eric > > Thanks, > > Alex > >> Thanks, >> Ricardo >> >>> Thanks, >>> >>> Alex >>> >>>> } >>>> >>>> if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))
